Leroy was shocked when the source code appeared. It was familiar yet strange, like an old lover's kiss. The code was five years old – an artifact of Leroy's first project. Leroy slowly scrolled through the code and pondered his next move. It wasn't a bug that was bothering Leroy – there were no race conditions or tricky numerical conversions. No performance problems or uncaught error conditions. It was all about design …
public class BankAccount
{
public void Deposit(decimal amount)
{
_balance += amount;
LogTransaction("Deposited {0} on {1}", amount, DateTime.Now);
}
public void Withdraw(decimal amount)
{
_balance -= amount;
LogTransaction("Withdrew {0} on {1}", amount, DateTime.Now);
}
public void AccumulateInterest(decimal baseRate)
{
decimal interest;
if (_balance < 10000)
{
interest = _balance * baseRate;
}
else
{
interest = _balance * (baseRate + 0.01);
}
LogTransaction("Accumulated {0} interest on {1}", interest, DateTime.Now);
}
void LogTransaction(string message, params object[] parameters)
{
using(FileStream fs = File.Open("auditlog.txt", FileMode.OpenOrCreate))
using(StreamWriter writer = new StreamWriter(fs))
{
writer.WriteLine(message, parameters);
}
}
public decimal Balance
{
get { return _balance; }
set { _balance = value; }
}
decimal _balance;
}
"Times have changed, and so I have, fortunately", Leroy thought to himself. "And so will this code…"
To be continued…
Comments
Being an important finance related class, I'd like to see both pre- and post-logging.
Also, the 10000 in the if statement in AccumulateInterest and the 0.01 addition to baseRate in the else block are magical. This needs to be refactored out into a suitable arrangement depending on the behaviour they are there to implement.
Regards,
2) Balance property should be readlonly with init in the constructor.
3)There should be a single event labeled 'BalanceChanged' which can allow for logging or anything else.
Furthermore, Balance exposes a public setter. Seems like a gross encapsulation violation since anyone could just set the balance to anything without logging a transaction.
Finally, the code isn't thread-safe. I wouldn't assume that reading or writing a decimal is necessarily, if ever, an atomic operation.
1) You could change the Balance by setting a property (no need to deposit/withdraw)
2) Validations on baseRate argument. User could pass in a negative value or -0.01 which will keep reducing the balance
Interest is getting calculated but not added to the balance :)
The replies above also make a lot of sense.
Also, I assume the transactional locking is done external of this class. Otherwise the balance will be wrong on concurrent updates.
1. Not having error management in case the logging fails
2. Allowing direct access to setting "Balance"
3. Nothing checks that the amount to Withdraw is less or equals that balance
Log might not be valid.
@Jason, @Lynn - both very valid points. I like the event idea. Facrtoring out the magic number is good, too.
@Wilhelm, @Zeb, @Rik, @Kalpesh, @Ilia - throwing away the calulated interest. Yes - that would be like an actual bug :) Perhaps Leroy should be writing some tests?
For everyone who mentioned concurrency problems, let's not worry about threading. We can documument the class as not being thread safe.
Plus I would never indent like that...
;)
http://www.ridgenet.net/~do_while/toaster.htm