[Quarrelsome]

> Naming comments can be very useful in code that gets read by a lot of people. It can make the process of understanding the code much quicker.

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.

[hinkley]

Generally you’d like the variable to imply a call to action. Even if the call to action is for a feature still in the backlog.

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.

[spooky_action]

Sorry for the dumb question, is the second version actually better than the first? Because I prefer the first. But perhaps you chose this as a particularly annoying/unuseful comment

[Quarrelsome]

I personally don't give a shit either way but I've worked in dev shops with a clear preference for the second one. I can see their point because the code as natural language parses better but I don't think its strong enough to care about.

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.

[tharkun__]

Let's take it up a notch!

    var itemCount = items.Count;

depends on what `items` is, no? Is the `.Count` O(1)? Do you really need a variable or is it fine for the (JIT) compiler to take care of it? Is it O(n) and n is significant enough? Maybe you need a variable and spend time arguing about that name. Yes I chose this because almost everyone I know at least would argue you always have to create the variable (and then argue about the name) ;)

    fussy about test naming

I get fussiness about test naming. I believe that a good test "name" should tell you enough for you to be able to "double check" the test setup as well as the assertions against the test name with some sort of "reasonable" knowledge of the code/problem domain.

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()

Every single test case gives you the ability to specify both a good name and clear, concise test assertions. If I see anything but a bunch of assertions related to project permissions for the logged in user in this test, I will fight you tooth and nail on that test ;) I couldn't care less tho if you use camelCase or snake_case or whatever. I just had to choose something to post. I also couldn't care less if you had 17 different assertions in the test (we all know that "rule", right? I think the "test one thing" and "one assertion" is not about the actual number of "assert statements". People that think that, got the rule wrong. It's all about the "thing" the assertions test. If you have 17 assertions that are all relevant to testing the project permission in question then they're great and required to be there. If 1 is for asserting the project permissions and the other 16 are repeating all the other "generic assertions" we copy and pasted from previous tests, then they're not supposed to be there. I will reject such a PR every time.

[Quarrelsome]

It matters if there's a lot of churn or the test fails a lot but if its a test that I write and for whatever reason it never fails I think we've just wasted our time on being fussy.

I appreciate neither of those test names are great but it was just a strawman example to show the fussiness.

[solid_fuel]

If I was going to nitpick it I would point out that `itemsCount` could easily be confused with `items.Count`, or vice versa, depending on syntax highlighting. That kind of bug can have a negative impact if one or the other is mutated while the function is running.

So clearly distinguishing the local `numberOfItems` from `items.Count` _could_ be helpful. But I wouldn't ping it in a review.

[layer8]

That’s why it’s `itemCount` and not `itemsCount`. ;)

(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.

[ambicapter]

They’re both equally bad to me, I don’t see the improvement over just using item.count. I may be nitpicking a toy example though.

[Quarrelsome]

I think in this case itemCount had application in a couple of conditions later in the function, so there was value in extracting the count. In my recollection I might be missing some nuance, lets say for the sake of argument it was:

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.

[Izkata]

Almost. I think you're reflexively doing the same thing GP was questioning (which I agree with; in the original example the new variable was just straight duplication of knowledge and is as likely to be the source of bugs as anything else (like if items were added or removed, it's now out of date)).

Here though you're missing the ".Count", so

  var relevantItems = items.Where(x => x.SomeValue > 5);
  relevantItems.Count

As long as it's not a property that's calculated every time it's accessed, this still seems better than pulling the count into its own variable to me.