PR Nitpick or not? When is “functionally correct, but messy” worth asking for revisions on?

I joined a new team at work recently and I'm still getting a feel for how they do things. On my previous team there were only three of us, and we had aligned on many coding practices such that we had no issues pointing out minor mistakes in each others code (accidental use of older/superseded language features, inconsistent styling, etc.) in addition to the more meaningful/functional feedback you'd expect of a PR review.

I'm aware that this would generally be considered unwanted nitpicking, so I've tried to avoid doing so in the new larger team, however this situation caught me a bit off guard because I didn't think it was purely stylistic feedback. This is an abbreviated pseudocode equivalent of the JS I reviewed:

function makeDropdown() { let dropdown = new Dropdown(); //... add items to the dropdown let that = this; dropdown.onChange(function(event) { let currentSelection = event.selection; let filterById = that.getFilterById(); if (filterById) { that.filterByDropdownSelectedId(currentSelection); } else { that.filterByDropdownSelectedLabel(currentSelection); } }); //... the rest of the function return dropdown; } function filterByDropdownSelectedLabel(selection) { let value = selection.getText(); this.filter(value); } function filterByDropdownSelectedId(selection) { let id = selection.getId(); this.filter(id); } 

I found this to be extremely verbose and awkward to follow for something that should be very straightforward. For a very small event handler, it seemed odd to have the logic split across three locations in the code.

I suggested that to make the code more readable for people maintaining it in the future, the two near-identical functions that only really have one functional line of code each could be moved into the event handler, and that we could avoid unnecessarily aliasing "this" as "that" by using an arrow or bound function.

My suggested change looked like this:

function makeDropdown() { const dropdown = new Dropdown(); //... add items to the dropdown dropdown.onChange(onDropdownChange.bind(this)); //... the rest of the function return dropdown; } function onDropdownChange(event) { const currentSelection = event.selection; const filterById = this.getFilterById(); if (filterById) { this.filter(currentSelection.getId()); } else { this.filter(currentSelection.getText()); } } 

My colleague's response was that the original code works and I shouldn't waste his time with stylistic changes when the code functions correctly and refactoring would require changing his unit tests. He seemed frustrated that the code review process is making him "unproductive" and that we're focusing "too much on the programming language rather than the functionality".

Was this a nitpick? When do you consider non-functional revisions "worth it" during code review?