Skip to content

Latest commit

 

History

History
159 lines (100 loc) · 8.54 KB

Coding.mkd

File metadata and controls

159 lines (100 loc) · 8.54 KB

Coding

Overview

Sprint.ly develops technology products. This requires us to write a lot of code. This document outlines how we approach technology and how we like to code.

General Principles

Codified standards over personal preference.

Syntatic arguments around code are a waste of time. Standards that can be validated with automated tools (such as linters) are preferential.

Tests as a driver for well factored code.

Code is tested because it prevents regressions, but also because it positively influences the design of software. Code that is painful to test is likely poorly factored. Bug fixes should always include appropriate regression tests. Ideally, failing tests should be added first to exercise the bug, with the accompanying patch making the test pass.

Code review as knowledge sharing and buy-in.

Code review happens for the following reasons:

  1. Enforce style. This should ideally be done automatically with linters.

  2. Communicate changes. In a changing code base, it's important that others have a sense of what is happening and how it's changing.

  3. Architectural changes. It is useful to have someone look over your code early on to question the structure of your change. This would ideally happen before a lot of time has been invested in the solution.

  4. Education. Each of us is knowledgeable in different aspects of delivering software. Code review is how we share this knowledge. As such, longer comments explaining a concept are encouraged when you feel they will be helpful. Taking the time to educate others is how we all learn.

Pragmatism over novelty.

Doing something because it is "neat" or "looks cool" is not an acceptable reason for something. This applies as well to code. While using Python's itertools (as an example) might feel "neat", that in itself is not a good reason.

Similarly, we do not encourage NIH or Not Invented Here syndrome in which a company must invent their own technology as others aren't good enough. As a startup, if we are to succeed, we must stand on the shoulders of those in open source who came before us.

Optimize for clarity, not brevity.

Legibility of code is paramount. As such, naming is important and should clearly identify intent. Prefer index over idx. This is not 2002 and vowels aren't a bad thing. One caveat to this rule is when it goes against community guidelines (such as using i, j, and k for loop variables).

Code comments play a large role when it comes to clarity. Commenting is necessary when coding in teams. Docstrings highlighting the function of a code block are encouraged. Line-level comments that explain what is happening are discouraged, preferring instead declarative functions. Line level comments explaining why something is happening are fine.

Take this example code:

def myfunc():
    ...
    # Find the last day of the month.
    first_day, last_day = calendar.monthrange(day.year, day.month)
    start = timezone.make_aware(datetime(year=day.year, month=day.month,
        day=day.day), timezone.utc)
    stop = timezone.make_aware(datetime(year=day.year, month=day.month,
        day=last_day), timezone.utc)
    ...

This would be clearer written as:

 def first_and_last_day_of_month(day):
      first_day, last_day = calendar.monthrange(day.year, day.month)
      start = timezone.make_aware(datetime(year=day.year, month=day.month,
          day=day.day), timezone.utc)
      stop = timezone.make_aware(datetime(year=day.year, month=day.month,
          day=last_day), timezone.utc)
      return start, stop


def myfunc():
    ...
    start, stop = first_and_last_day_of_month(day)
    ...

Okay use of inline comments:

for acct in to_deactivate:
    # Unfortunately, recurly won't handle our trial accounts, so we
    # must handle those ourself.
    acct.expire()

Automation over one-off

It is important that we all try to remove ourselves from the critical path as much as possible. As such, we must codify our knowledge in text (preferrably in the form of source code). The way we do this is automation. We seek automation of our infrastructure and deployment process such that it can be done by anyone for this very reason.

Tactical Guidelines

Sizing tickets

When it comes to sizing tickets, there are a few guidelines.

First, you should always size your tickets. It helps us get an understanding of how big something is. The exception to this rule is don't size stories if you're going to size the subitems. We prefer subitems to be sized.

  • The small estimation means "I know exactly what needs to happen, I just need to do it. It requires minimal thought."
  • The medium estimation means "I have a sense of what needs to happen. There's some ambiguity and I expect I'll probably hit a minor setback."
  • The large estimation means "This requires substantial thought on my part and the code involved is risky."
  • The xlarge estimation means "I don't actually know what this task entails."

Given those, never size tasks as XL. It should likely be a story with sized subitems instead.

Giving feedback to Code Review.

When giving feedback on a code review, the chief concerns are to end up with code that is well formed, well tested code that solves actual user problems is shipped to production.

This means that there's a balance to be achieved between ideal code and shipping useful things. Here are some ideas about what might trigger a discussion of refactoring.

New mechanism discovered since code was written.

One example is that we used to do repeated this.listenTo calls in backbone views, but realized that backbone offers a syntax for speeding up repeated calls. The fix is minor, but it's applied inconsistently through our codebase.

Second implementation of something you already know exists.

If you recognize a pattern of something we're doing, suggest combining implementations. This keeps common code paths from diverging. See also, DRY.

Difficult to understand code.

If code is difficult to understand during code review, it'll also be difficult to understand when there's a bug in it. Asking someone to clear things up to make it easier to read is a nice thing to do for people who come to the code later. Avoid cleverness.

Future product plans

If there are future product plans that are easier now than they will be later, feel free to make the suggestion to change it now.

Implementation strategy the other person might not know about.

If you know of a way of doing something that someone else might not know about, consider suggesting it if it adds value. If it doesn't add value, feel free to mention it for education reasons, but make it clear that a change isn't warranted.

Responding to Code Review.

Code review comments are up for debate unless otherwise said.

A few possible responses to a code review comment.

  1. Do the thing they ask. In 90% of all code review comments, the correct answer is to just do it.
  2. Push back on the change.
  3. This could be due to some knowledge you have that they don't. One example there is this PR where we were editing code that would only live a few weeks longer, so more investment in it didn't make a ton of sense.
  4. This could be because you disagree with the approach. Disagreement is okay. Make your case. If the other person feels strongly about it, talk it out in Slack.
  5. Seek an outside perspective. If available, pull in a more veteran person from the team.
  6. Answer the question they have. A lot of comments amount to "Why are we doing it like this?" or "Have you thought about how this affects this other component?". Answer honestly and make changes as necessary.

Overall, our goals should be to ship good code. As with all things, there is a balance and a learning process involved.

GitHub labels

To help expedite the code review process, we use a system of labels to get an at-a-glance view of open PRs.

  • "in review" implies that there was an ongoing discussion involved, but would be ready to be merged when those things were satisfied
  • "wip" means "work in progress". It's not ready for review. Sometimes PRs enter into this state if there are larger changes that need to happen.
  • "needs test" is for things that lack necessary test coverage for merge, but are otherwise fine.