Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

How I review code

engineering.tumblr.com

Post reply on HN
Print view View original

91–100 of 144 posts

Re: How I review code

#91

Post by jrs95 »

megous wrote: post #78
stctgion wrote: post #65

Earlier quoted context omitted.

Totally agree with this. I've had to pick up the pieces on more than one occasion when some key piece of code was written in the flavour of the month and the writer shortly left. I've heard other Devs say you should pick the right language for the job but I think there should be a heavy bias to what the rest of the code base already is using

None of those, except perhaps for the golang are a flavor of the month languages. Using "Right tool for the job" is also important.

And golang is a relativley "safe" FOTM language in that basically anybody ought to be able to learn enough about it to work with it in a short amount of time. When Scala was a FOTM, it probably did a lot more damage.

Re: How I review code

#92

Post by [deleted] »

cousin_it wrote: post #49
taylodl wrote: post #29

"Senior engineers sometimes need to be reminded that highly performant, abstract, or clever code is often difficult to read and understand later, which usually means asking them for more inline comments and documentation." Ha! That's not a senior engineer. Senior engineers write the most simple-looking code that just works. In every rainy day scenario imaginable. The clever code writers aren't there yet.

Ha! That's good engineers. Senior engineers write an ambitious unusable framework, get promoted and move on to the next project. So the article's advice is ironically spot on.

[deleted]

Re: How I review code

#93

Post by lcfcjs2 »

anameaname wrote: post #3

A conundrum for me is how to get other people to code review the way I want to be code reviewed? Particularly, I noticed code reviewers on my team are pretty pedantic, obsessed with correctness, and need to be explained why each change is okay. These are people that regularly write good quality code themselves, but there is a high amount of distrust. Why doesn't a team of talented programmers trust each other? (in ca…

Don't take it so personally. If they are asking you to make changes in an unfriendly way, make a joke like "Tell me how you really feel :)". They will get the message and take a second to consider your comment. Code reviews are particularly difficult in a toxic environment, it's good to talk about things before they escalate.

Re: How I review code

#94

Post by jrs95 »

ot wrote: post #60
taylodl wrote: post #29

"Senior engineers sometimes need to be reminded that highly performant, abstract, or clever code is often difficult to read and understand later, which usually means asking them for more inline comments and documentation." Ha! That's not a senior engineer. Senior engineers write the most simple-looking code that just works. In every rainy day scenario imaginable. The clever code writers aren't there yet.

Reminds me of the "Evolution of a Haskell programmer": https://www.willamette.edu/~fruehr/haskell/evolution.html Make sure you don't miss the punchline, "Tenured professor". It's the same with the progression of engineering seniority: increasing levels of cleverness and unnecessary sophistication, until you reach a point where you don't have anything to prove anymore, and you can feel comfortable writing the simplest…

It's kind of amusing to me that a lot of engineering interviews also seem to be focused on doing things that you generally won't and shouldn't be doing for the position. What's the point of seeing if someone knows data structures and algorithms they won't be using if they can't write a web application that interfaces with a SQL database without doing queries in a for loop?

Re: How I review code

#95

Post by Unkechaug »

madsohm wrote: post #82
ot wrote: post #60

Earlier quoted context omitted.

Reminds me of the "Evolution of a Haskell programmer": https://www.willamette.edu/~fruehr/haskell/evolution.html Make sure you don't miss the punchline, "Tenured professor". It's the same with the progression of engineering seniority: increasing levels of cleverness and unnecessary sophistication, until you reach a point where you don't have anything to prove anymore, and you can feel comfortable writing the simplest…

I've been doing a lot of Haskell on Codewars.com recently. This is exactly what I see. I write up a long solution, that uses the basics like pattern matching, heads of lists and such. The solution that has the most "Best practise" up-votes are usually something involving importing control.monad and other similar stuff.

Sites like Codewars seem to me like good ideas starting out, but it doesn't take that long to turn into an arms race of complex one liners that people vote up as the coolest solution, not the best in terms of productivity for a company.

Re: How I review code

#96

Post by hinkley »

taylodl wrote: post #29

"Senior engineers sometimes need to be reminded that highly performant, abstract, or clever code is often difficult to read and understand later, which usually means asking them for more inline comments and documentation." Ha! That's not a senior engineer. Senior engineers write the most simple-looking code that just works. In every rainy day scenario imaginable. The clever code writers aren't there yet.

We call people senior after five years. Mastery can take a lot longer than that (and mastery doesn’t mean the end of learning).

Sooner or later someone will have to debug this code late at night. Don’t make it require brain cells.

Re: How I review code

#97

Post by thetruthseeker1 »

I don’t know how much time he spends code reviewing. But if at the end of the day he wants anybody to get the complete context of what the change entails by looking at the PR... I would think the code review process is more elaborate and time consuming than many companies can afford.

Re: How I review code

#98

Post by hinkley »

madsohm wrote: post #82
ot wrote: post #60

Earlier quoted context omitted.

Reminds me of the "Evolution of a Haskell programmer": https://www.willamette.edu/~fruehr/haskell/evolution.html Make sure you don't miss the punchline, "Tenured professor". It's the same with the progression of engineering seniority: increasing levels of cleverness and unnecessary sophistication, until you reach a point where you don't have anything to prove anymore, and you can feel comfortable writing the simplest…

I've been doing a lot of Haskell on Codewars.com recently. This is exactly what I see. I write up a long solution, that uses the basics like pattern matching, heads of lists and such. The solution that has the most "Best practise" up-votes are usually something involving importing control.monad and other similar stuff.

If you write code that other people have to ask you about, you will reach a point where you don’t have time to write code anymore because you’re too busy answering for your own code. You may interpret this as “helping your less fortunate teammates” but the wise know they can reduce your influence on the team by sending more people to ask you questions.

Giving them more time to clean up your mess and you none to make it worse.

Re: How I review code

#99

Post by hinkley »

cousin_it wrote: post #49
taylodl wrote: post #29

"Senior engineers sometimes need to be reminded that highly performant, abstract, or clever code is often difficult to read and understand later, which usually means asking them for more inline comments and documentation." Ha! That's not a senior engineer. Senior engineers write the most simple-looking code that just works. In every rainy day scenario imaginable. The clever code writers aren't there yet.

Ha! That's good engineers. Senior engineers write an ambitious unusable framework, get promoted and move on to the next project. So the article's advice is ironically spot on.

Tell me more about getting them to move on. I’m asking for a friend.

Re: How I review code

#100

Post by MaxBarraclough »

ams6110 wrote: post #83
MaxBarraclough wrote: post #25

Earlier quoted context omitted.

Disagree. A code-review is not purely checking for non-functional properties of the code, it's checking for overall code quality, and that includes bugs. Occasionally a reviewer will spot a bug. Occasionally there will be a false positive that turns out not to be a bug. You really want to deliberately discard this bug-finding opportunity? Why? Even if it's a false positive, doesn't that indicate that something bears…

> Occasionally a reviewer will spot a bug Isn't the code review generally pretty late, i.e. just prior to release? At that point, the code should be passing all unit tests and I'd expect obvious bugs to be pretty unlikely. Non-obvious bugs generally won't be spotted in a code review setting.

Not impossible that the reviewer might spot a bug. There's value in more eyeballs. Useful for spotting dangerous constructs or things like UB in C/C++ that appear to work fine... for now.

Also, lots of software houses don't have formal testing of all new code. Not unusual with GUI code, for instance.

Post reply on HN