-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Architectural resources to "tame" the codebase #694
Comments
🎯 Clean ArchitectureEver since I've learned about "clean architecture", I fell in love with this concept and employed it in my own projects. The project structure and the philosophy, as well as the design patterns, really helped separate all those concerns of my code. While it might be too much effort to rejig MaMpf to use this kind of architecture, it might be still worth to check it out and borrow some ideas.
Actual examples
|
📂 Folder structureIn my opinion, the topic of file/folder structure is one of the most important topics to achieve a sustainable codebase. I mean, it's like the house we're living in whenever we work with the MaMpf code, so it should feature a clear separation of what the rooms are for. And it shouldn't take too long to find a room. To be honest, I feel a bit uneasy with the Rails default folder structure. For example, I find it weird that it separates files by their type and not by the feature they belong to.
|
💫 Design patterns / RefactoringWe don't have to reinvent the wheel: there are many useful design patterns already out there that could be useful. But I feel like there is sometimes an over-emphasis on these patterns; more often than not some plain old Ruby classes should work just fine to create cohesive modules with low coupling between them.
Concerns
Service Objects
|
▶ Concrete ideas on how to proceed in our codebaseThese are just some ideas to make models smaller, more cohesive and reduce their inter-dependency (if there is any?).
Pain points to address
|
Thank you for all your work here!
As I did in our basecamp pings, I want to contribute this link, henceforth known as A. I personally think that using the things that Rails already offers (instead of completely overworking everything like the person in the first DDD link) would be a good way to go, also considering our limited resources. In the links above there are two links which are on different ends of the spectrum regarding this (in particular towards concerns, namely this one, henceforth known as B and the one I linked above. I personally have a preference for the archicture described in A, but I would love to hear your opinion on this, and I still can be convinced of B (also, B offers some concepts that we can use as well even if we follow A's way concerning concerns). This would be relevant already in #671 as it currently introduces a service model If I understand the point of view of A correctly (please correct me if I am wrong here), it would be better to do it this way (or in a similar way): # app/models/voucher.rb
class Voucher < ApplicationRecord
include Redeemable
...
end # app/models/voucher/redeemable.rb
module Voucher::Redeemable
extend ActiveSupport::Concern
included do
has_many :redemptions, dependent: :destroy
end
def redeem(...)
# what is currently in the VoucherProcessor
end
end such that in the redeem action in the Refactoring other classes could then follow a similiar pattern just as you described (for A, it would mean concerns). |
I agree that OOP is a tool we shouldn't underestimate. Creating POROs can greatly help orchestrating everything while keeping it DRY. Citing resource A: "Delegating functionality to additional systems of objects (AKA using plain object-oriented programming)". And yes: going full into DDD with clean architecture is not feasible for us with the already existing code. It might still be a valuable source for inspiration and we might borrow very specific things from it. A vs. B💠 ConcernsThe main point of B that rebels against A is this paragraph:
This is countered in A by:
So B argues that it's just a "facade", we clean the room by putting the dirt into smaller bins (e.g. using concerns aka "mixins"). On the contrary, A argues that this is exactly what makes looking at the room nicer. Bins can be labeled and then it's easier to work with those smaller pieces inside a cleaner room. The distinction can be really subtle. I find that pattern 6 (policies) in B provides a good example: class ActiveUserPolicy
def initialize(user)
@user = user
end
def active?
@user.email_confirmed && @user.last_login_at > 14.days.ago
end
end We could easily model this exact same thing using concerns. The only real difference would be that using concerns, we wouldn't pass the For a simple example as this one here, I'd prefer the point of view of B where we have a really independent PORO: both on the implementation level and the "runtime" level as it gets passed the But what do you do if you need more than a simple value object and instead access data from the database? You might need some Tip In my opinion, the better approach would be to first look at the infrastructure level and think about whether we can't outsource some parts of a class into a completely separate "thing" (PORO or ActiveRecord) that might stand with its own right (which might include adding another table in the DB). If that doesn't work, one can still resort to concerns in order to extend an existing thing by means of another file. Just one point that I don't like in the example in A: class Recording < ApplicationRecord
include Incineratable, Copyable
end
module Recording::Incineratable
def incinerate
Incineration.new(self).run
end
end For 💠 ServicesI feel that A & B somehow agree on the point that we shouldn't resort to services too hastily. From A:
And B (point 2):
For me that means, think twice when using a concept that resembles a service. Maybe a simple value object (or something else?) might fit better. But if an action is complex enough or even reaches across multiple models, we might consider using a service. Now A strains away form calling their PORO objects (that are kind of services) by that name: "service". For me it's still kind of like a service, but that's just naming. Whatever you might call it, I like A's idea of not treating services in a special way (for me that also means not necessarily putting them in a dedicated
See A's signup exampleclass Projects::InvitationTokens::SignupsController < Projects::InvitationTokens::BaseController
def create
@signup = Project::InvitationToken::Signup.new(signup_params)
if @signup.valid?
claim_invitation @signup.create_identity!
else
redirect_to invitation_token_join_url(@invitation_token), alert: @signup.errors.first.message
end
end
end
class Project::InvitationToken::Signup
include ActiveModel::Model
include ActiveModel::Validations::Callbacks
attr_accessor :name, :email_address, :password, :time_zone_name, :account
validate :validate_email_address, :validate_identity, :validate_account_within_user_limits
def create_identity!
# ...
end
end
The signup is modeled as PORO object and then instantiated. It uses See B's authentication exampleclass UserAuthenticator
def initialize(user)
@user = user
end
def authenticate(password)
return false unless @user
if BCrypt::Password.new(@user.password_digest) == password
@user
else
false
end
end
end
class SessionsController < ApplicationController
def create
user = User.where(email: params[:email]).first
if UserAuthenticator.new(user).authenticate(params[:password])
self.current_user = user
redirect_to dashboard_path
else
flash[:alert] = "Login failed."
render "new"
end
end
end I don't see a big difference here. In A, the controller stores the But this is actually a great example where stepping back for a moment yields another possibility, namely the one you proposed. Without any "service", but instead using concerns. Your proposed approach with concerns instead.class Voucher < ApplicationRecord
include Redeemable
...
end
module Voucher::Redeemable
extend ActiveSupport::Concern
included do
has_many :redemptions, dependent: :destroy
end
def redeem(...)
# what is currently in the VoucherProcessor
end
end I think this really shows the power of concerns. In this example, Voucher and Redemptions are closely related: redemption is an essential part of a voucher. So I feel it's ok to not model the redemptions far away from the vouchers and instead include them in the vouchers itself. But to avoid too much clutter in the vouchers, we use concerns to split it up to another file. Calling Maybe this also shows that we should sometimes just write down how we'd like the statement to roughly look like in the end (regardless of any squiggly lines and warnings you get in the IDE). For example, you might have written 💠 SummaryIn summary, I think the patterns presented in B can be very useful, if you happen to be in the exact or similar situation the examples were made up for. This is always the thing with these guides: there's no one size fits all solution and there won't be, as requirements are just too context-sensitive for the very specific programming project in mind. There's also no definitive "How to write a book" guide as there are just too many genres and everyone has their one styles. I like the pragmatic approaches taken in A even though they raise my hackles sometimes as it's so different to what I'm used to from (my limited experience with) DDD. But I acknowledge that their approach works as well and that Rails might even be designed in a way to tailor the specific needs of web applications where boundaries of layers can be blurry. And apparently, they navigate the landscape well with their approach. The more I work with Rails, the more I like their pragmatism.
As this is already our approach (I think?), maybe we can leave that. But if classes get too big, introduce composition within the domain model to outsource to objects on their own. Most of this can probably done within Maybe this whole thing is even more about just taking the time to step back and think through component boundaries:
For existing classes that are too big
As I'm also learning a lot of new things here, I might put forward contradictory arguments. Please point it out to me should I disagree with my own at some places ;) |
According to concepts discussed in #694. Also auto-load subdirectories in models folder and set Current.user for usage in models.
This is a "structured dumping ground" for useful resources that could help refactoring our code base. I think some issues are pretty-well summarized by a teaser for Jason Swett's book "Growing Large Rails Applications".
"How did our Rails app get to be such a confusing mess?"
While it might not be that bad, we currently tend towards that direction with ever-growing models that act more like a dumping ground. Let's tackle this issue. It's a big one of course and will probably need a lot of time to solve, if it's ever to be solved. It's rather a continuous improvement I guess.
I don't think there's this one recipe that you follow to achieve a super clean and understandable codebase. See also Rails can only take you so far. Apps are too different and operate in various domains that they need different "solutions". Instead, let's collect many resources here to learn about new design patterns and experience from other people that deal with big code bases. I don't think buying Jason's book is necessary (it is not that cheap in the end) and am certain that we can gather a good pool of websites, blog posts etc. to move forward.
Feel free to edit my comments down below to add links you find useful.
The text was updated successfully, but these errors were encountered: