Skip to main content

You are not logged in. Your edit will be placed in a queue until it is peer reviewed.

We welcome edits that make the post easier to understand and more valuable for readers. Because community members review edits, please try to make the post substantially better than how you found it, for example, by fixing grammar or adding additional resources and hyperlinks.

Required fields*

7
  • The biggest mistake I can see in this method is the overuse (IMHO wrong use) of var. I had to read it three times to see all that variables are integers, and the code makes an integer division. Do yourself a favor and write int instead of var, that is not even more typing. Commented Jan 27, 2018 at 12:19
  • Btw, this might be clearer if you write target = target % denomination, i.e. the remainder after giving that change. It then becomes clear that every line is integral to the solution, and nothing is worth extracting. (Except maybe fetching and ordering the denominations? Dunno. This method should arguably be called GiveChange(), and be a member of a currency class.) Commented Jan 27, 2018 at 14:04
  • @amon, I see what you mean about putting the method in the Currency class. However, I am planning to encapsulate Cost in a method (value object), so I think it makes sense to use a Domain Service, which operates on: Cost (value object - which may have methods for calculating discounts etc) and Currency. Do you agree? Commented Jan 27, 2018 at 14:31
  • the biggest mistake is the potential infinite loop Commented Jan 27, 2018 at 16:24
  • @Ewan, would you elaborate? Commented Jan 27, 2018 at 16:28