Tuesday, May 29, 2012

account.transfer(..) - is it good OOP?

There is this famous example, that you can find in almost every OOP-related book:

Let's say that account is an object, and you have:

account.transfer(200, target_account)

For some time now, I've had issues with this example.

Is it really a responsibility of the account object to transfer money to another account?

Is it not one of the responsibilities of the Bank object? It just feels weird that an account knows how to do the proper stuff about the complicated transferring use case.

The topic of DCI is really popular recently, which is cool, as it's a very fresh approach to object oriented architectures. The .transfer example in DCI is usually implemented as injecting the SourceAccount and TargetAccount roles to the basic account objects. We change the implementation, but the effect is the same - the account object has the knowledge about the transfer stuff.

Wouldn't it be better if we have this as a responsibility of the Bank object? It could work as a use case or as a role, but both fit better in the bank, in my opinion.

What do you think?

 If you read this far you should Follow andrzejkrzywda on Twitter and subscribe to my RSS.

17 comments:

kikito said...

Well, you certainly could do this:

Transfer.new(200, source_account, dest_account)


The adecuacy of this depends on whether it makes sense to have Transfers as individual objects in your system. I a banking system it would probably make a lot of sense, but in a video store app it would probably be overkill.

Andrzej Krzywda said...

Yes, wrapping it in a transfer object is certainly a good idea.

I don't think that it may be an overkill in smaller apps. It's always good to have a nice OOP design.

Paul Keeble said...

There is an infinite number of ways to represent this. You could really start to pull out every single piece of functionality into differing abstractions and combine them in wonderful ways.

This example is used this way because its how many people think about transfers between accounts, and pretty much everyone has a bank account and does transfers. So its a universal concept.

Many banks actually create a transfer object, containing the amounts, the two accounts. They apply initially a reduction in the source account (but its more complex than that) and then execute the transfer separately via a transfers system to other banks if needed. But how any organisation decides to do that really depends on their model of the world and the way it works for them.

Andrzej Krzywda said...

Thanks Paul, wise words.

I'm just unable to find models where it feels good that the source account should know about the target account, even for just one second.

Bartosz Blimke said...

In OOP, an Account should know nothing about other accounts.

It's very common antipattern to find in "too fat" ActiveRecord models, responsible for everything related to them.

Andrzej Krzywda said...

Bartosz, exactly, it's very common in Rails/ActiveRecord, however as I wrote, this example is very popular in many other places.

Martin said...

If it was really banking software it would probably look more like:

transaction = Transaction.new
transaction.add(Operation.new(account1, amouont, TYPE_DEBIT)
transaction.add(Operation.new(account2, amount, TYPE_CREDIT)
transaction.process

And this snippet would be all over the place :)

Michal said...

Just thinking... Isn't the bank an object doing a transfer within his infrastructure instead of transfer "doing" itself?

Transfer.new(src, target, amount)
Bank#execute(transfer)

There is several possible ways to di that. One is more OOP-ish than the other. This classic account#transfer is just to scratch the sufrace of OOP for beginners. Sadly some developers see nothing wrong with that.

Ed Blackburn said...

In my mind a balance (pardon the pun) needs to be found between modelling the real semantics of the problem domain and writing maintainable easy to understand code.

I think a Bank type would be something of a god class and open to abuse?

A Transfer type maybe more sensible especially if it is invoked as a result of a transfer screen / api?

I would argue that context is very important and in a large system you will have more than one context and thus more than one Transfer type.

Concepts such as Boundary Contexts [1] are probably overkill in most applications though helpful in large financial apps (certainly from my expereince where they swifty turn into big balls of mud and entropy is smacking you on the nose with technical debt every day)

Ultimately...it depends :)

michał said...

I agree with kikito and have the same idea. Bank is better for manage account, transfers etc, Transfer should have responsibility for changing account price.

Responsibility for account is probably information about owner and price state, not for managing this data, and for risk operation like transfer I think special class is obvious.

Johno Schank said...

Why not something like:

Account.deposit(otherAccount.withdraw(200));

Though I agree, putting it in a transaction object which can manage any exceptions is probably a better choice.

John

Anonymous said...

Accounts should not know about other accounts. They don't need to. If it helps your code to let them manage transfers, your code is broken. I think examples keep it 'simple' to prevent the introduction of semaphores etc at beginner level. Try multithreading an app where account.add() works :-S

nagisa said...

I'd just think about that like real money handling.

* You are in control of your own pocket.
* Only you know how much money there are.
* Only you can take out money from your pocket and give it to other people (otherwise it is stealing).
* If receiving party doesn't want to take money then who cares, money is yours after all!

So
John.give_money(allowance, JohnJr)

Brendan said...

Your design will need to grow from your actual use case. What you propose may be sufficient, but there may be much more involved in "transfer 200 to an account."

For example, some systems might require double-entry accounting. I've use a helpful article on double-entry accounting to guide my design in the past, and I'd recommend reading it to help inform this discussion.

That model seems over-engineered for many scenarios. For my use cases, building out a batch & journal system was unnecessary -- volumes are low, and those can be added later. Additionally, I removed the separation between "account" and "user" since our system only ever has a single account per user.

One way to consider whether the design is "good" is to figure out what you can and can't change if you adopt that design. By adopting the "posting" & "asset_type" pattern from this (I always rename them to "transaction" and "transaction_type"), your system could easily grow to provide more complicated accounting and reporting functionality. Simply doing "account.transfer(...)" might indicate that you're missing supporting infrastructure, the lack of which might bite you later.

Sjuul Janssen said...

That would make the Bank object some type of God object. I wouldn't do it.
http://en.wikipedia.org/wiki/God_object

Resistance said...

PaymentOrder p = new PaymentOrder();
p.setRemitterAccount(...)
p.setBeneficiaryAccount(...);
p.setAmount(new MonetaryAmount(new BigDecimal("200.00"), Currencies.USD));
paymentService.debit(p);

It really comes down to what makes sense in your particular context/use case

Unknown said...
This comment has been removed by the author.