News for hackers. Stuff that matters.

A comment on Pull request review mistakes.

Re:Pull request review mistakes by blauditore on Saturday January 28, 2017 @08:24AM (#13506136)
About 4. Style over substance:

Coding style (apart from formatting) can be crucial for understandability, so it's important to have a critical look at it when reviewing. For instance, consider the following snippets:

  void myMethod(FooBar arg) {
    if (arg != null) {
      arg.foo(something);
      for (int i = 0; i < arg.bars().size(); i++) {
        arg.bars().get(i).baz();
      }
    }
  }
vs

  void myMethod(FooBar arg) {
    if (arg == null)
      return;

    arg.foo(something);
    arg.bars().forEach(baz);
  }
Even though they're functionally identical, they look wildly different. These things can hardly be judged by an automatic process like linting, so it's important to manually do that.
[ Parent | on HN ]
Re:Pull request review mistakes by rocky1138 on Saturday January 28, 2017 @10:57AM (#13506870)
For me, a lack of adherence to our conventions is a smell that not all i's have been dotted and t's have been crossed. If the style is poor, I start to think the logic may be suspect, as well.

This is doubly true when delinting and styles are automatic via git hooks.

[ Parent | on HN ]
Re:Pull request review mistakes by glebm on Saturday January 28, 2017 @08:29AM (#13506157)
> These things can hardly be judged by an automatic process like linting

Both of these (if vs guard clause, loop vs each) are enforced by the Rubocop linter in Ruby.

[ Parent | on HN ]
Re:Pull request review mistakes by throwawayish on Saturday January 28, 2017 @08:38AM (#13506201)
Today in: Broadly missing a point.

It's good and nice that there is one linter for a particular language that catches 100 % of these issues in an example, but that doesn't do much to change the fact that most linters for most languages are unable to do this, and that there are quite some instances of these decisions where it is doubtful that any linter would be able to lint it.

[ Parent | on HN ]
Re:Pull request review mistakes by glebm on Saturday January 28, 2017 @08:48AM (#13506235)
> most linters for most languages are unable to do this,

In the long run improving the linter is better for everyone. The examples given by the OP as infeasible are feasible.

Focus on the design over trivial style issues. Your style guide should be 95% covered by the linter. If it isn't, fix your linter.

> Broadly missing a point

My points is: Underestimating static analysis will lead you to the wrong conclusions.

[ Parent | on HN ]
Re:Pull request review mistakes by glebm on Saturday January 28, 2017 @08:52AM (#13506254)
> some instances of these decisions where it is doubtful that any linter would be able to lint it.

Such issues are rarely style issues. They are more in the design territory where the focus of code reviews should be.

[ Parent | on HN ]
Re:Pull request review mistakes by scottnonnenberg on Saturday January 28, 2017 @04:01PM (#13509278)
Oh, I certainly agree that coding standards are important. Consistency of coding style makes a codebase substantially more approachable. I'm just interested in making that as automated as possible: https://blog.scottnonnenberg.com/eslint-part-3-analysis/
[ Parent | on HN ]