Upcase Code Contest

Video

Want to see the full-length video right now for free?

Notes

Contest Outline

This week we're trying out something a little different. Recently, we ran a group-coding app-building contest for Upcase subscribers. You can read the back story in the associated forum thread.

We asked subscribers to form teams, and for those teams to work on an application inspired by the site Goodreads, an app for tracking your reading, book lists, and reviews. It provided a great example CRUD application for us to work with in the contest.

The specific pages and functionality is outlined in detail in the associated forum thread. In addition, we provided a hint at the judging criteria we would be using including things like a clean git history, solid testing, logical data modeling & RESTful routing, and overall clean code.

Lastly, we provided a set of wire frames, like the one below, to provide a little extra guidance on the rough structure of the pages. It's worth noting that these were very low-fidelity mockups, and this maps to how we tend to work here at thoughtbot.

books page mockup

Common Aspects Across the Apps

Each of the teams' implementations were unique, but there were a number of common tools and approaches that are worth highlighting across the projects.

Suspenders

Every team used Suspenders to start their apps. Suspenders is thoughtbot's base Rails template that rolls up all of our preferences, tweaks, and best practices for starting apps. Suspenders represents thousands of hours of experimentation and learning, and even if you don't use it directly, perusing Suspenders's PRs can be a great way to learn.

If Suspenders is too thoughtbot specific for you, you can use Rails Templates to build your own Rails initializer.

Using PRs for Code Review

Every group used GitHub Pull Requests to submit changes and perform code review. We can't recommend this highly enough. Code review is one of the most potent tools for improving code quality and for sharing knowledge.

Take for example [this PR where the author learned something][] during the code review. Code review is about sooo much more than just catching bugs (although it can do that too!)

The Tips for Code Review episode of the Weekly Iteration
thoughtbot colleague Derek Prior's talk on Implementing a Strong Code-Review Culture

[this PR where the author learned something]: example PR with learning!: https://github.com/ChillinAstroTurtles/astro-books/pull/28

Using Automated Tools to Aid in PR Review

All of the teams used at least one automated tool / service to help review PRs, and most used multiple. The specific services used were:

CircleCI for continuous integration
Hound for automated style checking
CodeClimate for automated code quality & security checking

At this point these sort of automated services are a requirement on any app we build, and the free for open source plans make them easy to try out in side projects.

Using Trello for Project Management

Two of the teams used Trello to organize their work and do general project management. thoughtbot has tried (and built!) countless project management and Agile planning tools over the years, but recently we seem to be converging towards Trello thanks to it hitting the sweet spot between structure and freedom.

Automated Setup and Test Scripts

Nearly all of the projects had a bin/setup script and a solid Rakefile plus bin/rake script. This made it so setting up and getting to the point of first test run was essentially a 1 minute task for me. Again, can't overstate the value in keeping this onramp smooth and easy, especially when working with a team.

Solid Git History

All of the apps had clean Git histories. The commits were small and focused, and the commit messages did a great job of explaining the "why" to support the "what" of the code change.

To provide an example, check out this commit which explains the why very well. In addition, check out our recently released Mastering Git course for much more detail on how thoughtbot uses and thinks about Git and version control, especially the thoughtbot Git Workflow video.

Cover App

Check out the repo for Cover App to see the full code. Some of the particular features that stood out are:

Caching the Average Rating via a Background Job

The average_rating is a persisted attribute of a book, and with this comes a background job to recalculate and The average rating and a callback to recalculate this value over time.

We would recommend starting with the simplest option of a method that calculates this on the fly, and if performance became a concern, look to moving the calculation into a SQL query as the first potential optimization.

Great Feature Specs

The feature specs in this application do a great job of keeping to very high level, user facing description of the app. Each of the tests phases is clearly seperated with a new line, and they even went as far as to extract custom matches like has_book to keep that high level language.

The [user_views_all_books_spec][] is a great example:

feature 'User views all books' do
  scenario 'list all books' do
    create(:book, title: 'Practical Vim')
    create(:book, title: 'The Giver')

    visit books_path

    expect(page).to have_book('Practical Vim')
    expect(page).to have_book('The Giver')
  end

  # ...

  def have_book(title)
    have_selector('.book', text: title)
  end
end

[user_views_all_books_spec]: https://github.com/ajkamel/cover/blob/ec1487fa547f8bce071ac909de18797201ca67b1/spec/features/user_views_all_books_spec.rb#L4-L12

Vimstars

The next app is from team Vimstars, and unfortunately they were a bit time crunched so they didn't get as far as the other teams. None the less, you can check out [the Vimstars repo][] for the code.

[the Vimstars repo]: https://github.com/antwonlee/vimstars

Validations in Models but Not In DB

There are a number of validations implemented on the models, for instance validates :name, presence: true, but these are not reflected in the database. Typically we'll want to mirror every presence validation with a not NULL constraint in the database, likewise mirror uniqueness validations with a index, etc.

Controller Specs Test Default Behavior

While testing is great, like anything else in life, it isn't free. As with any production code, tests require maintenance and changes over time, and we want to make sure they're providing sufficient value to warrant the cost.

This app was one of the few with controller specs. Overall we're big fans of testing, but in this case these controller specs were largely testing the default controller behavior. For instance:

describe BooksController do
  describe 'Get #index' do
    it 'reponds successfully with an HTTP 200 status code' do
      get :index
      expect(response).to be_success
      expect(response).to have_http_status 200
    end

    # ...
end

This behavior will largely be covered by any feature spec that properly exercises the page (which we'll want to have anyway). We'll typically hold off on writing controller specs until we have unique behavior such as authorization checks added to our controllers.

Chillin Astro Turtles

Next up was the Astro Books app by the ChillinAstroTurtles team (what a name!). The [astro-books repo][] is available if you want to dive into the full code, but to highlight a few specific points:

[astro-books repo]: https://github.com/ChillinAstroTurtles/astro-books

Instance Variables and Before Blocks in Feature Spec

The feature specs in this application had a different shape than those in most of the other apps. In particular, the feature specs use a number of instance variables and a before block to set up the context for a given spec.

In general we prefer to localize all setup into the test and rarely, if ever, use before or let or similar constructs. The thinking behind this is we want each spec to tell a complete story, and for a reader of the spec (say, you in two months when the spec breaks) to be able to come in and quickly have all the needed context without needing to hunt around the spec file. Three great resources that cover these ideas in more depth are:

The classic [Let's Not post][] on the Giant Robots blog
The [RSpec Best Practices][] episode of the Weekly Iteration
The [Testing Antipatterns][] episode of the Weekly Iteration

[Let's Not post]: https://robots.thoughtbot.com/lets-not [RSpec Best Practices]: https://upcase.com/videos/rspec-best-practices [Testing Antipatterns]: https://upcase.com/videos/testing-antipatterns

Modeling of the Genres

Most of the apps had a similar structure, but it's worth highlighting the data model for the genre (called Categorization in this app) as it is a very clean and straightforward implementation.

Specifically, the genres and books share a many to many relationship, and they modeled this using the "has many through" relationship:

# app/models/category.rb
class Category < ActiveRecord::Base
  has_many :categorizations, dependent: :destroy
  has_many :books, through: :categorizations
end

# app/models/book.rb
class Book < ActiveRecord::Base
  has_many :categorizations, dependent: :destroy
  has_many :categories, through: :categorizations
end

# app/models/categorization.rb
class Categorization < ActiveRecord::Base
  belongs_to :book
  belongs_to :category
end

Non Restful Routes

Looking at the [BooksController in this app][], we can see that there are two non-restful actions, specifically lists and list. Likewise, we can see the associated routing:

Rails.application.routes.draw do
  # ...
  get "/lists", to: "books#lists"
  get "/lists/:id", to: "books#list", as: :list
end

This is a case where we would strongly recommend leaning into the typical Rails convention of the seven RESTful actions exposed via the resources method in our routes.

In this particular case, we can directly map these two List related member actions out into their own ListsController as the #index and #show actions respectively. Lastly, the two routes can be replaced by:

Rails.application.routes.draw do
  # ...
  resources :lists, only: [:index, :show]
end

[BooksController in this app]: https://github.com/ChillinAstroTurtles/astro-books/blob/243b2939cc7bc286ff6c349a58e835d2e65e3cc4/app/controllers/books_controller.rb#L16-L22

Ruby Bookshelf

The final app, and our contest winner, is the Ruby Bookshelf app. You can see the full source of the app in [the rubybookshelf repo][], but again to focus in on some of the more interesting points:

[the rubybookshelf repo]: https://github.com/taylorrf/rubybookshelf

Extra Data in Specs

Overall the specs in this app, especially the feature specs, were extremely clear and high level. Similar to other apps, the feature specs had clearly separated setup, execution, assertion test phases, as well as extracted domain specific matchers like have_book and have_heading.

One of the tests, specifically the [sort_by_date_spec][], had an assertion that is worth discussing. The spec was covering the ability to sort the books list by date, but the assertion was specific to the titles of the books. This can work, but it's all the better if we can be slightly more explicit in our assertion to highlight the thing we are testing, namely the date based sorting in this case.

Justin Searls' talk [How to Stop Hating Your Tests][] has a great section on how to keep your tests focused and ensure they don't include extraneous detail that might distract the reader.

[How to Stop Hating Your Tests]: http://blog.testdouble.com/posts/2015-11-16-how-to-stop-hating-your-tests.html [sort_by_date_spec]: https://github.com/taylorrf/rubybookshelf/blob/8b94ee49cb6ebec4fcf567bb66298604c07dddfe/spec/features/sort_by_date_spec.rb#L12-L16

Factories File

The [factories file][] in this app deserves special notice as it is a really clear example of how [FactoryGirl][] can best be used. The factories are minimal and focused, not specific to any edge case or building a specific test object. That said, there are a few great examples of more advanced FactoryGirl features like trait and sequence mixed in to make this an A+ FactoryGirl example.

[factories file]: https://github.com/taylorrf/rubybookshelf/blob/8b94ee49cb6ebec4fcf567bb66298604c07dddfe/spec/factories.rb [FactoryGirl]: https://github.com/thoughtbot/factory_girl

Explicit View Rendering w/ Locals

One very unique aspect of this application is the use of locals, rather than instance variables, to render views for controller actions. This excerpt from the [BooksController][] demonstrates this approach:


class BooksController < ApplicationController
  def index
    render locals: {
      books: Book.sort_by(params[:sort]).page(params[:page]),
      filter: Filter.new
    }
  end

  # ...
end

This is a much more functional approach to rendering a view, and it takes out the magic of passing instance variables, replacing it with an explicit enumeration of the data being made available to the view. thoughtbot is currently experimenting with this approach on a few projects, but I'm personally a fan. I may not be a Pythonista, but I'm totally on board with the [Zen of Python][], especially the "explicit is better than implicit" bit.

[BooksController]: https://github.com/taylorrf/rubybookshelf/blob/8b94ee49cb6ebec4fcf567bb66298604c07dddfe/app/controllers/books_controller.rb [Zen of Python]: https://www.python.org/dev/peps/pep-0020/

Our Next Contest

Since we had a great time with this contest we've decided to run it back and host another Upcase code contest. Head on over to [this forum thread][] for details on the timeline and structure!

[this forum thread]: https://forum.upcase.com/t/the-second-upcase-code-contest

The Weekly Iteration

37 minutes