There was a good comparison about those floating around:
For me the code on the right is far more readable and easier to understand.
For me the code on the right is far more readable and easier to understand.
- consts are dropped
- variable definitions are merged onto multiple lines
- usefully named constants like `nPixels`, `nBytes` are elided
- the `idx` lambda is inlined
Local consts are not particularly useful, especially for things like ints. Compilers know it's const, and a reader doesn't need to worry about it in a local scope.
width, height usually come in pairs, so putting them on the same line is usual.
nPixels and nBytes are not used more than once so they are not useful abstractions, and the pattern of `width * height * bytes_per_pixel` is so common that there is no ambiguity about what it does.
The idx lambda is probably a distracting abstraction which requires the reader to think of it in a different context than the immediate what pixel goes to where. Again, the pixel moving pattern in the right is so common it's familiar to most people working in similar areas, and in terms of error-prone both are not better than the other.
Same thing with doing two separate things in the loop. Not only can it stop the the compiler from optimisations from time to time (the c++ compiler is very clever... but many times it gives up), I've had speed increases in breaking it up into several loops, but I also have to search and in my mind break things up. Very easy to do when I'm writing the code (and I'd probably do the same because of laziness), but quite annoying when I read someone else's code that does it.
The argument about optimization is almost certainly premature optimization. Most of the time how you write a loop doesn't matter. You only find out what matters via profiling and refactor accordingly.
If the compiler can enfore an invariant (constness), why cede that functionality on the hopes you can ensure it yourself?
By the same line of argumentation you should do away with the static type system.
In that case, it makes sense.
The changed version is idiomatic in image processing or similar areas that deal with pixels. Being idiomatic makes it familiar to read, and easy to change.
It's under that lens that I am criticizing the changed code.
I can still assign to it. Sure, you can argue that if the code is complex enough that you might accidentally do that without it being obvious, then the code should be simplified or split up, but that's besides the point. If its const, the compiler will complain, if its not, it will silently let me. Nobody can write ideally-factored code all of the time and following good consistent practices helps. Also, I hear arguments like that all the time, but so much of actual real-world shipped code breaks these "rules", so I'd rather be pragmatic and choose a style that improves otherwise imperfect code.
Its also as much a hint to future me that the variable is intended to not be modified.