My gut feeling is this wouldn't pass the unofficial code reviews at my work - too many functions making the code look messy, instead of cleaner.
Does anyone have any counterpoints?
My gut feeling is this wouldn't pass the unofficial code reviews at my work - too many functions making the code look messy, instead of cleaner.
Does anyone have any counterpoints?
> 1. It is pretty long, sixteen lines, multiple parts separated by empty lines.
I wouldn't call 16 lines "long".
It contains empty lines for easier reading. Well done, not actually a problem.
But it lacks any comments on why the assertions have to be made.
> 2. It does several things. It gets an item by name, validates parameters, and it implements the logic of vending an item.
These are the things that naturally belong together. Separating validation from logic is not a very good idea. The logic requires the specific assertions in the validation part. They belong together.
> 3. It has several abstraction levels. The high-level vending process is hidden among the finer-level details like Boolean checks, usage of specific constants, math operations, etc.
In the single function implementation it's actually not hidden because it's all in one function. Use some comments to structure your function.
---
In my opinion separating this function into multiple functions would only make sense if these parts are used multiple times AND if they are commented well on why they need to act this way.
That's more or less the problem when discussing how to work with complex, real world software and giving examples of how to do it: the examples end up being simple enough that the techniques aren't worthwhile in that case. Don't mistake that for the techniques not being worthwhile ever.
I have seen the same when code is presented showing examples of how to use an IoC Container. The response "but this overcomplicates the code" is true, but misses that it doesn't apply to the (far larger) codebase that IoC containers are aimed at.
At first I wouldn't expect a function "validatedItemNamed" to check the price. While it checks the price it doesn't check the inventory number.