How I review code • Hacker News Board

Re: How I review code

#141

Post by LandR » Thu, Jan 25, 2018, 11:21 AM UTC

watwut wrote: post #68

LandR wrote: post #63

Earlier quoted context omitted.

This, this and this. You get told to quickly write something as a Proof of Concept. Then get told, it works so put it in the code and reelase it. We will rewrite it later. You never get to rewrite it later. Over time the code base goes to hell. Every single time

I have hard time to believe you never refactor function you come across. I see how refactoring whole architecture or something major is avoided, but when it comes to smaller functions and pieces of code, refactoring it as you go takes around same amount of time as refactoring it during that detailed code review (e.g. often very little). The really big mess tend to be emergent - when features and code base grew too mu…

At my last job, if you had unecessary changes for the work item (e.g. refactoring) you failed the code review.

But then we had 5% unit test coverage on a 5million+ LOC code base, they were terrified that refactoring (with no tests to back it up) might break things.

We were writing medical software and they were very risk adverse (obviously), so if i refactored a method I would have to raise that and would mean test cases would have to be written and manually run to ensure I hadn't broken anything.

So refactoring hardly happened (The code was an absolute mess), when i left they were working on a project to fix it but the timescales for it were 5 to 10 years.

Re: How I review code

#142

Post by darkerside » Fri, Jan 26, 2018, 2:54 AM UTC

cobbzilla wrote: post #75

darkerside wrote: post #64

Earlier quoted context omitted.

> I echo the author's point in "Review the code with its author in mind". I don't understand how the rest of your post relates to that, although I think it's an interesting point and following discussion. On the topic of reviewing code with the author in mind, I'm not sure I agree at all with the linked article. Does it matter who wrote the code in any way? Good code is good, and bad code is bad. It may be a helpful…

If you know your team well, it will help you keep an eye out for common mistakes they've made in the past. It may also help adjust your tone, as developers you've worked with for a long time will understand light humor or other well-intended comments that might be read as off-putting by newer devs.

The anti-pattern to avoid here is assuming code written by senior engineers is inherently "better" in some way than that by a junior. Yes, it typically is, but the code should speak for itself. Ad hominem assumptions add little value.

Similar to blind testing in musical auditions. http://gap.hks.harvard.edu/orchestrating-impartiality-impact...

Re: How I review code

#143

Post by menor » Sat, Jan 27, 2018, 1:55 PM UTC

watwut wrote: post #9

gizmo686 wrote: post #4

Earlier quoted context omitted.

>obsessed with correctness, and need to be explained why each change is okay. Isn't this the point of code review? When I click accept on a code review, I am saying that I have looked over the change and believe that it is correct and okay. If I just arrive at the conclusion by saying "Joe wrote this, and I trust Joe" the there is no point in me reviewing it.

It continues with "and need to be explained why each change is okay". If reviewer suspects there is a bug, reviewer should check for it instead of having reviewee explain him or defend every detail. It is micromanagement this way - not just being similar or kinda like micromanagement, but it is literally it. If every five lines big code change in run of the mill fronted requires two hours long negotiation, then somet…

> I review mostly for architectural compliance and "bad idioms" or code smells. There is difference between not like I would write it and badly written mess and many programmers confuse them.

This ^ I just had this discussion last week in a PR, if the comment is a matter of opinion, the whole team should agree and put a rule in the linter or style guide. If not every one is free to have their own preferences.

Re: How I review code

#144

Post by e12e » Tue, Jan 30, 2018, 10:59 PM UTC

jrs95 wrote: post #94

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…

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?

> 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?

Isn't this the basic architecture of a node js "event loop" Cms that gets content from a rdbms? ;-)

  # pseudo code
  While new_user_request
    query_db with request parameters

I know it's not what you meant, you meant do the above, rather than:

    # pseudo code
  While new_user_request
    query_db for user data
    query_db for session data

    For parameter,
        user_data,
        session_data
        query_db with each item