Comment by segmondy
5 years ago
actually, "don't optimize prematurely" is a poor advice. just recently I was doing a good review that had the same issue where they were counting the size of an array in a loop, when stuff was being added to the array in the loop too. obvious solution was to track the length and
arr = []
while ...:
if something:
arr.append(foo)
...
if count(arr) == x:
stuff
...
changed to
arr=[]
arr_size = 0
while ...:
if something:
arr.append(foo)
arr_size++;
...
if arr_size == x:
stuff
...
This is clearly optimization, but it's not premature. The original might just pass code review, but when it wrecks havoc, the amount of time it will cost will not be worth it, jira tickets, figuring out why the damn thing is slower, then having to recreate it in dev, fixing it, reopening another pull request, review, deploy, etc. Sometimes "optimizing prematurely" is the right thing to do if it doesn't cost much time to do or overly completely the initial solution. Of course, this depends on the language, some languages will track the length of the array so checking the size is o(1), but not all languages do, so checking the length can be expensive, knowing the implementation detail matters.
I'm not sure I would prefer the second version in a code review. I find the first version is conceptually nicer because it's easy to see that you will always get the correct count. In the second version you have to enforce that invariant yourself and future code changes could break it. If this is premature optimization or not depends on the size of the array, number of loop iterations and how often that procedure is called. If that's an optimization you decide to do, I think it would be nice to extract this into an "ArrayWithLength" data structure that encapsulates the invariant.
> In the second version you have to enforce that invariant yourself and future code changes could break it.
Yes, that's a real issue. But we've been given two options:
- Does the correct thing, and will continue to do the correct thing regardless of future changes to the code. Will break if the use case changes, even if the code never does.
- Does the correct thing, but will probably break if changes are made to the code. Will work on any input.
It actually seems a lot more likely to me that the input given to the code might change than that the code itself might change. (That's particularly the case for the original post, where the code serves to read a configuration file, but it's true in general.)
Yes, I absolutely see the reasoning and I think if one does go the route of encapsulating the more efficient array logic one can have the best of both options.
2 replies →
With these things, I have always had the hope that an optimizing compiler would catch this. I think it is an allowed optimization if the count function is considered `const` in C or C++ at least.