What's Wrong With This Code (#19)

Monday, March 31, 2008

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;
"Deposited {0} on {1}", amount, DateTime.Now);

public void Withdraw(decimal amount)
        _balance -= amount;
"Withdrew {0} on {1}", amount, DateTime.Now);

public void AccumulateInterest(decimal baseRate)
decimal interest;

if (_balance < 10000)
            interest = _balance * baseRate;
            interest = _balance * (baseRate + 0.01);
"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…

Jonathan Monday, March 31, 2008
Create a strategy for accumulating interest, rather than the AccumulateInterest() method that could grow over time into multiple if's, whereas's and whatnot's?
Ryan Gray Monday, March 31, 2008
Wild guess - logging the transaction shouldn't be part of this class, as it violates the SRP. I'm thinking Decorator pattern to the rescue, but maybe all the IoC stuff I'm reading lately is scrambling my brain...
Jason Stangroome Monday, March 31, 2008
Two things jump to mind:

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.

lynn Monday, March 31, 2008
1) There needs to be a check for parameter values on all of the methods, contract for positive decimals only.

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.
Wilhelm Svenselius Monday, March 31, 2008
I agree with the above. In addition, the AccumulateInterest method simply throws away its calculated value. Shouldn't it affect the account balance? Also, it ignores the case where the balance is negative, doesn't check that "baserate" is indeed a value in the expected range (it could be >1 or negative for all we know).

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.
Ilia Jerebtsov Monday, March 31, 2008
Unless I'm missing something, the AccumulateInterest function doesn't actually do anything aside from logging the (local) interest value, which means that in order to retrieve it, one would have to parse through the log.
Laurent Monday, March 31, 2008
May I add the suggestion to use the Balance property instead of the private _balance field ?
Kalpesh Monday, March 31, 2008
2 things

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.
Rik Hemsley Monday, March 31, 2008
Where is the interest value actually added to the balance?
Kamran Shahid Monday, March 31, 2008
Decimal overflow might be expected as there isn't any check for the boundary condition.
Zeb Monday, March 31, 2008
interest does nothing ... it's a local variable, but it's being logged as being accumulated.

Also, I assume the transactional locking is done external of this class. Otherwise the balance will be wrong on concurrent updates.
edddy Monday, March 31, 2008
Other than than 10000 and 0.01 not being configurables and the log function in the same class

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
cSpare Monday, March 31, 2008
Afaik += and -= are not thread safe.
Amr Elsehemy Monday, March 31, 2008
Concurrency problems when updating at the same time?
Log might not be valid.
LaptopHeaven Monday, March 31, 2008
The algorithm is not mutual exclusive.
scott Monday, March 31, 2008
@Johnathon, @Ryan: yes - that's along the lines of what Leroy is thinking!

@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.
Tim B Monday, March 31, 2008
Okay, if we're focusing only on design here, I'd say the two most important refactorings would be moving interest rate accumulation and logging into separate classes. A bank account shouldn't be aware of the business rules around a rate table - it should ask for and be provided with the interest amount for its current balance. Likewise for accounts knowing how to log themselves.
simonjohnroberts Monday, March 31, 2008
I dont like the font, it's a bit small.

Plus I would never indent like that...

Zeb ... Monday, March 31, 2008
Sounds like YAGNI to me. Don't just stick an abstraction in there to replace one If-Then statement. When the next 2-3 business rules come in and make the code yucky, then refactor out to a strategy.

Mikeon Tuesday, April 1, 2008
Zeb ... has a point. You just can't put a pattern on everything. It's just not right.
scott Wednesday, April 2, 2008
@Zeb, @Mike: agreed with YAGNI. Let's just see where this goes, however (and we always have to simply the real world to make it fit on a blog, unfortunately).
Simon.Fox Wednesday, April 2, 2008
No one has mentioned using the Measurement patterns from Analysis patterns (Martin Fowler). These could help with a number of things relevant here.
Eric Wednesday, April 9, 2008
Leroy is probably lamenting not writing this as a TSQL stored procedure.
Comments are now closed.
by K. Scott Allen K.Scott Allen
My Pluralsight Courses
The Podcast!