3 ms·

>What previously was a thirty-line function with several indentation levels and intermediate variables that had to be kept in the head is now ten functions, eac

>What previously was a thirty-line function with several indentation levels and intermediate variables that had to be kept in the head is now ten functions, each having a self-documenting specific name. 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?

KarlFreeman 11y ago
I feel like somewhere between the two would pass. Keep the validation as distinct functions but inline validatedItemNamed, reduceDepositedCoinsBy.
- static_noise 11y ago
  My problem with putting the validation in an own function is that this should only be done if the validation itself is generic like "check checksum" but not if it relates to the actual logic like "A must be smaller than B". 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.
deanclatworthy 11y ago
I feel the same. When I'm new to a code base its a pain to keep jumping around functions to see what they actually do. I agree functions should be short but this is a step too far for me.
static_noise 11y ago
To me the general idea of splitting a complex problem into a series of less complex problems is good. However the example used seems trivial and the solution is worse than the problem: > 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.
- SideburnsOfDoom 11y ago
  > However the example used seems trivial and the solution is worse than the problem: 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.

KarlFreeman 11y ago

I feel like somewhere between the two would pass. Keep the validation as distinct functions but inline validatedItemNamed, reduceDepositedCoinsBy.

static_noise 11y ago
My problem with putting the validation in an own function is that this should only be done if the validation itself is generic like "check checksum" but not if it relates to the actual logic like "A must be smaller than B". 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.

deanclatworthy 11y ago

I feel the same. When I'm new to a code base its a pain to keep jumping around functions to see what they actually do. I agree functions should be short but this is a step too far for me.

static_noise 11y ago

To me the general idea of splitting a complex problem into a series of less complex problems is good. However the example used seems trivial and the solution is worse than the problem: > 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.

SideburnsOfDoom 11y ago
> However the example used seems trivial and the solution is worse than the problem: 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.