7

I’m a senior frontend developer. My career is going great but I feel like Im terrible at reviewing other peoples code. There’s a much higher level of complexity that I can understand when it’s code Ive written compared to reading other people.

I know this is true for everyone to some extent, but not sure if I’m worse than average. I also don't know how much care other people are putting into code reviews.

I’m also dyslexics which makes it hard to remember lots of variable names across multiple files. Anyone else feel this way? Or has anyone else had this issue but come up with a strategy that helps?

  • What tools are you using for code review? An IDE can help greatly. – Michael Jaros Aug 21 at 11:53
  • Do you have the developer walk you through it first? We used to do that for code reviews of complex assembly language programs. – Laconic Droid Aug 21 at 12:21
  • 1
    This is not a workplace.SE question. – Chris Aug 21 at 17:30
17

It depends on the scope What are you reviewing? You can't possibly check all changes for bugs, or that they meet the Acceptance Criteria. That's the developer's and QA's job to do.

Key takeaway: You have to speak with your team and agree what the scope of your code reviews should be.

For me, I limit myself to reviewing code conventions:

  • Formatting. In some projects we use pre-commit hooks to deal with formatting,. In others we don't, and I've seen some horrible stuff. For me that's grounds to put the PR in the penalty box, because when the next developer does something in the same area and they format the code, they will prevent the previous developer's commit message being immediately accessible.
  • Weird / inefficient expressions. For example, someone doing a bunch of loops and if-statements when the same can be done with Java 8 streams.
  • Common pitfalls for your language. For example in Java I always flag str.equals("constant") and ask them to change it to "constant".equals(str). The OWASP list of common vulnerabilities is an excellent source for this, and they have sections for all major languages.
  • Documentation. Flag any public interfaces that aren't documented, or if the documentation does not agree with the code.
  • Naming conventions. We have them, and we have them for a reason, and PRs is the place to make sure they're followed.
  • Library abuse. If I'm familiar with a library, internal or external, I will make sure it's used properly. This is the closest I come to your review style, and not everyone can do this on every PR.
  • Magic Constants Applies to all languages but I've seen it in SCSS more than others. For example, setting the element colour to a raw hex value and not using a variable.
  • Language keys Same as magic constants above. My current project is multi-lingual and sometimes we forget ourselves and hardcode UI labels when we should be fetching them via language keys. Pretty common issue, easy to spot.
  • Others. I'm sure there are other things that escape me now.

I should also mention, a lot of these things can be cut down by using a linter (ie. for CSS, JS) or static code analysis (ie. Sonar for Java, C#).

At one time in my current project (when we first started doing code reviews) the procedure was to download the branch and test it against the Acceptance Criteria on the ticket. That didn't last long because it created a huge backlog, so, don't make more work for yourself than you should.

  • 1
    I agree with you answer, but what would say that I review pretty much the exact opposite. At least in my work, it is possible to spot bugs or issues that would have only been found by chance by the developer, and would hopefully get picked up by QA. Important thing is people understand what has been reviewed, which is your main take-away. – Gregory Currie Aug 21 at 12:18
  • 1
    Also, the str.equals "pitfall" really only makes sense in situations where Null is allowed. Otherwise you're just likely to be hiding bugs. – Gregory Currie Aug 21 at 12:21
  • "Magic Numbers" are my giant huge peeve. Good on you for calling them out. – Julie in Austin Aug 21 at 15:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.