Three-layer Architecture about five-bells-ledger

[MatthewPhinney]

This will be a worthwhile exercise, particularly as the code base grows. I think the biggest issue to consider here is if we want to use the existing model class in five-bells-shared for the persistence layer.

[justmoon]

Totally agree that the ledger controllers should not contain the business logic. Express (and Koa) are MVC frameworks, so they really work best in conjunction with the MVC pattern. In MVC, business logic should be handled in the model.

In general I think there are a few common folder names for Express applications that we should adhere to, such as models/, controllers/, views/ (if any), middlewares/ (if any).

In terms of routers, I like the notary's setup. There's a Router class, which includes the controllers and has them register their routes. Finally, an "attach" method attaches the Router to the App.

https://github.com/interledger/five-bells-notary/blob/master/src/lib/router.js
https://github.com/interledger/five-bells-notary/blob/master/src/controllers/cases.js

Here are some resources on what others are doing:

https://www.terlici.com/2014/08/25/best-practices-express-structure.html
https://gist.github.com/lancejpollard/1398757
http://stackoverflow.com/questions/5178334/folder-structure-for-a-node-js-project

Note how MVC is pretty much universally used - it has worked well for me in the past and most engineers are familiar with it.

[clark800]

MVC is an architecture that is designed to separate the user interface from the business logic. Since this project doesn’t have a user interface (view), it is a bit of a square peg in a round hole situation.

If we want to use MVC for this project, we can really only use the “model” and “controller” parts because we don’t have any views. But consider some of the definitions of these terms:

“Controllers contain the interface between their associated models and views and the input devices (e.g., keyboard, pointing device, time)” - http://c2.com/cgi/wiki?WhatsaControllerAnyway

“In MVC, the Controller is a strategy that the view uses to handle user input. … In PAC, the Control is a mediator.” - http://c2.com/cgi/wiki?WhatsaControllerAnyway

Some definitions of “controller” don’t make sense without a view. And the term “controller” was apparently redefined at some point to fundamentally change its role in the architecture.

And regarding “models”, the links you posted say:
“models/ – represents data, implements business logic and handles storage”
“/models contains all your ORM models”

One says models contains business logic, which is the original definition, but the other doesn’t, and this is apparently what the current code interprets “models” as.

The fact that this repo uses MVC terminology, but doesn’t actually use MVC is evidence of the confusion behind the term “MVC”. Due to widespread abuse of the term, it seems to raise more questions than it answers in the web app context. Martin Fowler says “Probably the widest quoted pattern in UI development is Model View Controller (MVC) - it's also the most misquoted. I've lost count of the times I've seen something described as MVC which turned out to be nothing like it.” http://martinfowler.com/eaaDev/uiArchs.html

Another issue is that MVC does not provide an abstraction layer between the persistence layer and the domain layer, instead bundling both into “models”. This would be fine, and even preferable, for situations where endpoints are being mapped directly to the database with trivial business logic, but since we have non-trivial business logic I think it is better to keep the persistence code cleanly separated.

So to compare MVC with the three-layer architecture presented above,
the “controllers” directory is renamed to “server” to avoid confusion with the term “controllers”, and the “models” directory is split into “domain” and “data” to avoid confusion with the term “model” and to decrease coupling between business logic and persistence code.

[justmoon]

If we want to use MVC for this project, we can really only use the “model” and “controller” parts because we don’t have any views.

I think it's fine to use MVC and not have any views if your project doesn't need them.

One says models contains business logic, which is the original definition, but the other doesn’t, and this is apparently what the current code interprets “models” as.

Agreed, we should use MVC correctly.

Another issue is that MVC does not provide an abstraction layer between the persistence layer and the domain layer, instead bundling both into “models”.

That is totally fair.

I want to take one moment to make sure we agree on the fundamental approach to architecture. I believe there is no one right architecture. I think for a project that has a 100 lines total, the correct architecture may well be to have everything in a single file. For a project with 100000 lines, the architecture could be a lot more complex than MVC, or at least it could be MVC with a lot of additional separation like breaking models into different layers etc.

You want to avoid overengineering (a simple program turns into a complex set of units split over dozens of files) as much as underengineering (a complex program is implemented as a handful of tightly coupled God classes.)

The way you determine the right architecture is by refactoring classes that are getting too complex into separate units. And by getting rid of and merging objects that only exist for architecture's sake.

The split you're advocating is often called Domain Driven Design and there is some good discussion out there, such as this question from StackOverflow. Excerpt:

Many times, mixing infrastructure concerns with your "domain model" can help you achieve great strides in speed at the cost of scalability. The point is, you need to ask yourself, "Are the benefits of pure DDD worth the cost in the speed of development?".

Right now I recommend MVC (no domain/persistence separation) for simplicity. Yes, we may decide to refactor that in the future, but it's perfectly fine to incur some architectural debt if it means rolling out faster. That's the magic of refactoring and TDD.

So to compare MVC with the three-layer architecture presented above, the “controllers” directory is renamed to “server” to avoid confusion with the term “controllers”, and the “models” directory is split into “domain” and “data” to avoid confusion with the term “model” and to decrease coupling between business logic and persistence code.

This is where you're losing me. Yes, not every developer has the perfect understanding of MVC. But if they see controllers they're going to expect the Koa handlers and they're going to be right. I don't buy the argument that using terms that people are totally unfamiliar with reduces confusion. I get your point in principle - I just think that people's understanding isn't so bad and so confused that it would warrant that drastic of measures. There is a flipside to consider which is that using uncommon terms means a steeper learning curve.

Geert made a good point today that I haven't done a good job communicating the bigger vision behind the Five Bells projects. We'll set up a meeting about it, but the net result is that developer experience is an important design goal for these projects. That's one reason why I'm pushing for adherence to certain standards. And if that means we have to do fewer changes to the current architecture and we can roll ILP out faster, even better.

TL;DR - Agree with moving the business logic out of the controllers and to the models. Other architecture changes can wait.

[alandotcom]

But if they see controllers they're going to expect the Koa handlers and they're going to be right.

Koa refers to these as middleware and also doesn't refer to itself as an MVC framework.

A Koa application is an object containing an array of middleware generator functions which are composed and executed in a stack-like manner upon request.

The reason we may expect there to be a controllers directory that contains the Koa middleware may be related to the confusion @clark800 noted above

[clark800]

I'm going to start by fixing the controller/model separation, then we will be able to look at a more concrete example of what the domain/persistence separation looks like.

[clark800]

Here is what the database abstraction layer would look like: #123

I think this makes the business logic more readable since the low-level database code is out of the way and it also makes it easier to see all the database operations at a glance. The benefits will be more significant for the transfers model because the business logic and database code are more intermingled there.

[justmoon]

@clark800 The structure makes sense to me now and by keeping the MVC naming I can find my way around.

That said, I gotta play devil's advocate here and say:

Anytime you have a refactor that doubles the amount of code that it's refactoring it gives me pause. Refactoring should simplify. Ok, lines of code are an imperfect metric for simplicity. But you went from one class Account under src/models/account.js to:

• class Account under src/models/db/account.js
• src/models/db/accounts.js
• src/models/accounts.js

If I want to know what happens when I fetch all accounts, I first look here:

https://github.com/clark800/five-bells-ledger/blob/e4dc43a40a472a3350143cdafe6933583e6ec10b/src/models/accounts.js#L9
which is a thin wrapper around
https://github.com/clark800/five-bells-ledger/blob/e4dc43a40a472a3350143cdafe6933583e6ec10b/src/models/db/accounts.js#L6
which is a thin wrapper around Account#findAll.

Architecture should be a way to deal with increasing complexity. If we had a very big, complex Account class and this refactor was splitting it into three files, roughly keeping the amount of code the same (or even decreasing it) while separating concerns, it'd be great. But this seems like you're adding an additional layer, ending up with more files without significantly simplifying any of them.

I trust you to know what's best for the ledger architecture, so it's your call, but I would probably favor a single model class for now (which unifies the three files mentioned above) until that class gets too big.

[MatthewPhinney]

I generally agree with Stefan that refactoring should be an organic process of splitting the code into multiple files as the codebase grows. In this case, it doesn't seem like there is enough functionality to warrant splitting into two files.

That being said, I can see the argument for a clear separation of concerns. The business logic for transfers is quite a bit more complex, so this structure may make more sense when considered in that context.

[clark800]

a refactor that doubles the amount of code

The line diff metrics are a little deceptive; the added lines are mostly whitespace, function declarations, and exports; there isn't really any new "code" or complexity.

with more files

I don't really see this as a problem per se. Typically you want to focus on one thing at a time. If you are trying to fix the business logic, then you probably want the database code out of the way and vice versa. When you are working on the interface between them, you would have two panes side by side.

Regarding the two files in the "db" directory account.js and accounts.js: I think the next step would be to merge these together; the pull request just shows the first step.

Also, imagine if you wanted to make a totally different database backend that wasn't supported by the ORM, like storing all the data in a bunch of JSON files in the filesystem. With the database abstraction layer you can easily look at the files in "db" and just re-implement each of the functions without needing to touch the models or controllers at all. It's not that we will actually want to do this, but this flexibility is the mark of a solid decoupled design.

Architecture should be a way to deal with increasing complexity.

This refactoring is a response to the current complexity, but I think I have a lower threshold for when refactoring is appropriate. I think there are cases when a 10 line long function should be split up - the criterion isn't lines of code, it is whether it is mixing different layers of abstraction or if it has logical subcomponents that can be given a name that captures their functionality.

Also, it's better to be proactive about this kind of thing - even if it doesn't seem quite necessary in the accounts case, applying these constraints early on will ensure that future development will be cleanly decoupled and not require a more difficult refactoring in the future.

The business logic for transfers is quite a bit more complex, so this structure may make more sense when considered in that context.

Right, I did the accounts refactoring first because I wanted to do the quick and easy one as a sample, but the idea is to imagine extending this pattern to the rest of the code base.

[alandotcom]

Also, it's better to be proactive about this kind of thing - even if it doesn't seem quite necessary in the accounts case, applying these constraints early on will ensure that future development will be cleanly decoupled and not require a more difficult refactoring in the future.

This reflects my thoughts on refactoring in general. It's easier and less time consuming to have a template of how we want the code structured as Chris pointed out, it makes future development easier.

[gip]

I'd be in favor of having a clear separation between the different layers. Even though the complexity is low for the 5b components, I find it easier to code review a codebase where the separation of concerns is there by design. It's also generally a good idea in the long term as project complexity increases.

Stefan has a point about that fact than more code isn't ideal -- and indeed, one of the general truth of software engineering is that the amount of bugs in pretty much proportional to the number of LOCs. I don't see that as a big problem here however: in the long term, separation of concern should make code reuse easier and make sure the number of LOC doesn't grow too much. A short term loss for a long term gain.

[MatthewPhinney]

@clark800 can we close this?

[clark800]

It's not done and it's still in our backlog, so what is the reason for closing?