So you have a PR: A Code Review Checklist

Reader, be it is open-source or in-house code, front-end or back-end code, the pull request (PR) model is widely used in web development. But, before a PR is merged, it should be reviewed by peers, and the code modified by the developer as needed, in an iterative process. Ideally, we want to spend quality time reviewing each modified line so that we don’t build tech debt when code is added or modified.

But reader, you know as well as I that with pressing deadlines, sometimes we simply don’t have enough bandwidth for code reviews. And when we do have time, we may not be doing as good a job as we could because we don’t have a clear set of team coding guidelines or we may not know what we should look for. So, below find some suggestions about how code review can be done, with specific front-end things to focus on.

Prerequisites:

1. Thorough and meaningful code reviews take time. We need to scope for code review just like any other task. For that to happen, everyone must believe in its purpose and importance — that means management and product managers must actively encourage code reviews and give developers the necessary time.
2. The team has agreed on naming convention, styling, and coding standards, whether it is HTML, CSS, or JS. Styling standards give the code a consistent and professional look. A naming convention makes it easier to understand, search, build, and maintain. Coding standards make the code cleaner and easier to read, build, maintain, and debug. There are many popular standards out there. It is less important which one you choose, but it is crucial that the entire team follows one standard. If a rule is controversial within the team, discuss and decide, then change the standard and apply the new rule across the entire code base. Consistency is key.
3. The team has agreed on what to review — from spaces and camelCase, to where business logic should be, to whether the new code is violating established architecture.

When Reviewing HTML:

• Does the code follow agreed-upon standards and conventions?
• Does the markup use the same element ID name more than once? If so, change it to a class because each ID should be unique. If a reusable module contains an element ID, the ID is no longer unique once the module is reused in the same page. Remember that getting an element by ID returns only the first element that matches, which may cause issues when you have multiple elements with the same ID name.
• Are there unnecessary wrapper elements? If a <div> does not have a class, or the <div> has a class but the class has no associated styles, chances are that it is not needed. Remove it and check the layout. If the layout does not change, you do not need that wrapper.
• If the app uses a template language that allows complicated logic, are you over-using it, thus making the markup hard to read? Consider applying the logic to the view model BEFORE the view model is passed to the template.
• Does the markup contain the proper ARIA attributes for accessibility?

When Reviewing CSS:

• Does the code follow agreed-upon standards and conventions?
• If you are using a CSS precompiler, such as LESS or SASS, are you nesting classes (or even worse, nesting IDs) because it looks more organized? Unnecessary nesting makes overriding CSS difficult; it also hurts performance and maintainability.
• Is CSS in modules namespaced to avoid name clashing? Consider a top-level wrapper class with the same name as the module. If you don’t want class nesting, consider using BEM convention.
• Are some class names so generic that name clashes are likely? This can happen with either other modules in the same app or with other libraries that you may use.
• Are regularly used CSS rules (or blocks of rules) repeated so often that they should become mixins?
• If the app uses a CSS library, such as Bootstrap or Foundation, are you maximizing its use? Does your app redefine mixins available in the library, or worse, does your app have blocks of CSS where using a single class from the library would do?
• Are padding and margin used correctly? Is padding used when margin should have been, or vice versa?
• Does using percentage instead of pixel make sense? How about rem vs. px?
• Are there duplicate rules for the same selector? Remove the duplicates.
• Are the same rules found for different selectors in the same module? Merge them.
• Are the same rules found across different modules? Put them in a shared CSS file.
• Are there too many nested classes? Are they necessary for override? Try removing some nesting. If the layout doesn’t change, they are not needed.
• Are IDs used for CSS? IDs are HIGHLY specific and need other IDs to override them. And if you must insist on using IDs for CSS, DO NOT nest IDs. First, nested IDs require the same number of nested IDs or more to override, which makes your CSS longer and more complicated. Second, and this relates to fundamental CSS principle — IDs are by definition unique. If an ID needs to be nested to work, it’s not unique. It should not be an ID. Make it a class instead.

When Reviewing JS:

• Does the code follow agreed-upon standards and conventions?
• Is there JSDoc or similar documentation, especially for exposed methods?
• If something is to be fixed/enhanced later, is there a TODO with a developer’s name next to it?
• Is a solution either too clever or too complicated? Is there a slightly longer but simpler and more maintainable solution? If you have to read the code (or its comments) several times to understand what it is trying to do, it is probably too complicated and should be simplified.
• Does each method do one specific task only? If not, break it up into smaller, focused tasks. It will help in reusability and unit testing.
• Is a method particular to one use case, with hard-coded values and hard-coded CSS selectors, and so on? Or is it generic and reusable, and takes parameters? Generic methods are scalable and easier to maintain, AND they are much easier to unit test.
• Does a block of code look familiar from elsewhere? If an existing util method does the same, use it. If there is a util method that is related but slightly different, update it. If the util does not exist, create it. Then, use that util and remove identical and similar blocks globally.
• If an app already uses a utility library such as Underscore or Lodash, is the PR introducing another similar library? Discuss with your team which library best suits your app.
• Is the app loading a heavy library when you use only a few methods in it? Is there a lighter alternative? Can you pull out just what you need?
• Does a function take too many parameters? If there are more than 4 (or some number set by your team), use an object to avoid the hassle of parameter ordering and null values.
• Are values and data cached? Any string, object, etc., should be cached if it is used more than once.
• Are variable and function names so short that they are not meaningful at a glance? Longer names (be reasonable, of course) are more likely to be meaningful and self-documenting. Furthermore, searching globally for a longer name will return fewer results, which makes code-digging easier.
• If using third-party libraries, is the app using deprecated methods? Use the latest APIs as recommended by the libraries.
• Are all console logs and debuggers removed?
• Are listeners removed before being re-added? Does the handler run multiple times on the same event on the same element? One way to check is by printing logs in the handler.
• Pay attention to elements (usually inputs) that are listening to multiple events, such as keydown, keypress, and keyup. When one user action causes all events to fire, such that the handler is run once for each fired event, is the app still behaving as expected? Even if it is behaving correctly, does it make the app less responsive?
• If the app architecture is based on the modular pattern, is one module referencing or modifying DOM elements of an unrelated module, thus breaking the pattern?

A few notes on jQuery:

• Are all jQuery object variables prefixed with $ for quick identification?
• Are jQuery objects cached, if used more than once?
• Are jQuery calls chained?
• Are selectors cached in a string variable if used more than once? When no element is found for a selector and your code operates on this “missing” selector, jQuery fails silently, which is both GOOD and BAD at the same time. It is good because no error is thrown at run time; it is bad because the app may fail in other ways. A common reason is that the selector was changed and it was not updated globally. Cache the selector to minimize that risk.
• Is the code using jQuery event delegation to minimize number of listeners?
• How are you listening to events on elements that can be re-rendered? Via jQuery event delegation? That way, you don’t have to set up listeners again every time the element is re-rendered. It is easy to miss the re-add, especially when there are multiple render paths.
• Are you using deprecated methods? For example, use .on() instead of .bind()/.live().

Browser support

One difficulty in front-end development is getting a web app to work well across devices, operating systems, and browsers. With the large number of device, operating system, and browser combinations in the wild, including the old and the native, working well everywhere is an impossible task for developers. The reasonable approach is to get your app’s traffic breakdown and test the most popular combinations during development.

To review layout and browser quirks, an in-person code review is the most effective as the reviewer can play with the feature/fix while reviewing the code. (A very simple exercise, which is also very effective in finding layout bugs, is resizing the browser window.) If an in-person review is not feasible, attach screenshots of the app to the PR, showing the layouts in different forms:

• Desktop browser
• Tablet browser (both portrait and landscape mode)
• Phone browser (both portrait and landscape mode)