4 ms·

The types of nits I raise are the ones where I think the author of the pull request would appreciate as something they should do going forward. For example:

The types of nits I raise are the ones where I think the author of the pull request would appreciate as something they should do going forward. For example: if foo > bar: return foo else: return bar Should just be: return max(foo, bar) That's a nit because what they wrote was fine, but it's longer than it should be.

codeduck 5y ago
The first returns bar if foo == bar, the second is dependent on the implementation of max(). There's a potential difference in behaviour.
- rileymat2 5y ago
  I saw that too, but if there is a "real" difference in returning equal things, then it could indicate a way bigger problem in the design.
  - withinboredom 5y ago
    If you're using a language where there is a difference between value-equals and instance-equals, it could cause a very subtle bug by changing this code if the result of it was relying on an instance-equals later on.
    - rileymat2 5y ago
      I agree it can introduce bugs, my point is relying on that is probably a design issue somewhere else. Because even in the first, you are relying on the order of max being the same everywhere it matters.
      - withinboredom 5y ago
        Maybe. If it’s usage were in the scope of the current PR, I might suggest a fix like this. But if it might introduce something subtle, I probably wouldn’t suggest it or I’d ask if they had considered changing it as well as if there were any subtle issues to worry about. As someone who works in a multimillion line codebase that sees millions of executions daily since 2005, these are real issues and saying “it’s a design issue somewhere else” doesn’t exactly fly because that “somewhere else” may no longer have a team maintaining it.
onion2k 5y ago
What you suggest is actually more readable than the if block, especially if the variable names are meaningful, which is why it's better. It's more obvious what's expected to return. There's no need to claim shortness as a virtue of good code; generally speaking it's not better to write less code. Unless you work for an organisation that code golfs everything there should be no expectation that code should be as short as possible.
- 3pt14159 5y ago
  Everything else being equal, I prefer shorter code. I agree that this example is both shorter and more readable, but it doesn't really matter in the grand scheme of things.
  - onion2k 5y ago
    Everything else being equal, I prefer shorter code. I appreciate that some developers do, but things are never equal. Your own preference should largely be irrelevant if you work on a team. Code is how developers communicate with each another, so readability and clarity for someone else who's reading the code should always be the driving force behind your coding style. Often shorter code is actually easier to read, so it kind of works out, but it should never be the goal. I actively push back on code where clarity has been sacrificed for brevity.

codeduck 5y ago

The first returns bar if foo == bar, the second is dependent on the implementation of max(). There's a potential difference in behaviour.

rileymat2 5y ago
I saw that too, but if there is a "real" difference in returning equal things, then it could indicate a way bigger problem in the design.
- withinboredom 5y ago
  If you're using a language where there is a difference between value-equals and instance-equals, it could cause a very subtle bug by changing this code if the result of it was relying on an instance-equals later on.
  - rileymat2 5y ago
    I agree it can introduce bugs, my point is relying on that is probably a design issue somewhere else. Because even in the first, you are relying on the order of max being the same everywhere it matters.
    - withinboredom 5y ago
      Maybe. If it’s usage were in the scope of the current PR, I might suggest a fix like this. But if it might introduce something subtle, I probably wouldn’t suggest it or I’d ask if they had considered changing it as well as if there were any subtle issues to worry about. As someone who works in a multimillion line codebase that sees millions of executions daily since 2005, these are real issues and saying “it’s a design issue somewhere else” doesn’t exactly fly because that “somewhere else” may no longer have a team maintaining it.

onion2k 5y ago

What you suggest is actually more readable than the if block, especially if the variable names are meaningful, which is why it's better. It's more obvious what's expected to return. There's no need to claim shortness as a virtue of good code; generally speaking it's not better to write less code. Unless you work for an organisation that code golfs everything there should be no expectation that code should be as short as possible.

3pt14159 5y ago
Everything else being equal, I prefer shorter code. I agree that this example is both shorter and more readable, but it doesn't really matter in the grand scheme of things.
- onion2k 5y ago
  Everything else being equal, I prefer shorter code. I appreciate that some developers do, but things are never equal. Your own preference should largely be irrelevant if you work on a team. Code is how developers communicate with each another, so readability and clarity for someone else who's reading the code should always be the driving force behind your coding style. Often shorter code is actually easier to read, so it kind of works out, but it should never be the goal. I actively push back on code where clarity has been sacrificed for brevity.