Front-end Code Review Fundamentals

by Jack Haas on

We’ve been thinking about code reviews lately here at Advomatic.  Fellow Advo-teammate Oliver’s previous post covered the whys of our code review process, and Sarah covered the hows when she did a overview of some tools we use regularly.  This post focuses on the front-end side, and deals more with the whats of front-end code review… as in:

  • what are our main overall goals of the code we write?
  • what are the common standards we follow?
  • and what quick-and-easy changes can we make to improve our front-end code?

Guiding Front-end Coding Principles

In the world of front-end development, it seems like there is an intimidating, constantly expanding/changing set of tools and techniques to consider.  The good news is that there is a pretty dependable set of widely accepted working guidelines that aren’t tool-specific.  We really try to consider those guidelines first when we are reviewing a peer’s code (and the technical approach they have taken):

  • Write valid, semantic HTML5 markup – and as little markup as needed
  • Enforce separation of content, functionality/behavior, and presentation
  • The site should function without the use of JavaScript, which when added, is only used to progressively enhance the site. Visitors without JavaScript should not be crippled from using the site.

Best Practices

Sometimes there are a million ways to solve a single problem, but I think we do a good job of not pushing a particular tool or way of dealing any of those problems.  Instead, we’ll see if the code is efficient and if it can stand up to questions about its implementation, as far as best practices go.  For example:

  • Is there logic-related PHP in templates that should be moved to template.php?
  • Can JS be loaded conditionally from a custom module or the theme’s template.php via 
    $render['#attached']['js']

    so it doesn’t load on pages that don’t need it?

  • Is the project already using a DRY CSS methodology elsewhere?  BEM or SMACSS maybe?  Should it be used here?
  • Are we generally following coding standards?
  • Are we coding with web accessibility guidelines in mind?

Minimal Effort Refactoring or “Grabbing the Low Hanging Fruit”

Refactoring code as an afterthought (or as a reaction to some problem down the road), is never fun – it pays to be proactive.  Ideally, we should always be retooling and improving our code as we go along as we get more information about the scope of a project and if the project budget and/or timeline allows for it.  When we look at a peer’s code changes, what kinds of things can we suggest as a quick and easy “upgrade” to their code?

  • Markup:
    • Instead of that ugly Drupal-default markup we’re working with, can it be swapped out with an appropriate HTML5 replacement element?
    • Is that inline JS or CSS I see in the markup?  Crikey, what’s that doing here?!?
    • Do we really need all these wrapper divs?
    • Is this the bare minimum markup we need?
  • CSS/Sass:
    • Can we use an already-established Sass mixin or extend instead of duplicating styles?
    • Is this element styling worthy of a new mixin or extend that can be reused on other elements, either now or in the future?
    • Should this mixin actually be an extend (or vice versa)?
    • If the code is deeply nested, can we utilize our CSS methodology to make the selector shorter, more general, or more concise?
    • Is this selector too unique and specific?  Can it be generalized?
  • JS:
    • Does this work without JS?
    • Is there a chance this could adversely affect something unrelated?
    • Does it need to be more specific, or be within a Drupal behavior?
    • In Drupal behaviors, do selected portions of the DOM use the “context” variable when they should? For example: 
      $('#menu', context)
    • Is jQuery Once being used to make sure that code is only run once?
    • Should this code only run when we are within a certain viewport size range (a breakpoint)?
    • Does it need to be killed/destroyed when the breakpoint changes?
    • Would this functionality benefit from being fired via feature detection, especially if our target browsers are old-ish?

Quick Browser QA

I know, I know.  Browser QA is not really “code review”.  However, it does go hand in hand, and makes sense to deal with when you’re reviewing someone’s code.  It is at that point that you, the code reviewer, are most familiar with your peer’s work and specific goals.

While we do a more thorough and complete test and review of work in target browsers and devices at the end of an iteration (generally every two weeks), we also do a shorter burst of quick QA during an individual ticket’s code review.  We’ll quickly check it in the latest version of major target browsers/devices – this helps us find bugs and issues that are big, visible and easy to fix.  It also ensures that they don’t pile up on us to deal with during our final iteration… which can be really demoralizing.

Back to Basics

Code reviews are great for brushing up on the basics when other parts of your work can seem very complicated.  None of this is particularly revolutionary – it helps to revisit the basics of why you do what you do from time to time. Aside from reviewing the technical aspect of the work, you can act as an outsider that can see the big picture of how this work affects the user experience… easier than someone in the trenches.  This is an important and valuable role to play as things come together on the front-end of your project.