1

Is it a valid code review criticism to have someone manually arrange the order of keys in a dictionary or map they created for a feature, by numerical or alphabetical order when it doesn't improve functionality?

I can understand the argument of it could potentially improve readability but is this a valid criticism/review comment if its not part of the standards/guidelines?

Is there an inherent functional benefit to the below two styles of code other than readability?

var dictionary=map[string]string{
"a":"alpha",
"b":"beta",
"c": "gamma",
}

var sortedDictionary=map[string]string{
"b":"beta",
"a":"alpha",
"c": "gamma",
}
Improve this question
1
  • 3
    Does changing the order of the keys change the behavior of the program? If it does, the first style is the most correct, because it states the order explicitly. If it doesn't, the first style is still the most correct because the dictionary doesn't need to be sorted. The only way the second style is correct is if the behavior of the program depends on the alphabetical ordering of the keys.
    – Robert Harvey
    Commented Mar 25, 2022 at 16:30

4 Answers 4

Reset to default
7

Changes that suggest "making code look nice" are mainly a matter of taste (as opposed to "making it look less ugly", but see below). Such issues have a high potential of making code reviews a highly frustrating experience. If you don't find anything else, leave early and save both of you a lot of time. Code reviews should ideally be a pleasant experience for both the reviewer and the reviewed (at least the ones I made that went that way were the most productive ones) mentioning positive things as well as criticising and discussing objectively negative aspects and proposals for improval. Style issues can easily come across as nitpicking.

Style is to many a matter of taste and if you can't come up with a tangible functional argument why it should look different, you're concentrating on the wrong things. (Maybe you should consider code reviews a chance to make yourself more familiar with other people's coding style - You're going to face and will have to live with a lot of them during your career).

When I'm saying "making it less ugly" above, I mean cases where "style" (or lack of it) negatively affects readability and thus maintainability. I've seen code with illogical line breaks, ragged comments and messy indentation. That should be pointed out and fixed (in my experience, the visual state of code and its programmer's state of mind do somewhat correlate: if one of them is messy, the other one is likely to be confused as well... Your example doesn't seem to be of that sort.

If that example dictionary wouldn't have only three, but 300 entries, I would definitely go with you that it should be alphabetically sorted. But that's then a functional argument in that it may help to avoid duplicate entries, which doesn't really apply to three.

BTW: Should you happen to have a coding convention in place at your team that says "dictionaries must be sorted", it's obviously a different story. But then I might probably question that coding convention...)

Improve this answer
6
  • Good answer. In your „making less ugly“ section you could mention readability, too, because maintenance, for me, has the notion of modifying. But code is more often read than modified.
    – Hartmut Braun
    Commented Mar 25, 2022 at 18:25
  • 2
    @HartmutBraun I was always under the impression that "reading" is a big part of "maintaining" - ;) But, anyways ;)
    – tofro
    Commented Mar 25, 2022 at 18:50
  • Yes, in a way. Reading and understanding is the essential part before you can modify the code.
    – Hartmut Braun
    Commented Mar 25, 2022 at 18:58
  • Reading is also the essential part before reviewing the code as well. Style changes are perfectly valid requests in a code review as long as the style changes resolve obvious problems impacting how well the code is understood. Ordering dictionary keys seems like a personal preference. This makes it a bad candidate for change -- unless reordering the keys makes it easier to understand. And as long as reordering the keys does not change the behavior of the program. Dictionaries and similar data structures are implemented differently in each technology. Order might (or might not) matter.
    – Greg Burghardt
    Commented Mar 26, 2022 at 15:01
  • For example, in .NET, a dictionary is unordered. I remember years ago in Ruby, HashMaps were unordered at first. And then I seem to remember an update to the languag that made HashMap keys ordered (first in, first out when looping over them). Beware of situations like this.
    – Greg Burghardt
    Commented Mar 26, 2022 at 15:03
5

If you're spending time in your code review process arguing about the order of things in dictionaries, you are doing it wrong.

Create a standard, stick to it, enforce it in your tooling and spend your team's valuable code review brain cycles on stuff that actually matters.

Improve this answer
2
  • "enforce it in your tooling" And if you want it sorted, preferably have a tool in your CI/CD pipeline that automatically sorts it for you.
    – Mast
    Commented Mar 25, 2022 at 16:57
  • Ideally, yes. However, not all tools have this capability out-of-the-box. I've been doing a lot of my recent work in Ruby. Rubocop is the de-facto standard linter for Ruby applications. Looking at the docs, I don't see key order as something that currently exists. An organization would need to write their own plugin, go find a third-party plugin (that may be of questionable support), or handle this in a code review. For some organizations, handling this in a code review may make sense, although not from a performance perspective.
    – Thomas Owens ♦
    Commented Mar 25, 2022 at 17:08
3

There are two questions here.

The first question is about the constructiveness of a comment about the order of keys. A code review should focus on things that cannot be detected and/or corrected by automated tools. If your tools cannot detect cases where the order of the keys may not be correct, then this could be a constructive comment. The next step would be for the review to consider the impact on future readers and maintainers of the code. Would reordering the keys make it easier to understand the code? Or reduce the chances of injecting a defect later on? Or improve the performance of the system? If any of these things are true, then I would consider it a constructive comment that should be left in the review.

The second question is about benefits. This likely depends on your language and platform. However, in the big picture, any performance benefits would likely be micro-optimizations. In that case, refer to the earlier part of this answer. Instead of thinking about performance, think about readability, maintainability, and overall quality and if the proposed change would likely improve those or other aspects of the system. If so, propose the change. Otherwise, move on.

Improve this answer
-1

Sorting things in alphabetical order is usually the daftest thing you can do.

Eight, five, four, nine, one, seven, six, ten, three, two.

Arrange things in a logical order.

Improve this answer
1
  • This sounds more like a comment. Also, in OP's example, the alphabetical order is the logical order. Are you saying the reviewer is correct?
    – Llewellyn
    Commented Mar 26, 2022 at 22:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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