Request for Code Review/Comment

Ruby on Rails
1

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.ā€

2

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
3

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!