Me (to peer): I counted all of the items in the list, and if the list count is zero, we branch to this logic.

Peer: I don't like that, and reject your PR. You should get the list of items, and check to see if the list is empty.

- How is that fucking different than what I did?

- This is not a performance issue, we're talking less than 20 milliseconds on a command run three times a day by developers, not customers.

Peer is correct though, if the exact count is irrelevant, don't count the items, its not only about performance but also code clarity and even if it is irrelevant in this specific case code reviews are as much about teaching/learning as they are about code quality.

@ItsNotMyFault I agree, and even if performance is irrelevant in this case, someday someone might cut and paste or check your solution in a context where performance is important or where fetching the whole list is a very heavy operation.

I have seen it done. :/

Always strive for clear code, if you need to know if the list is empty, use the most clear way to check just that.

Is some cases the compiler might end up doing very different things :) that affect not only performance but stability.

WTF, both cases get the list. Performance would be the same. Both cases count the list. Empty/not empty is based upon count. If these operations are not the same then show this in code. Because saying they are different is really confusing.

🍦 //just a personal pin..

@Demolishun I really depends on the language and type of collection.
For example in C# list.Any() can take a look whether there is an element at all, while list.Count()!=0 will need to get the count, which is a potentially more expensive operation. Also the first one is more clear about its intent - which is more important than a minor performance difference in most cases.

@Demolishun not necessarily, in c# in a list Count is as fast, but if the collection is an enumerable, any might just fetch exists towards an underlying database while count might first fetch all rows, convert them into the objects mapped by the orm and then count them, that could be thousands times more work.

Thats just one example why its a good practice to always use the most suitable methods.

@Demolishun not all lists/collections store the count This is a big issue in languages like C# where you frequently apply filters to lists without creating a new list:

i.e someCollection.Where(e => e.x > 5).Any() iterates over the collection and returns true as soon as a match is found (or false if the end is reached) while someCollection.Where(e => e.x > 5).Count() != 0 will always iterate over the whole list and increment a counter for each match returning the count at the end and then finally doing a comparison with 0 to get a boolean result.

Structures such as linked lists are similar in that they don't necessarily store the count, checking for empty/nonempty can always be done with a comparison on the start pointer while a count might be an iteration.

For what it's worth, this is a bash script I was talking about.

It's the same thing with what you said and your colleagues opinion, but I would just check if there's a first item in the list just to save time be it for performance or not

@ItsNotMyFault You're already neglecting a much more important performance issue with the exact same implication there: You're checking all elements which is completely unnecessary.
someCollection.Any(e => e.x > 5)
would be the correct way.

@saucyatom that is effectively the same thing since .where, .select, etc return generators, you need to call a method like .any. .count, .tolist etc to actually iterate. (.any(x) is less verbose but the overload for .any that accepts a function calls .where(x).any for you, same with first, count, etc). I used .where to get the point across.

@ItsNotMyFault ah right, I forgot the where also returns an iterator (and thus is executed lazily).