technical_debt.md

How We Are Trying To Limit Technical Debt

Rails is a great framework for getting started quickly. You just need to generate a few models and off you go! The problem is that this invariably leads to unused and undermaintained code accumulating in different places like your models, views, and controllers all over your project. We have all learned a few basic mantras like "fat models, thin controllers" to combat some of this cruft, but it still leads to your models acquiring hundreds of methods and your views as branching nests of conditionals. We're trying a few techniques on this project to forestall that that could be summarized with the following principles:

1. Long model classes are bad
2. Composition/delegation is better than inheritance
3. NullObject is better than nil
4. View logic should not be in the templates

This document will explore how we try to adhere to these principles in how we organize the project. We don't usually design with these approaches up front -- it's often more effective to refactor after the fact -- and there are definitely areas where code could be improved still. But it's a continuous process we are learning from and implementing when it's time to reengineer certain components. We are indebted to Kane Baccigalupi for all her insight and experience with this approach.

An Illustrated Walkthrough

Long classes are often flagged as a code smell because it's hard to keep track of all the methods within them and easy to keep around methods that a developer built in case someone needed it (nobody did) or worse still to reimplement another method under a new name (display_price, formatted_price, page_price). Similarly, long classes are often trying to fulfill several distinct purposes at once, which compromises their conceptual clarity and makes it confusing for new developers to understand how they work.

We tackle that by using Plain Old Ruby Objects (POROs) to wrap our basic classes in additional functionality when we need it. This is called the Decorator pattern if you are used to design patterns, but we tend to give these classes different names based on their functionality. Here is a good primer on various ways in which this approach can clean up Rails projects if you wanted a bit more background, but this is how we apply it to this project.

If you don't know what a Decorator is, it's an object that wraps another object inside of it. It might delegate methods directly to an encapsulated class -- so a method like available? might just call that method on the internal object -- and it might define new specific methods that use one more or more contained objects. Decorators can wrap other decorators in turn, although once you get a few levels deep, it gets hard to figure out all the delegation and exactly where that method you are calling is defined, so resist the urge to make the stack too deep. ` Because we want to avoid large classes that contain many methods, we have a large number of small classes that each contain only a few methods. It can be a bit hard to understand where to start, but this document is here to help! And to be helpful, we have grouped these classes into broad subtypes of functionality:

1. ActiveRecord models
2. Ruby classes for basic non-AR-backed models
3. Presenters that wrap basic objects and provide special display functionality
4. ViewModels that collect methods for rendering objects on a page
5. Services that represent specific actions on objects
6. Parsers for creating and updating objects from form input
7. Validators that apply specific checks to objects
8. Serializers for converting objects to machine-readable outputs
9. Authenticators objects for representing specific authentication mechanisms

A Basic Request

I'm going to look at the Auction model and the various classes around it as an example of how we are applying this approach. Here is the code from the controller for executing the list of controllers on the homepage:

def index
  @auctions = AuctionsIndexViewModel.new(
    auctions: published_auctions,
    current_user: current_user
  )
end

def published_auctions
  @_published_auctions ||= AuctionQuery.new.public_index
end

The classes involved in this exercise give a good illustration of how the various classes relate:

1. AuctionQuery: this is the class where all queries against the Auction model are defined instead of using a bunch of named scopes in the AR model. The AuctionQuery#public_index method returns a list of Auction objects in reverse chronological order. Defining the methods here instead lets us keep the AR model clean and gives us an easy way to wrap responses in decorated objects if we need it. It also lets us forcibly execute the scopes where they are called in the controller (Rails will defer scopes until use, meaning you see an obscure error in your view when the query results are first used there).
2. Auction: this is the ActiveRecord class you'd expec. We use this for only basic DB access methods as well as defining AR relationships. You could also put named scopes here, but hold off on that since it's another way to accumulate things you don't need. The only thing that should go in the AR model are truly basic methods that apply to everything, but resist the urge to add new methods in there first.
3. The current_user in this case is actually an delegated method in the ApplicationController that calls the method in an internal instance of either the WebAuthenticator or ApiAuthenticator that was used to authenticate the user depending on which mechanism they accessed the application through.
4. Both objects are used to construct an AuctionsIndexViewModel object in the controller. This is then passed to the ERB view that renders the page. View Models are convenient places for helper methods to render objects correctly as well as conditional branches that would otherwise be hidden inside of ERB templates.
5. To render each auction in the list on the page, the ViewModel maps each auction object in the public_auctions array to an AuctionListItem object. This object contains useful methods for rendering attributes of an auction like user_bid_amount_as_currency as well as methods for picking the appropriate partials to display depending on the auction and user.
6. The AuctionListItem class also delegates to methods from an internal object. So, a call to AuctionListItem#available? within a view will actually call an internal variable instance of the BiddingStatus with that method. This approach lets us collect related functions like all the methods for querying the availability of an auction in a single focused place unlike having them be scattered among many methods in the Auction base class for instance.
7. In some cases, the method is called against an internal instance variable that could be one of several distinct types depending on the auction and or user. For instance, we demarcate an auction on the home page with an OPEN, CLOSED, EXPIRING or FUTURE label depending on what the current status of the auction is. Instead of using a series of if-elsif-elsif-end statements, this is accomplished through polymorphism. The StatusPresenterFactory class selects an appropriate presenter for the auction status and returns an object that supplies the appropriate values for labels or titles as needed.
8. We have also designed some basic presenters for transforming raw values from AR models to standardized formats we want to display to users. So, the AuctionListItem class calls such presenters as Currency for monetary amounts and HumanTime. Another big presenter is DcTimePresenter that is used
9. And if there is a concept whose meaning might change depending on the auction or user that is used in several places, it makes sense to define an appropriate class for encapsulating that logic in a single place and instantiating as needed. So, the WinningBid object is a class that represents the concept of a winning bid, whose implementation might vary depending on the auction (and whether the auction is closed or not)

This might seem complicated at first when compared to Rails' basic Model-View-Controller organization, but it provides a much more solid way of handling rapid growth of functionality and avoiding large files that contain every function under the sun and are unusable as a result.

Users Place Bids

Rendering auctions is one thing, but this system must also handle user interaction and do that in such a way that the functionality is clean. Normally, this might mean a lot of spaghetti code in either controllers or AR models depending on which developer wins the fight over where the code should go, but to keep things clean, we should define all that in other types of classes we haven't seen already. Let's look at what happens when a user places a bid. Here is the relevant code in the controller:

@bid = PlaceBid.new(params: params, user: current_user, via: via).perform

respond_to do |format|
  format.html do
    flash[:bid] = "success"
    redirect_to auction_path(@bid.auction)
  end
  format.json do
    render json: @bid, serializer: BidSerializer
  end
end

This hits a few other types of classes:

1. PlaceBid is a Service object that represents a specific action that changes the state of the system with all the information needed to execute it. In this case, we instantiate it with the parameters from the controller, the current user and a flag to indicate whether the bid came via the web or API. Its perform method in turn instantiates the auction from the ID, verifies that the user can indeed place a bid and the amount is valid, and returns the Bid object created.
2. PlaceBidValidator is what does the actual validation of a potential bid. This follows the established convention for Rails applications in which custom validation should appear in its own validation classes. Although we have gone beyond the basic organizational approach in some cases (and ignored them in a few cases like ViewModels, which are much more powerful and scoped compared to helper methods), we try to stick to Rails' conventions as much as possible. This would also allow us to use the validation if we had another mechanism for creating bids that was separate from PlaceBid.
3. Because different auctions have different rules on eligible bids -- for instance, a sealed-bid auction only allows the user to bid once, while a regular auction requires that the new bid must be lower than all other bids -- PlaceBidValidator calls a RulesFactory to load the appropriate rules for the auction. A rules class like Rules::ReverseAuction Rules::SealedBidAuction encapsulates rules about what the maximum allowed bid is, whether to show all bids.
4. Serializers like BidSerializer or AuctionSerializer describe how to represent objects as data in API responses. Because we want to return more information to administrators, there are also equivalent serializers for the admin controllers that return privileged fields in their responses.

Administrators Create Auctions

One last example to reinforce how things are organized. When an administrator creates a new auction, the relevant code in the controller looks like this:

@auction = BuildAuction.new(params, current_user).perform

1. BuildAuction is another Service object that represents the action of creating an auction and sticks to the familiar pattern with a perform method.
2. Within this class, the AuctionParser handles the process of validating input from the forms and converting it to the appropriate types in some cases. We defer most of our auction validation until the auction is published, but other parsing classes like the DateTimeParser handle specific data conversion tasks.

The AuctionParser and its related classes are the final major subtype of classes in our code and with that our tour is complete.

What Goes Where

The scenarios above should've given a rough overview of the various models in actual use, but here are some additional details on our organizational style.

Action Classes

One of the tenets of this design is that all important actions on an object should be placed into their own separate classes. These are sometimes called Service Objects or Policy Objects, but I prefer to call them Action Classes. Each of these classes implements at least two public methods:

1. initialize for setting the object or whatever else the action needs to run
2. perform for executing the action and returning the object

This changes the way the controllers interact with objects. Instead of calling ActiveRecord methods directly, the controller instantiates an object for an action and calls its Perform method. So, here is how the auctions_controller records a new bid:

@bid = BidPresenter.new(PlaceBid.new(params, current_user).perform)

In this case, the controller is instantiating a new PlaceBid object, then calling perform on it and then wrapping the ActiveRecord Bid object returned in a BidPresenter object. This might seem weird at first -- OOP urges us to think of our classes as nouns instead of verbs -- but it offers certain advantages over controllers calling models directly. It lets us place auditing or other controls in a single place without having to edit controllers or API methods to be consistent. It also would be simple to switch to an asynchronous job-based approach for any actions should we need to. And we can test the action with unit tests instead of controller tests.

View Models

View Models are a common approach for solving two other issues that commonly strike Rails projects of a certain complexity. We've all had projects where there are views that are filled with a lot of <% if ... > <% elsif %> <% end %> logic, and view templates are really the worse place to express and see how much branching logic you have. The second issue is about Rails helpers. All helpers basically are defined in a single global namespace and as the number of methods goes up, the more unwieldy your project's codebase becomes. It's hard to know if you can tweak or remove code in a helper without changing a view that uses that method elsewhere.

We solve both of these issues by defining a specific new decorator class that's used for wrapping the objects and helpers needed by a specific controller view. So, for the auctions_controller#show method, we have defined a AuctionShowViewModel class that wraps the current user and the auction we are showing. This lets us scope our view's helpers to be specific to this view model (or to delegate to the shared AuctionShowViewModel object). Furthermore, instead of doing inline conditionals in our views, we can instead define new partials for each distinct switch of the page's logic and replace that convoluted branching code in the partial with a helper method in the View Model that returns which partial to render:

<div>
<p class="auction-label"><%= @view_model.auction_status_header %></p>
<p class="auction-label-info"><%= render partial: @view_model.auction_status_partial %></p>
</div>

This makes our views smaller and more modular and lets us test which partial to render as a unit test instead of a functional test should we decide to do that.

Polymorphism Is Better Than Case Statements

Sometimes your have a situation where there are several related methods which have the same branching logic inside. For instance, we might have a method for an auction label and an auction stylesheet class and an auction display type, each of which checks if the auction is open or expiring or closed or such to determine what value to return.

The risk of such an approach is that you might forget a branching conditional in one of these methods and it creates a lot of tedious complexity even if you don't. To fix this, there are a few places where we delegate to an associated object and use polymorphism. So, we can define classes like AuctionStatus::OpenViewModel and AuctionStatus::ClosedViewModel each with its own methods like label or status that the auction presenter can delegate to by calling an instance of the appropriate class.

delegate :status, :label_class, :label, :tag_data_value_status,
         :tag_data_label_2, :tag_data_value_2,
         to: :status_presenter

private

def status_presenter_class
  status_name = if auction.expiring?
                  'Expiring'
                elsif auction.over?
                  'Over'
                elsif auction.future?
                  'Future'
                else
                  'Open'
                end
  "::AuctionStatus::#{status_name}ViewModel".constantize
end

def status_presenter
  @status_presenter ||= status_presenter_class.new(self)
end

This isn't really worth it for a little repetition in two methods, but it can be very convenient if you find yourself repeating the same conditionals in many methods.

Similarly, we have a few places where we branch depending on the auction type. This will likely make a good candidate for similar refactoring in the future.

Composition is Better than Inheritance

The way we handle different rules for different types of auctions is a good example of how we favor composition/delegation over large inherited types. Currently we have two different types of auctions -- a basic descending-value auction where all bid amounts are visible during the process and a sealed-bid auction where each user can only bid once and the winner is revealed when the auction is lower. We might add more, but these auctions have slightly different behavior from each other in a few different ways:

1. Can the user bid on this auction?
2. Should I show the user the amounts of other bids?
3. Is this bid from the user valid?
4. What summary information should I show for the auction on the home page?
5. What summary information should I show for the auction on the auction's page?
6. Should the user be able to see a list of bids when the auction is open? When it's closed?

and so on. We might consider implementing this by defining a ReverseAuction class and also a SealedBidAuction class with the variations encoded within. And this works okay, but what if we decide to add some more auction types? For instance, a ReverseAuctionWithBuyNowButton or a SealedBidOnlyForSmallBusinessVendorsAuction or a ReverseAuctionForSmallBusinessVendorsWithBuyNowButton or such? Ruby's single-inheritance starts to get unwieldy fast and what we really want is some sort of idea of mixins. So what if we do that.

Right now, there are several distinct places where we need to know what the Rules for an auction specify: when we are rendering a page, when we are validating a bid, when we show a winner. Currently both auctions are over when time is up, but if we added a Buy Now button, auction availability would also vary according to the rules. We could implement these variations as a sequence of if-then-else statements, but it's better to extract to polymorphism like above. And so, we have a series of Rules objects and auctions specify in a field which rules they use, but all are of the same Auction type. When we need to also specify Eligibility separately, we can do that with a different field without creating an OOP explosion.

Null Objects

Polymorphism is also useful for reducing the logic of null responses. Too often, we have code like this

user_bid = auction.user_bid

if user_bid.nil?
  "n/a"
else
  user_bid.display_amount
end

or such. This is a bit strange since it means that downstream users of an object have to expect either an object or a nil in all the places they call your methods. This means a lot of branching and also the possibility for some nils triggering fatal errors. What if instead of returning nil, we could define an Null class like this

class BidPresenter
  def display_amount
    bid.amount
  end

  class Null
    def display_amount
      "n/a"
    end
  end
end

Then, the user_bid method would return either an instance of BidPresenter or NullBidPresenter, both of which define a display_amount method. This then lets us redefine our original code to just be

user_bid = auction.user_bid
user_bid.display_amount

Much cleaner. There are a few places where we define Null equivalents to the BidPresenter and UserPresenter object for instance.

One other example of this technique is the Guest class. So much of the application is oriented towards users being logged in, but we should handle cases when there is no user logged in. Instead of checking if current_user.nil? everywhere, the WebAuthenticator instead returns a Guest object if no user is logged in.

def current_user
  @current_user ||= User.where(id: controller.session[:user_id]).first || Guest.new
end

This is turn is wrapped by the GuestPresenter, which allows the app to specify special partials for guests instead of regular users

  def nav_drawer_partial
    "components/guest_nav_drawer"
  end

  def win_header_partial
    "auctions/reverse/guest_win_header"
  end

  def nav_drawer_submenu_partial
    "components/guest_nav_drawer_submenu"
  end

You Have To Make Things Worse Before You Make Things Better

This structure was not created in advance and filled in later. Instead, this is the Nth iteration and reorganization of existing code. It's perfectly okay if things get messy and code is duplicated or classes collapsed first before you start refactoring. Don't be afraid to commit ugly and transitional steps on the way to a cleaner organization, as long as you stick to three basic rules:

1. Don't try to commit too many changes in a single pull request
2. Make sure the tests pass and cover all your new code
3. Relax about CodeClimate/Rubocop and other scores until you're done with the messy work

And remember that sometimes you might have to abandon a particular effort because it doesn't work or is too unwieldy. That's okay. You learned something in the process that will make the next attempt better.

Further Reading

• Practical Object-Oriented Design in Ruby
• Objects on Rails
• Sevice Objects
• Nothing is Something