Skip to main content
5 of 6
corrected concordancy

Divide into smaller functions is the best way for sure, at least for human readability.

  • By extracting a smaller function, you are naming a block of code. That really improves readability.
  • Readers can have a macro idea of what that function does, without having to bother with details of each step. If you want to see the details, than you look ate the sub-function. This is the Newspaper Metaphor in Uncle Bob's Clean Code.

Generally speaking, complex if statements should be extracted to their own methods. Those nasty ifs should be substituted by a meaningful name that explains what is being tested so readers don't have to parse those huge lines of code.

But besides that, there is more refactoring to be done there. If I were reading the purchase ou isCreditOk methods, I wouldn't want to see that for loop calculating the total value. That's not an easy-read. I'd have to stop my eyes at that point, mentally parse that block to try to figure out what it's doing. The for loop should be extracted to another method called getOrderPrice, getTotalPrice or something like that.

Better than that, the items set could be extract to it's own class: OrderItems. The class would have a method like getTotal that would return the sum of its own items. That's the Single Responsibility Principle, the OrderItems class is the one that handles its own items. In addition to that, the data structure (Set in this case) would be encapsulated and you are free to change to other data structures whenever you want (if you need to change it to a list, for example).