Skip to main content
Commonmark migration
Source Link

There are already great answers that focus on code style and trip-ups, so I'd like to approach from another angle: as a potential user/debugger of your code.

#Add/Subtract are not inverse operations#

Add/Subtract are not inverse operations

While clamping the values is attractive for the security it seems to give us, it also endangers the Principle of Least Surprise. If I have 500p and I add 800p, I end up with 999p (and 99g99s99c). If I then subtract 800p again, I do not have my original 500p; rather, I'd have less than 200p left.

If you're concerned about how to display large amounts of money in a limited space, the component in question can figure it out. Perhaps it could progressively leave out copper / silver / gold coins if it needs more space. If I have that much money, I wouldn't be worried about how many copper coins I have. ;)

#No feedback on failure#

No feedback on failure

(I am not a C# programmer so I may have this wrong.)

It appears that I can subtract 50 coins from 20 coins and end up with 0 coins. This surprises me. If a function cannot do what it claims to do (or seems to claim to do—tricky), it should signal the fault in some way, like by throwing an exception.

#HasXYZ vs GetXYZ#

HasXYZ vs GetXYZ

In the case where you have 200cp or 2sp, which is the same here, HasCopper() will return true because it also checks HasSilver(), but Copper() will return 0. It feels to me that HasCopper() should imply Copper() > 0, so either HasCopper() should return false or Copper() should give the total worth in copper coins (200).

#Simplify or Go Big#

Simplify or Go Big

My base advice would be to either:

  • make things simpler by doing away with the coin types and leaving that to the representation;
  • or to go all the way and count all different coin types separately, i.e. having a counter for each type, and then adding a Wealth() or Value() function that gives you your total worth from adding all the coin values. Adding will be easier, but subtracting will be harder.

After all, what is the point of going through the trouble of having 4 different coin types if this difference is not meaningful? (Think about how you will use this class: would you set up prices for items in a shop in platinum, gold, silver, copper coins? Or would you set a base 'worth'?)

There are already great answers that focus on code style and trip-ups, so I'd like to approach from another angle: as a potential user/debugger of your code.

#Add/Subtract are not inverse operations#

While clamping the values is attractive for the security it seems to give us, it also endangers the Principle of Least Surprise. If I have 500p and I add 800p, I end up with 999p (and 99g99s99c). If I then subtract 800p again, I do not have my original 500p; rather, I'd have less than 200p left.

If you're concerned about how to display large amounts of money in a limited space, the component in question can figure it out. Perhaps it could progressively leave out copper / silver / gold coins if it needs more space. If I have that much money, I wouldn't be worried about how many copper coins I have. ;)

#No feedback on failure#

(I am not a C# programmer so I may have this wrong.)

It appears that I can subtract 50 coins from 20 coins and end up with 0 coins. This surprises me. If a function cannot do what it claims to do (or seems to claim to do—tricky), it should signal the fault in some way, like by throwing an exception.

#HasXYZ vs GetXYZ#

In the case where you have 200cp or 2sp, which is the same here, HasCopper() will return true because it also checks HasSilver(), but Copper() will return 0. It feels to me that HasCopper() should imply Copper() > 0, so either HasCopper() should return false or Copper() should give the total worth in copper coins (200).

#Simplify or Go Big#

My base advice would be to either:

  • make things simpler by doing away with the coin types and leaving that to the representation;
  • or to go all the way and count all different coin types separately, i.e. having a counter for each type, and then adding a Wealth() or Value() function that gives you your total worth from adding all the coin values. Adding will be easier, but subtracting will be harder.

After all, what is the point of going through the trouble of having 4 different coin types if this difference is not meaningful? (Think about how you will use this class: would you set up prices for items in a shop in platinum, gold, silver, copper coins? Or would you set a base 'worth'?)

There are already great answers that focus on code style and trip-ups, so I'd like to approach from another angle: as a potential user/debugger of your code.

Add/Subtract are not inverse operations

While clamping the values is attractive for the security it seems to give us, it also endangers the Principle of Least Surprise. If I have 500p and I add 800p, I end up with 999p (and 99g99s99c). If I then subtract 800p again, I do not have my original 500p; rather, I'd have less than 200p left.

If you're concerned about how to display large amounts of money in a limited space, the component in question can figure it out. Perhaps it could progressively leave out copper / silver / gold coins if it needs more space. If I have that much money, I wouldn't be worried about how many copper coins I have. ;)

No feedback on failure

(I am not a C# programmer so I may have this wrong.)

It appears that I can subtract 50 coins from 20 coins and end up with 0 coins. This surprises me. If a function cannot do what it claims to do (or seems to claim to do—tricky), it should signal the fault in some way, like by throwing an exception.

HasXYZ vs GetXYZ

In the case where you have 200cp or 2sp, which is the same here, HasCopper() will return true because it also checks HasSilver(), but Copper() will return 0. It feels to me that HasCopper() should imply Copper() > 0, so either HasCopper() should return false or Copper() should give the total worth in copper coins (200).

Simplify or Go Big

My base advice would be to either:

  • make things simpler by doing away with the coin types and leaving that to the representation;
  • or to go all the way and count all different coin types separately, i.e. having a counter for each type, and then adding a Wealth() or Value() function that gives you your total worth from adding all the coin values. Adding will be easier, but subtracting will be harder.

After all, what is the point of going through the trouble of having 4 different coin types if this difference is not meaningful? (Think about how you will use this class: would you set up prices for items in a shop in platinum, gold, silver, copper coins? Or would you set a base 'worth'?)

Source Link
JvR
  • 2.8k
  • 12
  • 25

There are already great answers that focus on code style and trip-ups, so I'd like to approach from another angle: as a potential user/debugger of your code.

#Add/Subtract are not inverse operations#

While clamping the values is attractive for the security it seems to give us, it also endangers the Principle of Least Surprise. If I have 500p and I add 800p, I end up with 999p (and 99g99s99c). If I then subtract 800p again, I do not have my original 500p; rather, I'd have less than 200p left.

If you're concerned about how to display large amounts of money in a limited space, the component in question can figure it out. Perhaps it could progressively leave out copper / silver / gold coins if it needs more space. If I have that much money, I wouldn't be worried about how many copper coins I have. ;)

#No feedback on failure#

(I am not a C# programmer so I may have this wrong.)

It appears that I can subtract 50 coins from 20 coins and end up with 0 coins. This surprises me. If a function cannot do what it claims to do (or seems to claim to do—tricky), it should signal the fault in some way, like by throwing an exception.

#HasXYZ vs GetXYZ#

In the case where you have 200cp or 2sp, which is the same here, HasCopper() will return true because it also checks HasSilver(), but Copper() will return 0. It feels to me that HasCopper() should imply Copper() > 0, so either HasCopper() should return false or Copper() should give the total worth in copper coins (200).

#Simplify or Go Big#

My base advice would be to either:

  • make things simpler by doing away with the coin types and leaving that to the representation;
  • or to go all the way and count all different coin types separately, i.e. having a counter for each type, and then adding a Wealth() or Value() function that gives you your total worth from adding all the coin values. Adding will be easier, but subtracting will be harder.

After all, what is the point of going through the trouble of having 4 different coin types if this difference is not meaningful? (Think about how you will use this class: would you set up prices for items in a shop in platinum, gold, silver, copper coins? Or would you set a base 'worth'?)