Request for Code Review/Comment

Hi all,

This is a slightly weird situation but given this codebase (http://github.com/pedrosmmoreira/digibankdemo) and the following notes, could you give me some feedback? Feel free to bring on all the pain :slight_smile: Thank you!

“I opted to go with as minimal a implementation of a double entry book keeping system. Since we were only concerned with Transfers at a UI level, I used a form object to keep a layer of indirection between the stored data and the interface. My intent with that is to put in place an object that, given a set of data, will be able to generate the necessary objects to execute a transaction. Again, using a template/strategy pattern to specialise and properly encapsulate an operation. The ground work for this is already in place in the form of the DepositTeller object: given an symbol representing the operation, it is can initialize the proper object, in this case only a deposit. That is the reason for that object not presenting a cohesive api - the public API is the “deposit” method, which just calls to the generalised builder method, guarded by the allowed operations constant. I would also prefer to scope the call to constantize in a module (i.e. “Operations::#{request_operation.to_s.classify”.constantize) thus ensuring that only objects in that module can get instantiated (a lot of surprises could occur if we had a :queue operation for instance :slight_smile: ).
Finally, the industry standard in banking is moving this logic to stored procedures on the db and I’d like to play with that, but realistically, regarding persistence of data, I’m keen to stop depending on ActiveRecord callbacks to update account balance. Given the importance, I’d like to have that logic live on an object I can call .new on and have full API control over.”

Overall it’s very clean. I particularly like how RegistrationForm aggregates the validation errors from both models.

A few minor points of criticism:

  • Some of the code appears to be anticipating additional functionality which isn’t yet implemented (possible YAGNI). For example, DepositTeller::ALLOWED_OPERATIONS is an array of only one element.

  • I think the metaprogramming in DepositTeller adds unnecessary complexity and reduces readability.

  • The models have some methods related to views, e.g. to_partial_path. Those belongs elsewhere, e.g. in a decorator.

  • I prefer that a model doesn’t directly call the ActiveRecord methods of another model, e.g. account.update_attributes!. The interface should better reflect the domain, e.g. account.update_balance.

1 Like

Thanks, Andy! I agree across the board: check the notes for reasoning behind the DepositTeller class. The “partial_path” is to allow me do do polymorphic view partials - the view just calls render on a collection. I guess I could map them to decorators, but I dont have other methods to extract to them so far. And those callbacks do smell to high heaven :slight_smile: Thanks again!