On the other hand, if it's less important code or the renaming is not clearly an improvement it can be quite useless. But I've met some developers who has the opinion of reviews as pointless and just say "this works, just approve it already" which can be very frustrating when it's a codebase with a lot of collaboration.
1. you are violating a previously agreed upon standard for naming things
2. inconsistent naming, eg some places you use "catalog ID" and other places you use "item ID" (using separate words and spaces here because case is irrelevant).
3. the name you chose makes it easy to conflate two or more concepts in your system
4. the name you chose calls into question whether you correctly understood the problem domain you are addressing
I'm sure there are other good naming comments, but this is a reasonable representation of the kinds of things a good comment will address.
However, most naming comments are just bike shedding.
The bigger problem is people who feel ownership of shared codebases tied to their ego and who get angry when people suggest changes to names and other bits of interfaces instead of just making the suggested change.
If you get code review feedback, the default answer is "Done" unless you have a strong reason not to. If it's not obvious whether the name suggested by the author or the reader is better, the reader's choice should be taken every time.
I used to think that way, but in many nontrivial circumstances, every conceivable name will be a mismatch for where some person is coming from, and not be self-evident for their mental model. Even the same person, over a longer time span. There is often a gap to bridge from name to meaning, and a comment isn’t the worst way to bridge it.
In short I use it as a form of rubber ducking. No it’s not like this word, it’s more like that one, but most of all like this one.
Given enough time and rope, these parts of the code start to encroach on each other and the cracks start to show. There are definitely bugs the smart guy introduces because no, in fact, you can’t keep them straight in your head either.
So it does matter if you use, as a top of my head example, the word “account” for both the user and group management features of the app and to describe an entry to an incident report in another part. It will bite you in the ass, and it’s easier to change now when there are three references instead of 23.
yes but it can be severely diminishing returns. Like lets step back a second and ask ourselves if:
var itemCount = items.Count;
vs
var numberOfItems = items.Count;
is ever worth spending the time discussing, versus how much of a soft improvement it makes to the code base. I've literally been in a meeting room with three other senior engineers killing 30 minutes discussing this and I just think that's a complete waste of time. They're not wrong, the latter is clearer, but if you have a PR that improves the repo and you're holding it back because of something like this, then I don't think you have your priorities straight.
Over time I’ve developed some tricks that invite people to add features to the code in the “right” place, and this is one of them. Once in a while someone gets credit for work I already thought to do but didn’t have time. But for every one of those there’s a half dozen or a dozen cases of increasing the bus number on a block of code I wrote be nerd sniping people into making additions while I’m busy with something else.
Sort of place that is fussy about test naming so where I would do smth like:
TestSearchCriteriaWhere
they'd want
Test_That_Where_Clauses_In_Search_Criteria_Work
I think its a waste of typing but idk, I'm willing to let it slide because I think its a pointless hill to die on.
var itemCount = items.Count;
fussy about test naming
As such both of those test names are really bad, because they can't tell anything at all about whether you're testing for the correct thing. How do I know that your assertions are actually asserting that it "works"?
Instead, I'd want a test named something like this (assuming that that's what this particular test is actually about - i.e. imagine this particular test in the context of a user defined search, where one of the options is that they can specify a project to search by and this particular test is about verifying that we check the permissions the user has for said project. There would be different tests for each of the relevant where clauses that specifying a project in the search params would entail and different tests again for each of the other user specifiable parameters that result in one or more where clauses to be generated):
shouldCheckProjectPermissionsWhenProjectIdInSearchParams()
I appreciate neither of those test names are great but it was just a strawman example to show the fussiness.
So clearly distinguishing the local `numberOfItems` from `items.Count` _could_ be helpful. But I wouldn't ping it in a review.
(Because the correct English term is “item count”, not “items count”.)
Personally, I tend to only name it “count” if it’s a variable that is used to keep a count, i.e. it is continually incremented as new items are processed.
Otherwise I tend to prefer `numItems`.
Yes, this is very close to bike-shedding. There is, however, an argument to be made for consistency in a code base.
var relevantCount = items.Where(x => x.SomeValue > 5);
vs
var numberOfRelevantItems = items.Where(x => x.SomeValue > 5);
so it wasn't necessarily cheap enough to want to repeat.
Here though you're missing the ".Count", so
var relevantItems = items.Where(x => x.SomeValue > 5);
relevantItems.Count