Developer Blog

Cheat Codes for Contravariance and Covariance

I used to have nightmares about understanding variance. I thought things would get better when someone showed me this explanation…

panda hiding in an oreo factory

(image from rice.edu)

…but afterwards the nightmares got worse, since now they included clowns and burning pandas. I stayed up for days.

The goal of this post is to help you become more familiar with variance so that it is easier to understand in code you are reading, and so that you can use it when appropriate. The goal is not to promote the usage of variance (or overusage). Variance is a tool that can be powerful, but keep in mind that most parameterized types you define will be invariant.

Note that if you already know what variance is, and you just want a quick reference to remind you how to tell the difference between co/contravariance, refer to the cheatsheet at the bottom of this post

Why do we even need variance?

Variance is how we determine if instances of parameterized types are subtypes or supertypes of one another. In a statically typed environment with subtyping and generics, this boils down to the compiler needing to determine when one type can be substituted for another type in an expression. For simple types, this is straightforward:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
trait Life

class Bacterium extends Life

trait Animal extends Life {
  def sound: String
}

class Dog(name: String, likesFrisbees: Boolean) extends Animal {
  val sound = "bark"
}

class Cat(name: String, likesHumans: Boolean) extends Animal {
  val sound = "meow"
}

def whatSoundDoesItMake(animal: Animal): String =
  animal.sound

A Dog is an Animal, and so is a Cat, so anything that expects an Animal can also take a Dog or Cat. This is a classic example of the Liskov substitution principle.

What are we really doing here though? We’re trying to determine when some type T can safely be substituted for a type U in an expression that expects a U. The expression that expects a U only cares that it gets a U for 2 reasons:

  • The expression calls methods on a U
  • The expression passes a U to another expression that expects a U

An expression that passes a U along eventually makes its way to an expression that does call methods on a U (or stores it for later method calling). So we’re left with only caring about whether something can be substited for another thing, because we want to make sure that it is safe to call methods on that thing (and by “safe” I mean “will never fail at runtime”). This is what we want our compiler to do: make sure that we never call the method something.sound on a type that does not have the method sound defined.

A wild variant appears

Looking at a type that has parameters, it is no longer obvious when substitution within an expression is allowed. In other words, if a function takes an argument of type ParametricType[T], is it safe to pass it a ParametricType[U]? This is what variance is all about.

Covariance

Container types are the best example of something that is covariant, so let’s look at an example:

1
2
3
4
5
6
7
8
9
10
11
12
val dogs: Seq[Dog] = Seq(
  new Dog("zoe", likesFrisbees = true),
  new Dog("james vermillion borivarge III", likesFrisbees = false)
)

val cats: Seq[Cat] = Seq(
  new Cat("cheesecake", likesHumans = true),
  new Cat("charlene", likesHumans = false)
)

def whatSoundsDoTheyMake(animals: Seq[Animal]): Seq[String] =
  animals map (_.sound)

Our method whatSoundsDoTheyMake expects a Seq[Animal], and it calls the method .sound on those animals. We know that all Animals have the method .sound defined on them, and we know that we are mapping over a list of Animals, so it’s totally OK to pass whatSoundsDoTheyMake a Seq[Dog] or a Seq[Cat].

Dog <: Animal implies Seq[Dog] <: Seq[Animal]

Notice where the method call on the animals actually happens. It doesn’t happen within the definition of Seq. Rather, it happens inside of a function that receives the Animal as an argument. Now consider what would happen if we tried to pass a Seq[Life] to whatSoundsDoTheyMake. First off, the compiler wouldn’t allow this because it’s unsafe: error: value sound is not a member of Life. If it were allowed though, then you could attempt to call bacterium.sound, even though the method doesn’t exist on that object. Note that in a dynamically typed language you could try to do this, but you’d get a runtime exception like TypeError: Object #<Bacterium> has no method 'sound'.

Interestingly, the real problem doesn’t occur within Seq; it occurs later on down the chain. The reason is that a generic type makes guarantees to other types and functions that it interacts with. Declaring a class as covariant on type T is equivalent to saying “if you call functions on me, and I provide you with an instance of my generic type T, you can be damn sure that every method you expect will be there”. When that guarantee goes away, all hell breaks loose.

Contravariance

Functions are the best example of contravariance (note that they’re only contravariant on their arguments, and they’re actually covariant on their result). For example:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
class Dachshund(
  name: String,
  likesFrisbees: Boolean,
  val weinerness: Double
) extends Dog(name, likesFrisbees)

def soundCuteness(animal: Animal): Double =
  -4.0/animal.sound.length

def weinerosity(dachshund: Dachshund): Double =
  dachshund.weinerness * 100.0

def isDogCuteEnough(dog: Dog, f: Dog => Double): Boolean =
  f(dog) >= 0.5

Should we be able to pass weinerosity as an argument to isDogCuteEnough? The answer is no, because the function isDogCuteEnough only guarantees that it can pass, at most specific, a Dog to the function f. When the function f expects something more specific than what isDogCuteEnough can provide, it could attempt to call a method that some Dogs don’t have (like .weinerness on a Greyhound, which is insane).

What about soundCuteness, can we pass that to isDogCuteEnough? In this case, the answer is yes, because even if isDogCuteEnough passes a Dog to soundCuteness, soundCuteness takes an Animal, so it can only call methods that all Dogs are guaranteed to have.

Dog <: Animal implies Function1[Animal, Double] <: Function1[Dog, Double]

A function that takes something less specific as an argument can be substituted in an expression that expects a function that takes a more specific argument.

Conclusion

Enforcing safety by following expression substitution rules for parameterized types is a complex but super useful tool. It constrains what we can do, but these are things that we shouldn’t do, because they can fail at runtime. Variance rules, and type safety in general, can be seen as a set of restrictions that force us to engineer solutions that are more robust and logically sound. It’s like how bones and muscles are a set of constraints that allow for extremely complex and functional motion. You’ll never find a boneless creature that can move like this:

extremely complex and functional motion

Cheatsheet

Here is how to determine if your type ParametricType[T] can/cannot be covariant/contravariant:

A type can be covariant when it does not call methods on the type that it is generic over. If the type needs to call methods on generic objects that are passed into it , it cannot be covariant.

Archetypal examples: Seq[+A], Option[+A], Future[+T]

A type can be contravariant when it does call methods on the type that it is generic over. If the type needs to return values of the type it is generic over, it cannot be contravariant.

Archetypal examples: Function1[-T1, +R], CanBuildFrom[-From, -Elem, +To], OutputChannel[-Msg]

Rest assured, the compiler will inform you when you break these rules:

1
2
3
4
5
6
7
8
9
10
11
trait T[+A] { def consumeA(a: A) = ??? }
// error: covariant type A occurs in contravariant position
//   in type A of value a
//       trait T[+A] { def consumeA(a: A) = ??? }
//                                  ^

trait T[-A] { def provideA: A = ??? }
// error: contravariant type A occurs in covariant position in
//  type => A of method provided
//       trait T[-A] { def provideA: A = ??? }
//                         ^

Write Modern Asynchronous Javascript Using Promises, Generators, and Coroutines

Over the years, “Callback Hell” has been cited as one of the most common anti-patterns in Javascript to manage concurrency. Just in case you’ve forgotten what that looks like, here is an example of verifying and processing a transaction in Express:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
app.post("/purchase", (req, res) => {
  user.findOne(req.body, (err, userData) => {
    if (err) return handleError(err);
    permissions.findAll(userData, (err2, permissions) => {
      if (err2) return handleError(err2);
      if (isAllowed(permissions)) {
        transaction.process(userData, (err3, confirmNum) => {
          if (err3) return handleError(err3);
          res.send("Your purchase was successful!");
        });
      }
    });
  });
});

Promises were supposed to save us…

I was told that promises would allow us Javascript developers to write asynchronous code as if it were synchronous by wrapping our async functions in a special object. In order to access the value of the Promise, we call either .then or .catch on the Promise object. But what happens when we try to refactor the above example using Promises?

1
2
3
4
5
6
7
8
9
10
11
12
13
// all asynchronous methods have been promisified
app.post("/purchase", (req, res) => {
  user.findOneAsync(req.body)
    .then( userData => permissions.findAllAsync(userData) )
    .then( permissions => {
      if (isAllowed(permissions)) {
        return transaction.processAsync(userData);
        // userData is not defined! It's not in the proper scope!
      }
    })
    .then( confirmNum => res.send("Your purchase was successful!") )
    .catch( err => handleError(err) )
});

Since each function inside of the callback has its own scope, we cannot access the user object inside of the second .then callback. So after a little digging, I couldn’t find an elegant solution, but I did find a frustrating one:

Just indent your promises so that they have proper scoping.

Indent my promises!? So its back to the Pyramid of Doom now?

1
2
3
4
5
6
7
8
9
10
11
12
13
app.post("/purchase", (req, res) => {
  user.findOneAsync(req.body)
    .then( userData => {
      return permissions
        .findAllAsync(userData)
        .then( permissions => {
          if (isAllowed(permissions)) {
            return transaction.processAsync(userData);
          }
        });
  }).then( confirmNum => res.send("Your purchase was successful!"))
    .catch( err => handleError(err) )
});

I would argue that the nested callback version looks cleaner and is easier to reason about than the nested promise version.

Async Await Will Save Us!

The async and await keywords will finally allow us to write our javascript code as though it is synchronous. Here is code written using those keywords coming in ES7:

1
2
3
4
5
6
7
8
app.post("/purchase", async function (req, res) {
  const userData = await user.findOneAsync(req.body);
  const permissions = await permissions.findAllAsync(userData);
  if (isAllowed(permissions)) {
    const confirmNum = await transaction.processAsync(userData);
    res.send("Your purchase was successful!")
  }
});

Unfortunately the majority of ES7 features including async/await have not been implemented in Javascript runtimes and therefore, require the use of a transpiler. However, you can write code that looks exactly like the code above using ES6 features that have been implemented in most modern browsers as well as Node version 4+.

The Dynamic Duo: Generators and Coroutines

Generators are a great metaprogramming tool. They can be used for things like lazy evaluation, iterating over memory intensive data sets and on demand data processing from multiple data sources using a library like RxJs. However, we wouldn’t want to use generators alone in production code because it forces us to reason about a process over time and each time we call next, we jump back to our generator like a GOTO statement. Coroutines understand this and remedy this situation by wrapping a generator and abstracting away all of the complexity.

The ES6 version using Coroutines

Coroutines allow us to use yield to execute our asynchronous functions line by line, making our code look synchronous. It’s important to note that I am using the Co library. Co’s coroutine will execute the generator immediately where as Bluebird’s coroutine will return a function that you must invoke to run the generator.

1
2
3
4
5
6
7
8
9
10
11
import co from 'co';
app.post("/purchase", (req, res) => {
  co(function* () {
    const person = yield user.findOneAsync(req.body);
    const permissions = yield permissions.findAllAsync(person);
    if (isAllowed(permissions)) {
      const confirmNum = yield transaction.processAsync(user);
      res.send("Your transaction was successful!")
    }
  }).catch(err => handleError(err))
});

If there is an error at any step in the generator, the coroutine will stop execution and return a rejected promise. Let’s establish some basic rules to using coroutines:

  • Any function to the right of a yield must return a Promise.
  • If you want to execute your code now, use co.
  • If you want to execute your code later, use co.wrap.
  • Make sure to chain a .catch at the end of your coroutine to handle errors. Otherwise, you should wrap your code in a try/catch block.
  • Bluebird’s Promise.coroutine is the equivalent to Co’s co.wrap and not co on it’s own.

What if I want to run multiple processes concurrently?

You can either use objects or arrays with the yield keyword and then destructure the result.

With the Co library:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
import co from 'co';
// with objects
co(function*() {
  const {user1, user2, user3} = yield {
    user1: user.findOneAsync({name: "Will"}),
    user2: user.findOneAsync({name: "Adam"}),
    user3: user.findOneAsync({name: "Ben"})
  };
).catch(err => handleError(err))

// with arrays
co(function*() {
  const [user1, user2, user3] = yield [
    user.findOneAsync({name: "Will"}),
    user.findOneAsync({name: "Adam"}),
    user.findOneAsync({name: "Ben"})
  ];
).catch(err => handleError(err))

With the Bluebird library:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
// with the Bluebird library
import {props, all, coroutine} from 'bluebird';

// with objects
coroutine(function*() {
  const {user1, user2, user3} = yield props({
    user1: user.findOneAsync({name: "Will"}),
    user2: user.findOneAsync({name: "Adam"}),
    user3: user.findOneAsync({name: "Ben"})
  });
)().catch(err => handleError(err))

// with arrays
coroutine(function*() {
  const [user1, user2, user3] = yield all([
    user.findOneAsync({name: "Will"}),
    user.findOneAsync({name: "Adam"}),
    user.findOneAsync({name: "Ben"})
  ]);
)().catch(err => handleError(err))

Libraries that you can use today:

Releasing Git Town

We are excited to release Git Town today! Git Town is an open source command-line tool that helps keep software development productive as project and team sizes scale. It provides a few additional high-level Git commands. Each command implements a typical step in common software development workflows.

Check out the screencast to get an overview of what these extra Git commands are and how to use them.

The problem: Software development doesn’t scale well

Many software team runs into a few typical scalability issues:

  • More developers means more frequent commits to the main branch. This makes feature branches outdate faster, which results in bigger, uglier merge conflicts when finally getting the LGTM and merging these feature branches into the main branch. Time spent resolving merge conflicts isn’t spent writing new features.
  • These merge conflicts increase the likelihood of breaking the main development branch. A broken main branch affects the productivity of the whole team.
  • Old branches are often not deleted and accumulate, making it harder to maintain an overview of where current development is happening, and where unmerged changes are.

These are only a few of the many headaches that diminish developer productivity and happiness as development teams grow. These issues are almost completely independent of the particular workflows and how/where the code is hosted.

The approach: Developer discipline

Fortunately, most of these issues can be addressed with more discipline:

  • Always update your local Git repo before cutting a new feature branch.
  • Synchronize all your feature branches several times per day with ongoing development from the rest of the team. This keeps merge conflicts small and easily resolvable.
  • Before merging a finished feature branch into the main development branch, update the main development branch and merge it into your feature branch. Doing so allows you to resolve any merge conflicts on the feature branch, and tests things before merging into the main branch. This keeps the main branch green.
  • Always remove all Git branches once you are done with them, from both your local machine as well as the shared team repository.

The solution: A tool that goes the extra mile for you

It is difficult to follow these practices consistently, because Git is an intentionally generic and low-level tool, designed to support many different ways of using it equally well. Git is a really wonderful foundation for robust and flexible source code management, but it does not provide direct high-level support for collaborative software development workflows. Using this low-level tool for high-level development workflows will therefore always be cumbersome and inefficient. For example:

  • creating a new, up-to-date feature branch in the middle of active development requires up to 6 individual Git commands.
  • updating a feature branch with the latest changes on the main branch requires up to 10 Git commands, even if there are no merge conflicts
  • properly merging a finished feature branch into the main development branch after getting the LGTM in the middle of working on something else requires up to 15 Git commands.

Keeping feature branches small and focused means more feature branches. Running all these commands on each feature branch every day easily leads to each developer having to run a ceremony of hundreds of Git commands each day!

While there are a number of tools like Git Flow that focus on supporting a particular Git branching model, there is currently no natural extension of the Git philosophy towards generic, robust, high-level teamwork support.

We are excited to release exactly that today: Git Town! It provides a number of additional high-level Git commands. Each command implements a typical step in most common team-based software development workflows (creating, synching, and shipping branches). Designed to be a natural extension to Git, Git Town feels as generic and powerful as Git, and supports many different ways of using it equally well. The screencast gives an overview of the different commands, and our tutorial a broader usage scenario.

The awesomeness: Made for everybody, as Open Source

Git Town is for beginners and experts alike. If you are new to Git, and just want it to stay out of your way and manage your code, let Git Town provide the Git expertise and do the legwork for you. If, on the other hand, you are a Git ninja, and want to use it in the most effective manner possible, let Git Town automate the repetitive parts of what you would type over and over, with no impact on your conventions, workflow, and ability to do things manually.

Git Town is open source, runs everywhere Git runs (it’s written in Bash), is configurable, robust, well documented, well tested, has proven itself on everything from small open source projects to large enterprise code bases here at Originate, and has an active and friendly developer community.

Please try it out, check out the screencast or the tutorial, let us know how we can improve it, tell your friends and coworkers about it, or help us build it by sending a pull request!

FAQ

Does this force me into any conventions for my branches or commits?
Not at all. Git Town doesn’t require or enforce any particular naming convention or branch setup, and works with a wide variety of Git branching models and workflows.

Which Git branching models are supported by Git Town?
Git Town is so generic that it supports all the branching models that we are aware of, for example GitHub Flow, GitLab Flow, Git Flow, and even committing straight into the master branch.

How is this different from Git Flow?
Git Flow is a Git extension that provides specific and opinionated support for the powerful Git branching model with the same name. It doesn’t care too much about how you keep your work in sync with the rest of the team.

Git Town doesn’t care much about which branching model you use. It makes you and your team more productive by keeping things in sync, and it keeps your Git repo clean.

It is possible (and encouraged) to use the two tools together.

Is it compatible with my other Git tools?
Yes, we try to be good citizens. If you run into any issues with your setup, please let us know!

Managing Data Classes With Ids

At Originate, we have worked on a number of medium- to large-scale Scala projects. One problem we continuously find ourselves tackling is how to represent the data in our system in a way that is compatible with the idea that sometimes that data comes from the client, and sometimes it comes from the database. Essentially, the problem boils down to: how do you store a model’s id alongside it’s data in a type-safe and meaningful way?

The Problem

Consider your user model

1
case class User(email: String, password: String)

When a new user signs up, they send a User to your system, which you then store in the database. At this point, a user now has an Id. Should the id be stored inside the User model? If so, it would have to be an Option[Id]. Perhaps, instead of storing optional ids in your user model, you prefer to just have two data classes. One case class to represent data received from the client, and one to represent data received from the database.

1
2
3
4
5
6
// option 1: duplicate data classes
case class User(id: Id, email: String, password: String)
case class UserData(email: String, password: String)

// option 2: optional ids
case class User(id: Option[Id], email: String, password: String)

Both options have their pros and cons. There is a 3rd option, which is the purpose of this blogpost, but let’s cover our bases first.

Duplicate Data Classes

One simple way to solve this problem is to consider that your system has two versions of your data: the version it receives from the client, and the version it receives from the database, an idea generalized by the CQRS design pattern.

Unfortunately, this adds a lot of overhead/boilerplate. Not only do you have to duplicate the amount of models you have in your system, you also have to make sure to reference the right version of that model in the right places. This can lead to a lot of confusion, not to mention the fact that with User and UserData, it’s not immediately clear what the difference is to someone new on the project.

The biggest issue here is that we lose the correlation between the two types of data. User does not know that it comes from UserData and vice versa. Even worse, if we have methods on that data, for example, something that gives us a “formatted name” for a user… either we need both User and UserData to inherit from the same trait, or we duplicate the method. Unfortunately, this pattern is clunky and annoying.

Optional Ids

We’ve tried this pattern on several large projects. On the one hand, it prevents us from having to duplicate all of our data classes, which is nice. The big problem with optional ids is that, most of the time, your data actually does have an id in it. Wrapping it with Option means that the rest of your system has to always consider that the value might not be there, even when it definitely should. This ends up producing a lot of code like this:

1
2
3
4
for {
  user <- UserService.first()
  userId <- user.id
} yield ...

It’s not the worst thing in the world, but when 80% of your code is dealing with users that have ids, it feels unnecessary. Also note that there is a hidden failure here. If your UserService.first() returns Some(user) but for whatever reason that user doesn’t have an Id, then it will look the same as if UserService.first() returned None. Programming around this is possible, but gets ugly if you use this pattern all over your codebase.

Additionally, lazy developers will say “I know that the id should be there, why can’t I just use user.id.get?”. Option#get is a slippery slope, if you make exceptions for it in your codebase, people will abuse it, and then you lose the safety of having Options in the first place. At that point you might as well not even use Option, because you are getting the Option version of an NPE and also dealing with the overhead of Option. If you have developers trying to sneak in .get, consider checking out Brian McKenna’s library: WartRemover.

Furthermore, the Optional Id pattern leads you to create a base class that all of your data classes inherit from.

1
2
3
trait BaseModel[T <: BaseModel[T]] { self: T =>
  val id: Option[Id]
}

This is so that you can create base service and data access traits that your layers inherit from. It’s worth noting that in this situation, you will likely want a method def withId(id: Id): T defined on BaseModel so that your base services/DAOs know how to promote a data class without an id (received from the client), to a data class that has an id (after being persisted). You’ll see in the next section that we can do away with all of this.

While this pattern works, and we have used it in production with success, the issue we run into is that the types and data don’t accurately reflect the concepts in our system. We want a way to say that ids are not optional when the data has been retrieved from the database, while also maintaining the relationship between data both in and outside of the database.

The Solution

After writing out the problem, and the other possible solutions, it starts to become clear that there is a better way. We know what we want:

  • There should be one class that represents a specific type of data, whether it’s from the client or the database.
  • We don’t want id’s to be optional. Data received from the database should represent that the id exists.
  • We don’t want values to ever be null.
  • Ideally we can minimize overhead (control flow, inheritance, typing overhead).

We introduce a class that contains an id and model data:

1
2
3
4
5
6
7
case class WithId[A](id: Id, model: A)

// receive data for a new user from the client
val user: User = Json.parse[User](json)

// receive data from the database
val user: WithId[User] = UserService.findByIdOrFail(userId)

This makes a lot of sense. We retain the fact that the only difference between client data and database data is that it has an id. We also avoid a good amount of overhead from the other two options (duplication, inheritance, and unnecessary complexity around ids). The main bit of overhead this introduces is extra typing when dealing with models in your service and DAO layer. While the types can get pretty nasty in some cases (our services are always asynchronous, so you may have Future[Seq[WithId[User]]]), it beats the alternatives.

Removing .model Calls

If the thought of having to do user.model.firstName feels ugly, there is a way around it using implicits:

1
2
3
object WithId {
  implicit def toModel[A](modelWithId: WithId[A]): A = modelWithId.model
}

Note that we have not tested this solution out on a large scale project, and it could add compilation time overhead.

Conclusion

Hopefully it is clear that while this is seemingly a small problem, finding the right way to model it in your system can have major implications on the cleanliness of your codebase. We have been trying the WithId pattern on a sizeable project for the last month with great results. No issues so far, and the type overhead isn’t that bad considering the additional safety it brings.

Android and CI and Gradle - a How-To

Updated 10/31/15: Since writing this, things have changed. As a result, some of the information below may be outdated and no longer applicable. For more up to date information on how to use CircleCI for your Android projects, please see the Continuous Integration section of our Android Stack Guidelines.

There are tech stacks in this world that make it dead simple to integrate a CI build system.
The Android platform is not one of them.

Although Gradle is getting better, it’s still a bit non-deterministic, and some of the fixes you’ll need will start to feel more like black magic than any sort of programming.

But fear not! It can be done!

Before we embark on our journey, you’ll need a few things to run locally:

  1. A (working) Gradle build
  2. Automated tests (JUnit, Espresso, etc.)

If you don’t have Gradle set up for your build system, it is highly recommend that you move your projects over. Android Studio has a built-in migration tool, and the Android Dev Tools website has an excellent guide on how to migrate over to the Gradle build system, whether you’re on Maven, Ant, or some unholy combination of all three.

A very general example of a build.gradle file follows:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
//build.gradle in /app
apply plugin: 'com.android.application'

buildscript {
  repositories {
    mavenCentral()
  }
  dependencies {
    classpath 'com.android.tools.build:gradle:1.0.1'
  }
}
//See note below
task wrapper(type: Wrapper) {
    gradleVersion = '2.1'
}

android {
  compileSdkVersion 19
  buildToolsVersion "21.1.2"

  defaultConfig {
    applicationId "com.example.originate"
    minSdkVersion 14
    targetSdkVersion 19
    versionCode 1
    versionName "1.0"

    testApplicationId "com.example.originate.tests"
    testInstrumentationRunner "android.test.InstrumentationTestRunner"
  }

  buildTypes {
    debug {
        debuggable true
    }
    release {
        debuggable false
    }
  }
}

dependencies {
  compile project(':libProject')
  compile com.android.support:support-v4:21.0.+
}

(NOTE: the Gradle Wrapper task isn’t strictly necessary, but a highly recommended way of ensuring you always know what version of Gradle you’re using – both for futureproofing and for regressions)

Check out the Android Developers website for some good explanations and samples.

Choose your weapon

At Originate, we are big fans of CircleCI. They sport a clean, easy-to-use interface and support more languages than you could possibly care about. Plus, they are free for open source Github projects!
(Other options include TravisCI, Jenkins, and Bamboo)

In this guide, we’ll be using CircleCI, but these instructions should translate readily to TravisCI as well.

Configure all the things!

In order to use CircleCI to build/test your Android library, there’s some configuration necessary. Below are some snippets of some of the basic configurations you might use. About half of this comes from the CircleCI docs and half of it comes from my blood, sweat, and tears.

At the end of this section, I’ll include a complete circle.yml file. (The complete docs for the circle.yml file is here)

Machine

First, the code:

1
2
3
4
5
machine:
  environment:
    ANDROID_HOME: /home/ubuntu/android
  java:
    version: oraclejdk6
  1. The setting of the ANDROID_HOME environment variable is necessary for the Android SDKs to function properly. It’ll also be useful for booting up the emulator in later steps.
  2. Although setting the JDK version isn’t strictly necessary, it’s nice to ensure that it doesn’t change behind-the-scenes and possibly surprise-bork your build.

Dependencies + Caching

1
2
3
4
5
6
dependencies:
  cache_directories:
    - ~/.android
    - ~/android
  override:
    - (source scripts/environmentSetup.sh && getAndroidSDK)
  1. By default, CircleCI will cache nothing. You might think this a non-issue right now, but you’ll reconsider when each build takes 10+ minutes to inform you that you dropped a semicolon in your log statement.

By caching `~/.android` and `~/android`, you can shave precious minutes off of your build time.
  1. Android provides us with a nifty command-line utility called…android (inventive!). We can use this in a little Bash script that we’ll write in just a second. For now, just know that scripts/environmentSetup.sh can be whatever you want, as can the Bash function getAndroidSDK.

Bash Scripts – a Jaunt into the CLI

Gradle is good at a lot of things, but it isn’t yet a complete build system. Sometimes, you just need some good ol’fashioned bash scripting.

In this section, we’ll download Android API 19 (Android 4.4 Jelly Bean) and create a hardware-accelerated Android AVD (Android Virtual Device – aka “emulator”) image.

Note: If android commands confuse/scare you, check out the d.android documentation.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
#!/bin/bash

# Fix the CircleCI path
function getAndroidSDK(){
  export PATH="$ANDROID_HOME/platform-tools:$ANDROID_HOME/tools:$PATH"

  DEPS="$ANDROID_HOME/installed-dependencies"

  if [ ! -e $DEPS ]; then
    cp -r /usr/local/android-sdk-linux $ANDROID_HOME &&
    echo y | android update sdk -u -a -t android-19 &&
    echo y | android update sdk -u -a -t platform-tools &&
    echo y | android update sdk -u -a -t build-tools-21.1.2 &&
    echo y | android update sdk -u -a -t sys-img-x86-android-19 &&
    #echo y | android update sdk -u -a -t addon-google_apis-google-19 &&
    echo no | android create avd -n testAVD -f -t android-19 --abi default/x86 &&
    touch $DEPS
  fi
}
  1. The export PATH line is to ensure we have access to all of the Android CLI tools we’ll need later in the script.
  2. The DEPS=... is used in the if/then block to determine if CircleCI has already provided us with cached dependencies. If so, there’s no need to download anything!
  3. Note that we’re explicitly requesting the x86 version of the Android 19 emulator image (sys-img-x86-android-19). The ARM-based emulator is notoriously slow, and we should use the hardware-accelerated version if at all possible.
  4. We create the Android Virtual Device (AVD) with the line android create avd ..., with a target of Android 19 and a name of testAVD.
  5. If you need the Google APIs (e.g., Maps, Play Store, etc.), you can uncomment out the line addon-google_apis-google-19.
  6. Even though Google has released an API 21 HAXM emulator, I still recommend using an API 19 AVD. API 21’s emulator doesn’t always play nice with CircleCI.

CAVEAT – Because of the way this caching works, if you ever change which version of Android you compile/run against, you need to click the “Rebuild & Clear Cache” button in CircleCI (or use the CircleCI API). If you don’t, you’ll never actually start compiling against the new SDK. You have been warned.

You shall not pass! (until your tests have run)

This section will vary greatly depending on your testing setup, so YMMV – moreso than with the rest of this post.
This section is assuming you’re using a plain vanilla Android JUnit test suite.

1
2
3
4
5
6
7
8
9
10
11
test:
  pre:
    - $ANDROID_HOME/tools/emulator -avd testAVD -no-skin -no-audio -no-window:
      background: true
    - (./gradlew assembleDebug):
      timeout: 1200
    - (./gradlew assembleDebugTest):
      timeout: 1200
    - (source scripts/environmentSetup.sh && waitForAVD)
  override:
    - (./gradlew connectedAndroidTest)
  1. The $ANDROID_HOME/tools/emulator starts a “headless” emulator – more specifically, the one we just created.
    1a. Running the emulator from the terminal is a blocking command. That’s why we are setting the background: true attribute on the emulator command. Without this, we would have to wait anywhere between 2-7 minutes for the emulator to start and THEN build the APK, etc. This way, we kick off the emulator and can get back to building.
  2. The two subsequent ./gradlew commands use the Gradle wrapper (gradle +wrapper) to build the code from your /app and androidTest directories, respectively.
  3. See below for environmentSetup.sh Part II. Essentially, after building both the app and the test suite, we cannot continue without the emulator being ready. And so we wait.
  4. Once the emulator is up and running, we run gradlew connectedAndroidTest, which, as its name suggests, runs the tests on the connected Android device. If you’re using Espresso or other test libraries, those commands would go here.
    4a. The CircleCI Android docs say that the “standard” way to run your tests is through ADB – ignore them. Gradle is the future and it elides all of those thorny problems that ADB tests have.

Bash Round 2

As mentioned above, after Gradle has finished building your app and test suite, you’ll kind of need the emulator to…y’know…run your tests.

This script relies on the currently-booting AVD’s init.svc.bootanim property, which essentially tells us whether the boot animation has finished. Sometimes, it seems like it’ll go on forever…


Android AVD Boot


*will the madness never stop?!*

This snippet can go in the same file as your previous bash script – in that case, you only need one #!/bin/bash – at the top of your file.

1
2
3
4
5
6
7
8
9
10
11
12
13
#!/bin/bash

function waitAVD {
    (
    local bootanim=""
    export PATH=$(dirname $(dirname $(which android)))/platform-tools:$PATH
    until [[ "$bootanim" =~ "stopped" ]]; do
      sleep 5
      bootanim=$(adb -e shell getprop init.svc.bootanim 2>&1)
      echo "emulator status=$bootanim"
    done
    )
}

Note: This script was adapted from this busy-wait script.

Results

By default, CircleCI will be fairly vague regarding your tests’ successes and/or failures. You’ll have to go hunting through the very chatty verbose Gradle loggings in order to determine exactly which tests failed. Fortunately, there’s a better way – thanks to Gradle!

When you run gradlew connectedAndroidTests, Gradle will create a folder called /build/outputs/reports/**testFolderName**/connected in whichever folder you have a build.gradle script in.

So, for example, if your repo was in ~/username/awesomerepo, with a local library in awesome_repo/lib and an app in /awesome_repo/app, the Gradle test artifacts should be in /awesome_repo/app/build/outputs/reports/**testFolderName**/connected.

In this directory, you’ll find a little website that Gradle has generated, showing you which test packages and specific tests passed/failed. If you like, you can tell CircleCI to grab this by placing the following at the top of your circle.yml file:

1
2
3
general:
  artifacts:
    -/home/ubuntu/**repo_name**/build/outputs/reports/**testFolderName**/connected

You can then peruse your overwhelming success under the Artifacts tab for your CircleCI build – just click on index.html. It should pull up something like this:


Example Artifact

Security, Signing, and Keystores

The astute among you will notice that I haven’t gone much into the process of signing an Android app. This is mainly for the reason that people trying to set up APK signing fall into 2 categories – Enterprise and Simple.

Enterprise: If you’re programming Android for a company, you probably have some protocol regarding where your keystores/passwords can and cannot live – so a general guide such as this won’t be much help for you. Sorry.

Simple: You’re not Enterprise, so your security protocol is probably a little more flexible – i.e., you feel moderately comfortable with checking your keystore files into your respository.

In either case, Google and StackOverflow are your friends.

My final word of advice is that CircleCI can encrypt things like keystore passphrases – stuff you might consider passing in plain-text in your buildscript files. Check out CircleCI’s Environment Variables doc.

Finally

Go into your CircleCI settings, add a hook for your Github repo, and then do a git push origin branchName. If the Gradle Gods have smiled upon you, Circle should detect your config files and start building and testing!

Depending on your test suite, tests can take as little as a few minutes or as much as a half-hour to run. Try not to slack off in the meanwhile, but rejoice in having some solid continuous integration!

Stay tuned for a future blog post about using CircleCI to automagically deploy to MavenCentral!

Flipping to the back of the book…

Below is the full circle.yml as well as environmentSetup.sh for your viewing/copying pleasure:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
# Build configuration file for Circle CI
# needs to be named `circle.yml` and should be in the top level dir of the repo

general:
  artifacts:
    -/home/ubuntu/**repo_name**/build/outputs/reports/**testFolderName**/connected

machine:
  environment:
    ANDROID_HOME: /home/ubuntu/android
  java:
    version: oraclejdk6

dependencies:
  cache_directories:
    - ~/.android
    - ~/android
  override:
    - (echo "Downloading Android SDK v19 now!")
    - (source scripts/environmentSetup.sh && getAndroidSDK)

test:
  pre:
    - $ANDROID_HOME/tools/emulator -avd testAVD -no-skin -no-audio -no-window:
      background: true
    - (./gradlew assembleDebug):
      timeout: 1200
    - (./gradlew assembleDebugTest):
      timeout: 1200
    - (source scripts/environmentSetup.sh && waitForAVD)
  override:
    - (echo "Running JUnit tests!")
    - (./gradlew connectedAndroidTest)

And the accompanying shell scripts:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
#!/bin/bash

# Fix the CircleCI path
function getAndroidSDK(){
  export PATH="$ANDROID_HOME/platform-tools:$ANDROID_HOME/tools:$PATH"

  DEPS="$ANDROID_HOME/installed-dependencies"

  if [ ! -e $DEPS ]; then
    cp -r /usr/local/android-sdk-linux $ANDROID_HOME &&
    echo y | android update sdk -u -a -t android-19 &&
    echo y | android update sdk -u -a -t platform-tools &&
    echo y | android update sdk -u -a -t build-tools-21.1.2 &&
    echo y | android update sdk -u -a -t sys-img-x86-android-19 &&
    #echo y | android update sdk -u -a -t addon-google_apis-google-18 &&
    echo no | android create avd -n testAVD -f -t android-19 --abi default/x86 &&
    touch $DEPS
  fi
}

function waitForAVD {
    (
    local bootanim=""
    export PATH=$(dirname $(dirname $(which android)))/platform-tools:$PATH
    until [[ "$bootanim" =~ "stopped" ]]; do
      sleep 5
      bootanim=$(adb -e shell getprop init.svc.bootanim 2>&1)
      echo "emulator status=$bootanim"
    done
    )
}

References

Refactoring Git Branches - Part II

This is a follow-up to Refactoring Git Branches – part I.

While coding on a feature we almost always stumble across areas of the code base that should be improved along the way.

If such an improvement is a larger change, I usually make a note to do it separately right after the feature I am currently working on. This helps me stay efficient by focusing on one task at a time, and prevents me from getting lost in concurrent changes happening at the same time.

Sometimes, however, it is just easier and faster to implement a small improvement like a syntax fix along the way, and so my feature branches can still end up implementing several independent changes. Reviewing such branches with multiple changes is harder than necessary, since it is much easier to talk about different topics separately than all at once.

Before submitting pull requests for such bloated feature branches, I usually do a git reset master (assuming master is the branch from which I cut my feature branch). This undoes all the git commit commands that I have done in my branch, and I am left with all changes from my feature branch uncommitted and unstaged.

Now it is easy to stage and commit only the lines that are actually related to my feature. I can do this using the git command line directly (through interactive staging of hunks), or through GUI tools like GitX or Tower. Then I commit unrelated changes into their own feature branches, even if they are small.

If my main feature depends on the peripheral changes made, I would first commit the peripheral changes into the current branch, then cut a new branch from the current branch and commit the actual feature into it. This way, the feature can use the peripheral changes, but both can be reviewed separately. If we have a remote repo set up, since we are changing history here, we need to do a git push -f on our old branch to overwrite the old commits on the remote repo.

This results in pull requests that are super focused on exactly one thing and nothing else, and are thereby much easier and faster to review.

This technique obviously only works for private feature branches. Never change Git history that other people use!

iOS Checklists - Creating and Submitting Your App

Over the last seven years, our team at Originate has created countless iOS apps. Along the way we have continuously fine-tuned our iOS development process, creating best practices that we apply to every new app we build.

We’ve put some of this down on paper in the form of two checklists, one used when starting an iOS project, and one used when preparing to submit to the App Store.

Following these checklists has helped our team work together more efficiently, architect better solutions, reduce development time, and reduce the risks that come with publishing an app to the App Store.

We hope these checklists will do the same for you.

Starting an iOS project

  • Repo/GitHub

    1. Start by creating a new repo in GitHub and adding the appropriate iOS gitignore file.

    2. Be sure to follow the git-flow workflow, with master holding your App Store builds, dev your latest development code, and feature branches holding the current work in progress.

  • Xcode

    1. Make sure everyone in the team is on the same version of Xcode.

    2. Turn on “Analyze during build” and “Treat Warnings as Errors” (Set it under “Build Settings” in your application target).

    3. Turn off tabs and use spaces: XCode > Preferences > Text Editing > Indentation > Prefer Indent using Spaces, Tab width 2, Indent width 2.

  • Jenkins/OS X Server/TestFlight

    Setup CI/CD (Continuous Integration/Continuous Deployment) making sure with every push to dev all tests are run and an Ad Hoc build is created/emailed to the team (with the commit logs in the email body). If the build or tests failed, a failure email should instead be sent out to the team. At Originate most of our iOS projects use OS X Server integrated with TestFlight.

  • Coding Style/Standards

    1. Follow Apple’s recommended iOS coding style guide.

    2. In addition, follow recommendations here: http://qualitycoding.org/preprocessor/

    3. Keep your .h files short and simple, exposing only what is needed by other classes. Move all other methods, properties, instance variables declarations, etc. inside the .m file.

    4. Name your view controllers based on the page they are displaying (e.g. LoginViewController).

    5. Organize your project navigator and files using groups. Good group names are Data Models, Views, Controllers, App Delegate, Supporting Files, Tools, etc. Messy project navigators should not be accepted.

    6. Before submitting a pull request, go over the checklist at the bottom of our effective code review blog post and look for red flags.

  • Architecture

    1. MVC (Model View Controller) is sometimes jokingly called Massive View Controller in iOS development. These massive controllers that do everything are common mistakes for beginners. They are not acceptable. As needed, split out table related delegates/data sources from the view controller into their own separate classes. Split out views (especially if they are reused) into their own view classes. Make sure helper methods are pushed out into helper classes. In addition, the View Controller should not make any calls to the server, instead the Model or a manager class should handle this.

    2. Some good sample code/tutorials/patterns: Lighter View Controllers, Viper and BrowseOverflow (from the iOS TDD book).

  • Views/Nibs/Storyboards

    1. Be sure to use constraints/autolayout for your views and support different screen sizes. Otherwise you will have to manually configure the frame sizes/positions for each view to make sure it fits correctly for each screen size. PureLayout and FLKAutoLayout have been used on some projects at Originate.

    2. Determine if Nib files will be used. Originate recommends avoiding them, but leaves the decision to the Tech Lead. Storyboards are not used as they they are hard to manage with multiple developers (e.g. trying to merge a storyboard), slow down Xcode, and add a level of complexity to the code that is not necessary.

    3. Use FrameAccessor to modify frames if needed. This will make it very easy to get and set a UIView’s size and origin.

  • Fonts and Colors

    Standardizing fonts and colors throughout the app (i.e. create helper classes) so that it’s easy to maintain/modify them as needed. This also makes for cleaner code that doesn’t have random RGB or font strings scattered in the views.

  • Display text

    1. All strings displayed to the user must be placed in a localization file.

    2. Avoid using image assets that contain text, use a UILabel instead, pulling the text out of the localization file.

  • Analytics

    http://replay.io is our go-to platform for analytics at Originate.

  • Crash reporting

    Although Apple and iTunes Connect are making progress here, it’s still best to use a third-party tool like Crashlytics. They work much faster and have a better UI/reporting interface.

  • Add AOP support if needed (e.g. for logging).

  • Third party code dependencies

    Cocoapods can be used to maintain third-party dependencies. At Originate this decision is left to the Tech Lead.

  • Server communication

    1. Add the ability to toggle between server environments (e.g. QA, dev, staging, etc.) in app settings.

    2. If needed, implement an app update notification system, where the server can inform the app a newer version is available and the app can display the appropriate message to the user.

    3. Make sure an activity indicator is displayed during server calls. MRProgress has been used on some projects at Originate.

    4. AFNetworking or RestKit (if Core Data is needed) should be used for network communication. See our AFNetworking cache blog post for optimizations/configuration.

    5. For debugging, make sure all server communications (i.e. web service requests/responses) are printed out to the console.

    NOTE: Use DLog to log all requests and response (in debug mode) to the console:

1
2
3
4
5
6
  #ifdef DEBUG
  #define DLog(fmt, ...) NSLog((@"%s [Line %d] " fmt), __PRETTY_FUNCTION__, __LINE__, ##__VA_ARGS__);
  #else
  #define DLog(...);
  #endif
  
  • Polish

    Small tweaks to an app can go a long way. Make sure to include simple user expected features like pull-to-refresh, tap status bar for scroll to top, activity indicator during server calls, etc. to make the app user friendly. Your Design and Product team should be on top of this already. The iOS Human Interface Guidelines is a good reference doc for them.

Lastly, ensure that the process to create the needed accounts (e.g. iTunes Connect, Urban Airship, Analytics Accounts, etc.) is started.

Submitting to the App Store

Releasing to the App Store must not be taken lightly. Steps must be taken to ensure the submitted app follows Apple’s guidelines and will work once published to the store. Four to eight hours should be allocated to perform this task.

When your app has been tested and is ready to release, follow these steps:

  • Apple Guidelines

    1. At a high level, Apple goes over everything related to App testing, Ad Hoc distribution, and App Store release in the App Distribution Guide. This document should be reviewed.

    2. And of course, most importantly, these documents should be memorized! App Store Review Guidelines and Common App Rejections.

    NOTE: Although not listed in the rejections link above, we’ve found that often developers forget to add content flagging for apps that create user generated data. Apple rejects these apps until content flagging is added.

  • Core Data

    If you are submitting an updated app that uses Core Data, then you must make sure to write a migration script in order to properly propagate changes to the DB. If this is not done, then it’s possible that the app can crash once updated and the user will have to delete and reinstall the app. See Core Data Model Versioning and Data Migration.

  • App Review Screen

    When you want your app to be reviewed by the user, UAAppReviewManager makes sure the review is done at the right time.

  • Release Scheme

    In the “Edit Scheme” section of Xcode, “Archive” should be set to “Release”. Release builds must hide log statements and turn off any test code/test screens used for development.

    NOTE: Due to compiler optimizations, release builds can sometimes function differently than debug builds. It is best to start testing release builds a few days before the release to the App Store in order to capture any possible issues.

  • Server

    Point your release build at the proper production servers (and use HTTPS)

  • Release Candidate Ad Hoc builds / App Store build

    Make sure to use the correct Bundle Identifier, Apple ID, certificates (e.g. Push Certificates), etc. for the release candidate and App Store builds.

  • Test Release Candidate configuration

    1. Make sure the app is communicating with the correct production servers (using HTTPS).

    2. Make sure all test screens are hidden.

    3. Make sure no sensitive data is printed to the console.

    4. Make sure Analytics is working with the correct production account.

    5. Make sure services such as Urban Airship are working with the correct production account.

  • Push to master

    1. Update the bundle version number in the app plist file. This number does not have to match the version number you assign to the app on the App Store, but it’s best that it does.

    2. Push the code that is ready for release to the master branch and tag it.

  • Setup the app in iTunes Connect

    1. In iTunes Connect create a new App, or if this is an update, select “Add Version”.

    2. Make sure to fill in all data and upload all necessary images (and video).

    NOTE: Only the fields “Description” and “What’s New In This Version” can be modified once the app is accepted and placed in the store. Other important fields like images cannot be modified.

  • Submit

    Use Xcode to create the App Store build and upload to iTunes Connect.

User-level Feature Specs With Cucumber

Summary

Applications are not just collections of technology. They are designed to provide meaningful functionality within the user’s domain of experience. To achieve that, they encapsulate complex technical implementations under intuitive, human-friendly user interfaces.

Congruent to that, the specifications for said application functionality should also be on the level of user experience, with their underlying technical implementation encapsulated.

Cucumber is often misunderstood as an unnecessary detour from expressing feature specs more directly in code. In this blog post I demonstrate that Cucumber’s code and language patterns emerge naturally when organizing/refactoring complex feature specs.

This substantiates the understanding of Cucumber as a set of patterns, tools, and programming languages specialized for expressing feature specs on the same semantic level as the functionality they describe, the level of user experience.

Introduction

An essential part of TDD are feature specifications (aka functional or integration tests). To verify that our application as a whole works, we fire up the complete application stack as well as a scriptable interaction device (browser or mobile application simulator). Using the latter we simulate users interacting with our app (clicking links and buttons, filling out forms etc) and check that our application as a black box exhibits the correct behaviors (displays the correct responses, sends the right messages to other apps etc). These feature specs can even drive the development of their features.

For simple feature specs we often don’t need anything beyond a fixture and mocking library together with a UI driver. As feature specs grow in size, however, expressing complex user interactions solely using only these intentionally low-level tools becomes increasingly cumbersome. Here is a representative example: the feature spec for changing the password of a user account in a typical web application. We use Ruby, RSpec, Capybara and Factory Girl.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
scenario 'changing the password' do
  user = create :user, email: 'tester@foo.com', password: 'old password'
  click_link 'Login'
  fill_in 'Email', with: 'tester@foo.com'
  fill_in 'Password', with: 'old password'
  click_button 'Log In'
  expect(page).to have_content 'Signed in successfully'
  visit '/'
  click_link 'my account'
  click_link 'change my password'
  fill_in 'New Password', with: 'new password'
  fill_in 'Password Confirmation', with: 'new password'
  click_button 'Update password'
  expect(page).to have_content 'Changed your password'
  click_link 'Log Out'
  click_link 'Login'
  fill_in 'Email', with: 'tester@foo.com'
  fill_in 'Password', with: 'new password'
  click_button 'Log In'
  expect(page).to have_content 'Signed in successfully'
  click_link 'Log Out'
  click_link 'Login'
  fill_in 'Email', with: 'tester@foo.com'
  fill_in 'Password', with: 'old password'
  click_button 'Log In'
  expect(page).to have_content 'Login not successful, please try again'
end

Did you understand what this quite massive and cumbersome spec verifies? How does changing the password work? How long did it take you to understand all that? How much low-level source code did you have to read, parse, and execute in a virtual browser in your head in order to derive how the application is supposed to behave here? And that was still a relatively small, simple, and straightforward feature!

Although the spec nicely lists all the individual steps for changing a user’s password, it is too low-level. It is hard to see how the product actually works, and I am not confident from just looking at this that we didn’t forget to check something. This is merely what a developer thought the product should do, expressed in ways only a developer understands. But like all people, developers occasionally misunderstand requirements, or translate them incorrectly into code.

Commented groups

As a start, let’s group related steps together and add some comments.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
scenario 'changing my password' do

  # Create a user with email "tester@foo.com" and password "old password"
  user = create :user, email: 'tester@foo.com', password: 'old password'

  # Log in as that user
  visit '/'
  click_link 'Login'
  fill_in 'Email', with: 'tester@foo.com'
  fill_in 'Password', with: 'old password'
  click_button 'Log In'
  expect(page).to have_content 'Signed in successfully.'

  # Change that user's password to "new password"
  visit '/'
  click_link 'my account'
  click_link 'change my password'
  fill_in 'New Password', with: 'new password'
  fill_in 'Password Confirmation', with: 'new password'
  click_button 'Update password'
  expect(page).to have_content 'Changed your password'

  # Verify that we can log in with the new credentials
  click_link 'Logout'
  click_link 'Login'
  fill_in 'Email', with: 'tester@foo.com'
  fill_in 'Password', with: 'new password'
  click_button 'Log In'
  expect(page).to have_content 'Signed in successfully.'

  # Verify that the old password doesn't work anymore
  click_link 'Logout'
  click_link 'Login'
  fill_in 'Email', with: 'tester@foo.com'
  fill_in 'Password', with: 'old password'
  click_button 'Log In'
  expect(page).to have_content 'Login not successful, please try again'
end

Great, this has already made more clear what we actually do here! But comments in front of blocks of code are an indicator that a method does too much (more than one thing), and that new methods want to emerge here. Also, this method is too long, and this code is not reusable. For example when testing other scenarios, we don’t want to duplicate the code for logging in.

Extracting reusable methods

Lets extract reusable methods. Doing so also gives us a chance to remove a now unnecessary comment, because the respective code piece is now self-describing.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
scenario 'changing my password' do

  # Create a user with email "tester@foo.com" and password "old password"
  user = create :user email: 'tester@foo.com', password: 'old password'

  # Log in as that user
  login_with email: 'tester@foo.com', password: 'old password'
  verify_login_succeeded

  change_my_password_to 'new password'

  # Verify that the new password works
  logout
  login_with email: 'tester@foo.com', password: 'new password'
  verify_login_succeeded

  # Verify that the old password doesn't work anymore
  logout
  login_with email: 'tester@foo.com', password: 'old password'
  verify_login_failed
end


# Helper methods

def change_my_password_to new_password
  visit '/'
  click_link 'my account'
  click_link 'change my password'
  fill_in 'New Password', with: new_password
  fill_in 'Password Confirmation', with: new_password
  click_button 'Update password'
  expect(page).to have_content 'Changed your password'
end

def login_with(email:, password:)
  click_link 'Login'
  fill_in 'Email', with: email
  fill_in 'Password', with: password
  click_button 'Log In'
end

def logout
  click_link 'Logout'
end

def verify_login_succeeded
  expect(page).to have_content 'Signed in successfully.'
end

def verify_login_failed
  expect(page).to have_content 'Login not successful, please try again'
end

The scenario is now more concise and reads better. And the extracted methods make sense. But it feels like we aren’t quite there yet, and there is more we can do here.

I bet most of my feature specs have to create a user and then log in as that user. Let’s combine those steps into one.

Also, our spec contains two separate levels of abstraction now: comments describe higher-level end-user perspective, i.e. what people want to do with the product, and the corresponding code blocks represent the respective technical implementation, i.e. how to do these things. Our current feature spec mixes these levels inconsistently:

  • Comments and methods like change_my_password_to are on the high-level end-user perspective.
  • Code like create :user is on the technical implementation level.
  • Methods like login_with are in between: they already encapsulate pieces of end-user interaction, but need to be combined with other steps to form full end-user interactions.

All of that smells bad, so let’s keep refactoring.

Separate product perspective from implementation

Let’s make it so that our scenario solely describes the high-level end-user perspective, and all of the technical implementation is encapsulated in helper methods.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
scenario 'changing my password' do
  create_user_and_login_with email: 'tester@foo.com', password: 'old password'
  change_my_password_to 'new password'
  verify_login_works email: 'tester@foo.com', password: 'new password'
  verify_login_fails email: 'tester@foo.com', password: 'old password'
end


# High-level product-perspective methods

def change_my_password_to new_password
  visit '/'
  click_link 'my account'
  click_link 'change my password'
  fill_in 'New Password', with: new_password
  fill_in 'Password Confirmation', with: new_password
  click_button 'Update password'
  expect(page).to have_content 'Changed your password'
end

def create_user_and_login_with(*args)
  create :user, *args
  login_with *args
  verify_login_succeeded
end

def verify_login_fails(*args)
  logout
  login_with *args
  verify_login_failed
end

def verify_login_works(*args)
  logout
  login_with *args
  verify_login_succeeded
end


# Reusable building blocks

def login_with(email:, password:)
  click_link 'Login'
  fill_in 'Email', with: email
  fill_in 'Password', with: password
end

def logout
  click_link 'Logout'
end

def verify_login_succeeded
  expect(page).to have_content 'Signed in successfully.'
end

def verify_login_failed
  expect(page).to have_content 'Login not successful, please try again'
end

Some parts of our scenario try to sound a bit too much like English for being actual method names. They are too long. This isn’t well-factored, technically sound source code. We shouldn’t start naming our methods like that in the rest of the code base.

And it’s still doesn’t really come together. It doesn’t form a cohesive user story that makes me wave my credit card and say “Yes, I want that!” It’s not clear why we do all these steps, and what we are actually testing here. That creating users works? That passwords can be changed? That logging in still works after a password has been changed?

Part of that is because such concepts have to be explained, but this is still nowhere near real, intuitive English. Trying to make a general-purpose programming language sound like a natural language only gets us so far. In my experience it will always feel like putting lipstick on a robot, and there is no good solution here.

Describing the product part in plain English

Ultimately it is questionable whether a general-purpose programming language is the most appropriate tool here altogether. Feature specs don’t contain complex algorithms, loops, code paths, or inheritance. They don’t even require functions or variables per se. Feature specs just express a number of linear user interactions with an application, expressed from a non-technical human perspective.

We only described our scenario in code because its underlying implementation is technical, and as developers code is our hammer. But not everything requires code. Let’s try something more close to natural language: Gherkin

1
2
3
4
5
Scenario: changing my password
  Given I am a user with email: "tester@foo.com" and password: "old password"
  When I change my password to "new password"
  Then I can log in with email: "tester@foo.com" and password: "new password"
  And I can no longer log in with email: "tester@foo.com" and password: "old password"

Wow, that feels like a breath of fresh air. We expressed our interactions with the application in perfect English. For the first time, it’s absolutely clear what we are actually doing and verifying here, and why.

Gherkin is part of Cucumber. Let’s see how the corresponding step definitions look. If you wonder about Kappamaki below, it converts textual lists into collections.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
Given /^I am a user with (.+)$/ do |user_data|
  user_data = Kappamaki.attributes_from_sentence user_data
  create :user, user_data
  login_with user_data
  verify_login_succeeded
end

When /^I change my password to "(.*)"$/ do |new_password|
  visit '/'
  click_link 'my account'
  click_link 'change my password'
  fill_in 'New Password', with: new_password
  fill_in 'Password Confirmation', with: new_password
  click_button 'Update password'
  expect(page).to have_content 'Changed your password'
end

Then /^I can log in with (.+)$/ do |login_data|
  logout
  login_with Kappamaki.attributes_from_sentence(login_data)
  verify_login_works
end

Then /^I can no longer log in with (.+)$/ do |login_data|
  logout
  login_with Kappamaki.attributes_from_sentence(login_data)
  verify_login_failed
end

These are the same high-level product-perspective methods we had before, just with more descriptive English names. The bodies are almost identical to the ones written in Ruby. The reusable helper files don’t change at all.

As we can see, Cucumber provides facilities to represent the abstractions that naturally emerge in well-factored, complex feature specs. And it allows to represent them in a more appropriate format than a general-purpose programming language can. Other advantages are:

  • Product experts can verify that feature specs describe the correct application behavior, resulting in better team play between the product and development departments.

  • User stories can be written directly in Gherkin. This means one less conversion step from product description to code, which means one less opportunity for things to get lost in translation. And less meetings.

  • Feature specs can be understood and executed by both machines and humans. Automation allows to catch bugs and regressions earlier, thereby making everybody’s life easier. Knowing that this happens, Quality Assurance (QA) personnel no longer have to do the boring and repetitive task of re-verifying already-tested functionality, but can instead focus on finding new issues and ensuring that the product looks correct.

I hope it becomes more clear that Cucumber as a platform for intuitive, user-level feature specifications provides value to the entire agile organization, including the development team. It allows for better functional testing than general-purpose programming languages, and should be a part of most serious agile projects.

Robust and mature Cucumber implementations are available for Ruby, JavaScript, the JVM, Python, .NET, and many other platforms. You can even develop cross platform Android and iOS specs with it.

No more low-level Gherkin that merely wraps individual interaction steps. That’s what Capybara is for. Cucumber is a high-level specification layer with end-user perspective, on top of the underlying technical implementation.

The future is green, friends!

How to Deploy Play Framework on AWS OpsWorks

AWS OpsWorks is the next evolution of Amazon’s auto scaling, cloud deployment solution Elastic Beanstalk. OpsWorks attempts to abstract things a bit further, giving you more control over the various pieces of your deployment, see a tutorial here.

OpsWork has a pretty steep learning curve. To help with that, pre-made layers for various kinds of web apps are available (Rails, Node.js, PHP, Java, Static). This post will show you how to setup a Play! Framework app on OpsWorks in just a few steps.

I – Create Your Deployment Cookbook

An OpsWorks app uses Chef cookbooks to provision instances and deploy applications. For this example we’ll create a public git repository with a copy of Originate’s Play Framework cookbook. Creating a separate repository for your app’s cookbooks makes it easier to work with OpsWorks and also allows you to customize things as needed down the road.

Create a custom cookbook

First, install Berkshelf, which is available through ChefDK. Berkshelf makes it easier to manage cookbooks.

1
2
3
4
mkdir play-sample-opsworks
cd play-sample-opsworks
git init
berks cookbook app --skip-git # this creates the cookbook for our play application

This creates the basic structure of the cookbook. Next we need to add a reference to Originate’s play2 cookbook in the Berksfile:

1
2
3
4
5
6
7
# app/Berksfile

source "https://supermarket.getchef.com"

metadata

cookbook "play2", git: "https://github.com/Originate/cookbooks.git", rel: "play2"

You also need to add that reference to metadata.rb so OpsWorks can find the play2 cookbook:

1
2
3
4
5
6
7
8
9
10
11
# app/metadata.rb

name             'app'
maintainer       'YOUR NAME'
maintainer_email 'YOUR EMAIL'
license          'All rights reserved'
description      'Installs/Configures app'
long_description 'Installs/Configures app'
version          '0.1.0'

depends "play2", "~> 0.1.0" # add a dependency on Originate's play2 cookbook

One caveat of the OpsWorks Chef integration is that it doesn’t automatically fetch dependencies. Berkshelf can solve this for us by grabbing dependencies and vendoring them into our repository. Note that Berkshelf nukes the directory you point it at (including the .git folder), so use the following to download dependencies to a temporary directory and move the files afterwards:

1
berks vendor /tmp/cookbooks -b app/Berksfile && cp -rf /tmp/cookbooks/* .

Next create the recipes for the OpsWorks setup and deploy phases:

1
2
3
4
5
6
7
8
# app/recipes/setup.rb
#
# Cookbook Name:: app
# Recipe:: setup
#

# This installs the play framework
include_recipe "play2::setup"
1
2
3
4
5
6
7
8
9
10
11
# app/recipes/deploy.rb
#
# Cookbook Name:: app
# Recipe:: deploy
#

# This deploys the application
opsworks_play2 do
  app "app"
  deploy_data node[:deploy][:app] # This data comes from OpsWorks' JSON blob
end

In the next section, we’ll use what you’ve done here to setup a running OpsWorks system. If you’re following along, commit your work and push it to a repository that OpsWorks can access. Alternatively, you can use this sample repository.

II – Configure AWS OpsWorks

Now that you have your cookbook for your app setup, head to the OpsWorks Home Page to setup and deploy your app.

Create a Stack

The first step is to create a new stack. A stack is composed of one or more layers. A layer represents a group of instances with a specific purpose. For example, OpsWorks comes with a load balancer layer, a database layer, and some common web application layers such as Rails or Node. OpsWorks doesn’t provide a pre-made layer for Play! applications, so make sure to select “Use custom Chef Cookbooks” and fill in the URL of the repository you created in part I. If you’re using a private repository, don’t forget to add the SSH key in the textbox.

Create AWS OpsWorks Stack

Next we need to tell the play2 cookbook to install version 2.1.3 of the Play! Framework and to bind the web server to port 80 instead of the default 9000. Add the following to the “Custom JSON” text box:

1
2
3
4
5
6
7
8
9
10
11
{
  "play2": {
    "version": "2.1.3",
    "http_port": 80,
    "conf": {
      "application": {
        "langs": "en"
      }
    }
  }
}

You can also add custom configuration for your application in the conf object. For additional information about which parameters can be tweaked, check out the README.

Create a Layer

Choose “Custom” as your layer type. This will represent the machines that run your Play application.

Create AWS OpsWorks Layer

Edit Settings to Call Custom Recipes

When you edit your layer, you can add custom recipes to the different steps of your deployment. In order to get up and running, add app::setup to the “Setup” step and app::deploy to the “Deploy” step.

In general, the format for adding cookbooks to a specific step of the deploy process is <cookbook_name>::<recipe_name>.

Edit Layer

Add the application

Add the sample application to your OpsWorks stack. Select “Other” for the application type, and use https://github.com/Bowbaq/sample-play-app.git for the repository URL.

Create Application

Add an Instance

The next step is to add an instance to the stack. This will provision and deploy an EC2 machine into a context completely managed by OpsWorks.

Add Instance

Newly added instances are not started by default. Click on start, and OpsWorks will run the setup recipes for you (which will install Play!) and then the deploy recipes (which will deploy the app).

Start App

Once OpsWorks finishes, you should be able to visit http://<instance public ip> and see the following:

Running Application

III – Profit

Now that you have your app setup on AWS OpsWorks, you can look forward to the following awesome features:

  • One click deploy
  • Automatic scaling based on various metrics
  • Integrated instance monitoring
  • Deployment tracking
  • Easy environment management
  • Easy Elastic Load Balancer integration

Conclusion

If you found this tutorial helpful and would like to contribute, feel free to fork the repository and submit pull requests. If you add any enhancements to your custom cookbooks that are worth sharing, contributions would be welcome. Additionally, I could use some help working on the enhancements listed here: https://github.com/Originate/cookbooks/issues.

Effective Code Reviews

Code reviews are a great way to improve code quality, catch bugs, and keep your whole team in sync with the latest changes. My favorite tool for doing a code review is still Github’s Pull Request feature, because of the easily-accessible diffs, inline comments, and commit-level and PR-level views of the file changes.

So, how do we make the code review process effective? I offer the following tips:

  • Actually review PR’s; telling an author to “just ship it” is detrimental.
  • Senior developers want you to review their code; don’t be scared.
  • Close PR’s that don’t have specs
  • If spec coverage is sparse, discuss it with the author (nicely).
  • Spec descriptions should accurately reflect the behavior of the module/method they describe.
  • Ask questions instead of making statements; the author likely put more thought into the code than you are aware of, and they’ve already solved the problem once.
  • Look for design problems:
    • Does this change solve a real problem? is it worth solving?
    • Given the constraints, is the overall approach correct and reasonable?
  • Look for code smells and suggest refactorings.
  • Ask for help with reviewing code that is outside your area of expertise.

Ever seen a “code review” that looks like this?

LGTM :ship:

This is not a helpful code review. Rather than communicating the message, “good job,” it tells the author that you can’t or won’t take the time and effort required to properly review their work.

If you’ve ever done this, you’re not alone, and I’m not going to yell at you—I’ve done it too.

I often hear people say they aren’t qualified to review some portion of a pull request; generally, this is because of a lack of familiarity with the language used, or of the frameworks. While this is an understandable view, I argue that you are, in fact, qualified to review the code. That’s true even if it’s not your favorite language, or you’re not particularly good at programming in that language, or you’re not familiar with the framework. How do you review code you’re not well-versed in?

Remember that code reviews are a service, not an insult.

Please, please, review my code! I think everyone who has a title like “Senior X”, “Director of X”, “X Architect,” or whatever else knows the importance of having someone look over their code, ask questions, and point out mistakes. You don’t need to be afraid of reviewing someone’s code, no matter their position. It’s ok. I am not my code. Neither is anyone else. I also still write bad code sometimes. So does everyone else. We all need peer reviews, and we are all peers here.

Moreover, if you are reviewing code from a senior developer, don’t be a yes-man! Point out mistakes you see, ask about why specs are written a certain way, ask about architecture decisions, point out weird styling issues… “Senior” people need help too. If someone gets mad at you for nicely pointing out oversights or for asking questions, that’s a sign of immaturity, not seniority.

Ensure that there is only one feature in the pull request

Each organization does things a bit differently, but ideally, your pull requests should correspond to exactly one user story. If you can achieve this level of granularity, you gain a number of benefits:

  • A feature spec can accompany the PR, and can be verified by your CI server.
  • The code will be easier to reason about.
  • If you use the nvie branching model (and you squash on merge), you can easily define different releases and cherry-pick features as you please.

Check that (nearly) all code has an accompanying specification.

If someone writes code without writing specs, they’ve introduced an unspecified, undocumented landmine into the code base. Your organization may have some policy about test-first, coverage level, etc.; I’m not going to get into that.

If you review code and there are no specs, talk to the author about it first. Discuss the importance of specifications, and offer to help them with writing some. I often suggest that the PR be closed and the code be rewritten using TDD.

Why do I recommend rewriting the code? Because if code was not driven by a spec, it is difficult to specify ex post facto. Bolting unit tests onto untested code results in brittle tests, encourages kludgy design, increases technical debt, and will make refactoring difficult in the future. If tests must be added on afterwards, they should be intergration tests.

I don’t say any of this to encourage dogmatic, or bully-like behavior. You and the author should discuss the specs, and their importance, and can come to a decision about the code together.

P.S.: Don’t close PR’s for purely cosmetic (i.e., only CSS/HTML) changes on this basis.

What about spikes? Aren’t they the exception?

Nope. Spikes are a really great tool, but their purpose is to provide a short-term proof-of-concept. They shouldn’t be merged; in fact, they shouldn’t even be reviewed. After a spike, should do a quick rewrite with specs.

Look for specs, and ask about the coverage level.

The first thing I do when I look at a PR is check the list of files changed (the github email digests are useful for this). If you see generic_modal.coffee was changed, then you should also see that generic_modal_spec.coffee (or something else that matches your naming scheme) was changed in the same PR.

Ideally, they should be changed in the same commit, or the spec should be changed first. This is not a rule, just a guideline; everyone commits a bit differently.

As mentioned above, keep an eye out for a feature spec (aka integration test). Optimally, there should be a one-to-one correspondence between a user story, a feature spec, and a pull request.

There should be relevant specs for all the code, both frontend and backend. If you don’t see something close to a 1-1 mapping between specs and source code, ask the author what’s going on. Some source code doesn’t need its own spec file: it may have been extracted from a spec’d module during a refactor; that’s ok.

If the author just neglected to write the specs, the PR should not be merged. This PR may be used as a reference for a spec-driven rewrite of the feature. If I see a PR without tests, I will close it. Specs are what give us the confidence we need to make changes later; we can’t afford to neglect them.

Read through the specs; they should make sense.

When you read through the specs, you should be able to quickly understand what each module does and what its public interface looks like. You should see relevant describe blocks, contexts, and it descriptions (or whatever your testing framework provides); they should describe, in English, what the module/API/UI does, and the code inside each should reasonably correspond to that description.

Ask the author to change non-obvious descriptions and tests; suggest a better description. For example:

1
2
3
4
describe '#fullName', ->
  it "works", ->
    person = new Person("John", "Doe")
    expect(person.fullName).to.equal("John Doe");

The above spec, “it works” makes no sense. The code inside is fine, but the description should be more like:

1
2
3
4
describe '#fullName', ->
  it "yields the full name of the person, space-separated", ->
    person = new Person("John", "Doe")
    expect(person.fullName).to.equal("John Doe");

If there is backend and frontend code, mentally separate them and review both.

In a web project, many times the backend and frontend components are written in different languages. When you’re only working on one part of the stack, it’s easy to just ignore code from the other parts. Instead, use PR’s as a learning opportunity; the other parts of the stack will affect the maintainability of the project just as much as the parts you’re working with, so make sure you look at both.

I find that it helps me to treat the backend and frontend code in a PR as if they were separate PR’s. It’s just a mental separation for me.

This works best when a pull request is for only one feature. The more features that are lumped together, the harder it is to review and, as a result, the largest PR’s end up with the worst reviews.

Be encouraging

The purpose of code reviews is to build up, not to tear down. That goes for both the code and the team. If you see something that was done well, say so! For example, “I like the way you were able to refactor that giant for-loop into a couple of map and reduce calls! I’ll have to keep an eye out for that in my own code!”

Everyone likes to receive positive feedback about their work. Plus, it will make you seem more appealing to work with, and it improves cohesiveness with the rest of the team.

Also, try adding some emoji, or some funny gifs to lighten the mood.

Have you tried asking questions, rather than making statements?

According to Dr. Albert Mehrabian, only 7% of our face-to-face communication is accounted for by the words we say. In the faceless, voiceless world of a text-based code review, it can be easy to offend someone, especially since people in our line of work tend to put so much care into their code. An easy way to diffuse this is to ask questions. For example, you see the following code:

1
rgba = hex.split(/\d{2}/).map (x) parseInt(x)

You could say,

This is wrong. Your split returns [“”, “”, “”], and parseInt needs a radix of 16.

Or, you could ask,

This doesn’t make sense to me; I think this will always yield [NaN, NaN, Nan]. Did I miss something?

Which would you rather someone post on your PR? I prefer the latter.

Asking questions is a powerful thing. When someone questions my code, sometimes that’s the first spark of thought about an alternative approach. Sometimes, it’s a gentle reminder that I left out a use case, or forgot to remove my debug lines, or left a swath of commented code.

My favorite is when someone new to my language of choice asks a question about my code. Sometimes it’s a chance for them to learn something new. Other times it’s a chance for me to break an age-old habit; you wouldn’t believe how many IE5/6 hacks I use out of habit…

If you’re not sure whether something is right, just ask the author. If it’s a mistake, they’ll recognize it and provide a fix. If it was intentional, they can justify whatever it is; you may even learn something.

Suggest refactoring in both the specs and the source.

Sometimes others point out code that I could refactor better; it’s easy to think I’m done refactoring when my head has been stuck in the same code for the last 6+ hours.

In the specs, point out where the testing tools could be better used. For example, I often forget about some of the features of a stubbing library. I sometimes write verbose, repetetive tests that could be extracted into a test helper. I also find that sometimes other developers can help make my tests less fragile. If you have an idea about improving the specs, share it! It may be helpful to add custom matchers to your spec suite; most tools support this in some fashion.

Do the same with the code! If you see where a specific refactoring can apply, say so. Look for code smells; you can refer to Cunningham & Cunningham’s wiki page on Code Smells.

We need to strike a balance here, too. The point of refactoring is to make the code easier to reason about, not to push indirection to its limits.

Ask for help.

It never hurts to have another pair of eyes on the code. Ask a senior person in your organization (or another open-source developer) to take a look; most will be glad to. If you know someone else with relevant experience, ask them! Most developers will gladly look over a PR with you.

If the PR contains a lot of code in a language you’re not great at, be proactive and find someone who is, and ask them to review with you! For example, if there’s a lot of CSS, and you’re not great at it, ask a frontend developer to help.

The Code Checklist

When it comes to most code (obviously this doesn’t apply to markup or styling code), try the following checklist:

  • Is every piece of code in the right place, i.e. model code in a model, controller logic in a controller, app-specific helper code in a helper, generic helper code in a library?
  • Do all classes have only one responsibility?
  • Do all methods do only one thing?
  • If a method has a side-effect, is it clear from the name, and otherwise documented?
  • Are all classes/methods/variables named properly so that the code is self-describing?
  • Is everything as private as possible, i.e. only the intended public interface is public?
  • Are too many things private? This could be an indication that you should extract an object.
  • Are all files within a reasonable size (e.g., less than 100 lines of code)?
  • Are all methods less than 10 lines of code?
  • No law of demeter violations (providing whole objects to methods when all that’s needed is the value of one attribute of them)?
  • Is everything tested? Is each thing tested enough?
  • Are there tests for private methods? This is a code smell.
  • Every class should have a small comment describing what it represents/does. Public methods should have comments describing what they are for, or when to call them, if this isn’t obvious from the code. Comments shouldn’t describe what the method does (this is visible from looking at the code).
  • Are there any obvious performance-stupidities, like making a database query for each loop iteration, rather than using a more optimized query that loads all data at once?
  • Spacing errors like no empty line between methods, or too many empty lines
  • There shouldn’t be any commented-out code.
  • There should be no debug statements like “console.log” or the likes.