DEV Community: Julio Olivera

Clean Code and Code Reviews

Julio Olivera — Tue, 28 Jan 2020 02:40:14 +0000

Dan Abramov recently published a blog post called "Goodbye, Clean Code", which looks back into an old work story and reflects on two things:

The pursuit of "clean" code just for the sake of it (and how it can actually backfire).
The team dynamics around that episode.

I'd like to focus on the second part because it's something I've been thinking about a lot.

To keep it brief (do read Dan's post, it's a short one and really drives the point home), the story is about a specific implementation by a coworker where Dan identifies duplication as something undesirable. He makes some changes, and then:

I checked in my refactoring to master and went to bed, proud of how I untangled my colleague’s messy code.

Lately, I've been thinking about code reviews and their purpose. I want to identify ways we can be better at them and, in order to do that, we need to understand why do we have a code review process in the first place. I like this story because it highlights a couple of places where code reviews might be particularly useful.

Quoting from the article again, something I wholeheartedly agree with:

A healthy engineering team is constantly building trust. Rewriting your teammate’s code without a discussion is a huge blow to your ability to effectively collaborate on a codebase together.

Even if the code review process might be skipped in some exceptional cases, it most likely won't be for pure refactoring work. This means that, just by following the process, a situation where one rewrites a teammate's code without any discussion won't happen.

Another aspect that's often mentioned as an advantage of going through a code review process is that of learning. Sometimes that learning implies learning something new - some new API, or a new approach or even design pattern that can help tackle a problem. In this case, the post reflects about how reducing duplication doesn't necessarily lead to objectively better code, and how it's instead a series of trade-offs. While it's possible that this wouldn't have come up during the review, if it did, it would've been a kind of learning where we question our own assumptions. Instead of only learning something new, we shed new light on something that we thought was clear. What initially might have been a routine comment, after being challenged becomes an opportunity to reevaluate and learn.

Your work is more than a diff

Julio Olivera — Mon, 29 Apr 2019 04:11:18 +0000

When working on a feature we don't think in terms of the files that need changing, line by line. We think at a higher level, more conceptually. On a typical MVC web application, a route is related to a specific controller's action, which in turn is related to some model performing the business logic. We think of all of these moving parts as a single "thing", just different parts in the same assembly line.

Yet when we send a pull request and someone has to review our code, what they see is a series of changes. File after file. There's a whole layer of meaning that existed on our minds only, and now the reviewer has to reverse engineer it from the output of git diff. Instead of being able to see the data flow of an API call, our reviewer will see a bunch of controller files' modifications. Then maybe the models, and they have to go back and forth to understand what's going on. Then maybe at the end they'll see the new routes.

The exact details are highly dependent on the project and on our design pattern of choice but whatever the case, looking at a series of modified files, line by line, doesn't convey the same ideas and thought-process that was going through our heads while coding. The underlying structure and design is not immediately obvious by looking at the changes alone. And while the code is the final form of those ideas, it's the design, the solution that we came up with at a conceptual level, what makes the real output of our work.

Based on that observation, I think as contributors it's important to find ways to transmit that thought process to our reviewers. And while I doubt there's a perfect way to achieve that, there are various techniques we can use to make our work easier to understand.

The first one, which is arguably both the best and the hardest to achieve, is to keep the scope of pull requests as small as possible. Smaller pull requests, without lots of moving parts, are easier to understand by looking at the code alone. If they have a clearly scoped goal, each modified line can be clearly traced back to that goal. Smaller changes can be more thoroughly reviewed. When the reviewer can understand the whole scope of the changes at once it makes it harder for bugs to slip in.

That being said, sometimes this isn't achievable. It could be that a big feature is full of moving parts and it's hard to scope them to a single change that makes sense to merge on its own. Or it could be that we send a pull request for a refactor which necessarily makes changes across the whole codebase.

For that purpose, I think writing a good description is crucial for the reviewer's understanding of our pull request. The modifications are just a list of all the moving parts that we needed to achieve our goal, the description tells the story of our work.

So, what makes a good description? This is highly subjective, but I think a good description should strive for a balance between clarity and brevity. A good description is the shortest description that can get the point across to our reviewer. And that means that it not only depends on the changes themselves, it also depends on the reviewer! A traditional feature in an MVC app might require little clarification if it's going to be reviewed by senior developers. A link to the corresponding user story in your issue tracker for background, maybe some clarification if you decided to add a new dependency, but little more is probably needed. If, instead, the pull request is going to be reviewed by someone less experienced, or someone from another team who might have less background on the codebase, then a more detailed explanation can be useful to guide them through the changes.

Even though this is subjective I've found that pointing reviewers to the interface of the changes, so to speak, can be incredibly useful. Were you working on UI changes? A screenshot or a GIF are worth a thousand words. Is it an API endpoint? Point your reviewers to the tests that use that new endpoint. Fixing a bug? Again, the tests that trigger the now fixed bug are a good place to make your reviewers go to.

Think about it this way: imagine you were sitting right next to your reviewer, explaining your changes. If at some point you would jump to the app and show something visually, that's the moment to add a screenshot. If you were to show how an endpoint works, that's the moment to point to a test. Give your reviewers something to get started with so that they don't have to navigate the changes blindly.

All in all, a reviewer has to understand what our goal is, how are we modeling the solution, and how did we implement it. Looking at the changes alone can explain the last part. In many cases, pointing to a ticket on an issue tracker can be enough to explain the goal. And it's left for the pull request description to tie those together by explaining the thought process that led to our particular implementation.

Bonus

Not all reviewers will necessarily be developers. In our team, we've found it incredibly useful to include project managers and designers in the review process. Given that they don't necessarily have an environment set up to run our changes locally, deploying each pull request to a separate environment simplifies enormously the process of reviewing changes from a functional point of view. This might not make sense on every project but I highly recommend it for web applications wherever possible.

Cover picture by Irvan Smith, found at unsplash

Writing tests for redux-observable

Julio Olivera — Sun, 24 Sep 2017 23:46:49 +0000

I attended a talk about redux-observable recently and, having been playing with Rx and liking it, I decided to give it a try. I won't get into the details of how to use it, but I did spend some time figuring out a way to easily test my epics that I wanted to share.

Let's say that, for authenticating users of our app, we have an epic that looks like this:

function loginEpic(action$) {
  return action$.ofType(LOGIN_START).mergeMap(action =>
    api
      .login(action.payload.email, action.payload.password)
      .map(loginSuccessful)
      .catch(err => Observable.of(loginFailed(err))),
  );
}

The epic takes our main stream of actions and for each LOGIN_START action, generates either an action with the loginSuccessful action creator, or one with the loginFailed action creator.

Here I see three things that deserve their own unit test:

The proper API call is being made.
We generate the success action if the login succeeded.
We generate the error action if the login fails.

The rationale behind all of the tests is going to be the same: We'll create an observable with the LOGIN_START action and pass it to the epic, subscribe to it and assert on the actions generated. Let's take a look at the first one, to check the API call:

I'm using Jest for the assertions and mocking here, but the same could be done with any other framework

it('logins through the api on LOGIN_START', (done) => {
  const email = 'test@test.com';
  const password = '123456';
  const action$ = ActionsObservable.from([login(email, password)]);

  api.login.mockImplementation(() => ActionsObservable.of({}));

  epic(action$)
    .subscribe(() => {
      expect(api.login).toHaveBeenCalledWith(email, password);
      done();
    });
});

A couple of things to note:

The login function is the action creator that generates LOGIN_START actions. Since we have it already, it makes sense to use it.
The API is implemented to return observables, so that's why the mock implementation returns one that simply emits an empty object (we don't really care about the response in this test)
api is mocked with Jest's mock facilities outside this test, like this:

jest.mock('../lib/api', () => ({ login: jest.fn() }));

Other than that, we pass the action stream to the epic, we subscribe and then we expect that after the first action is generated then we should've called the API already with the right parameters. Let's take a look at the tests that check the generated actions:

it('emits a LOGIN_SUCCESS action if the API call succeeds', (done) => {
  const action$ = ActionsObservable.from([login('test@test.com', '123456')]);
  const user = {};

  api.login.mockImplementation(() => ActionsObservable.of(user));

  epic(action$)
    .filter(action => action.type === LOGIN_SUCCESS)
    .subscribe((action) => {
      expect(action.payload).toBe(user);
      done();
    });
});

it('emits a LOGIN_FAILED action if the API call fails', (done) => {
  const action$ = ActionsObservable.from([login('test@test.com', '123456')]);
  const error = new Error();

  api.login.mockImplementation(() => ActionsObservable.throw(error));

  epic(action$)
    .filter(action => action.type === LOGIN_FAILED)
    .subscribe((action) => {
      expect(action.payload).toBe(error);
      done();
    });
});

The gist of it is that we filter the actions generated by the epic to ensure that we got the right type, and then when we subscribe we check that the payload of those actions are the right ones.

Note: I'm using filter instead of ofType like I would use to filter by action type inside an epic. This is because I can't be sure that the observable returned by the epic is going to be an ActionsObservable instead of a regular observable.

And that's it! I think this is a simple way to test epics. It might not be enough for more complicated cases, but personally I found it very simple and easy to reason about.