DEV Community: Udayakumar Rayala

Code review checklist

Udayakumar Rayala — Wed, 04 Jul 2018 14:35:34 +0000

http://blog.smartbear.com/wp-content/uploads/2015/09/Creating-Your-Code-Review-Checklist.jpg

I have posted in my last blog about how we do code reviews and compared it with pair programming.

One thing I noticed is, every team and team members have a different list of things they look for while doing code review. In this post, I will list out the things I look for. This is no way a complete checklist. If there any checks which you think are missing, let us know by adding them in comments.

Design

Depending on what programming language or platform you use, check whether the new code follows the right design.

What is a right design is up to the team to agree upon? It is always best to have upfront discussions among developers when new abstractions are brought in and the new design is followed. Code reviews can be a way to make sure it is implemented as discussed. This will make sure there will not be a lot of rework. But this can be exempted when you want to implement something new and then get feedback on it. Make sure you write enough so the idea is conveyed.

Following are some of the things you should check for (Thanks to Bhavin Javia for contributing to most of this list):

Verify if coding conventions as per the designated style guide is being followed to maintain consistency and best practices.
Check for Duplication inadvertently introduced by developers e.g due to lack of awareness about existing reusable components, misplaced logic/responsibilities, generic/reusable code not extracted and just copy/pasted elsewhere etc
Identify Refactoring opportunities which were missed or deferred during earlier tech debt and code reviews. Often if the developer is working in that area, has a lot of context which makes it easier to include some refactoring tasks as part of the story/PR vs finding a dedicated block of time for refactoring.
Spot and warn developers about potential dependencies or conflicts with other developer’s code e.g. when multiple features which touch same areas of the codebase are under development or review
If there’s any non-standard feature or hack is being introduced, make sure it’s annotated with a #TODO or #FIXME tags along with an appropriate comment e.g. reference to the code snippet/patch being used with reason and what the ideal solution should be. e.g. #FIXME Patch to workaround bug # in <framework> <link>. Remove after upgrading to fixed version.
Look out for new libraries introduced and see if they are required and are the latest version. Are there better alternatives for it?

Tests

All the above checklist about code also applies to test code.
Make sure all the code have tests written at all levels — unit, integration and functional.
Check if all tests passing.
Check if the tests are acting as documentation describing the intention of the code it is testing.
Check if tests give proper error messages when they fail. For example:

expect(subject).to include(:customer) #is better than:
expect(subject.key? :customer).to be\_true

Wear the QA hat

This is something which most people miss or probably think not as important. Working of the code is as important as its quality. Even if you have a testing team, catching bugs at code review will reduce the cost of fixing it.

Read the story description, ask questions to the BA or Product Owner and see if all the acceptance criteria is implemented as mentioned. I feel most bugs can be caught with just this check.
Think about scenarios outside of what is mentioned in the story. Check if the usual user flows related to the new changes are still working as expected.

Cross functional requirements (Non Functional Requirements)

Also focus on cross functional requirements like Performance, Security, Analytics, Logging, Alerting etc.

Check if the code can introduce any performance issues like N+1 query problem, or loading entire database into memory etc. Think about running this code in a production environment and anticipate what kind of problems might arise. Of course you can’t find all of them just by looking at the code but you might be able to identify some common mistakes from your past experience. Like doing Customer.all.
Security is something which can be identified early. Check if data is accessed only by authorised users. Read about common security precautions which can be taken and share the same understanding with the team.
If you already have integrated with some analytics system, check if the new functionality should integrate with it.
Check if enough logging is done to be able to debug the application.
Check if alerts are added when things break.

Continuous Delivery

When you are following continuous delivery, you should make sure the new changes don’t break production data and functionality.

Make sure data migration scripts are added and check if they execute as expected. Run them on your machine once if required. Don’t make changes which will result in data loss. Also, make sure the features going to production are auto-deployed to a staging or pre-production environment which has a replica of production data (obfuscated) to detected and fix potential issues.
Check if feature toggles are added for any functionality which you don’t intend to release.

Other

Check if the code under review is merged with the latest master branch code.
Make sure instructions are added for any other teams to follow after integrating these changes. For example, informing the QA team what areas to run regression on if there are any major changes. Or informing any downstream consumer systems for any changes in the API contract. Or updating README for helping other developers.

Overall, keep the changes to be reviewed small and raise pull requests soon to get feedback early.

Please add your thoughts in the comments. And if you like the post, recommend it so others can find it and participate in the discussion.

How to create your own AWS Lambda Middleware framework in Node.js?

Udayakumar Rayala — Wed, 06 Jun 2018 08:24:58 +0000

AWS Lambda Middlewares Framework

We have around 25 lambdas and they share some common functionality like:

Integrating with Sentry
Removing unnecessary events
Logging information about the event being processed
Datadog tracing etc.

We needed a way to define these functionalities once and reuse them across lambdas. This post is about a middleware framework we wrote in Node.js to do this with very little code and no external libraries.

Middleware Pattern

We could have extracted each of these functionalities into a function and called it in the lambdas which would have allowed us to reuse the code. But we thought if we can bring in some convention, integrating/maintaining these common functionalities into lambdas would be much easier.

Having working with a lot of web frameworks which have concepts like filters in java, middlewares in Express, we felt a similar approach would work for us as well.

The idea is to implement a lambda handler function as a core function and a set of middleware functions.

The Core function will do what the lambda is supposed to do like transforming the incoming data, writing the events to a data store etc.
Each middleware will handle one functionality like integrating Sentry or logging etc.

This is the order in which the middlewares are called:

MiddleWare1 -> MiddleWare2 -> MiddleWareN-> CoreFunction

Middlewares have the same signature as the lambda handler function with an additional argument of the next middleware. In case of the last middleware, the next argument will be the core function. If the core function returns some value, middlewares typically return the same value.

And each middleware function can choose when to call the next middleware. This way we can divide a middleware function into three sections:

Before Section — This code is executed before the core function. We can do things like filtering events, adding more data to context etc.
Next Middleware Section — This could be the actual call to the core function. And the middleware has a choice to not call the core function at all and finish the lambda execution.
After Section — This code is executed after the core function. We can do things like error handling, logging or returning a different response etc.

Promises instead of Callbacks

Most of our lambda function code create or work with promises since they mostly query/write to external systems like Dynamodb, Elasticsearch etc. We thought it would be easier if our core function and middlewares work with promises instead of callbacks.

A typical AWS lambda definition in Node.js v6.10 would look like this:

exports._myHandler_ = function(event, context, callback){
   // Do something
   // callback(null, "some success message");
   // or
   // callback("some error type");  
}

And this is how we wanted our handler functions to look like:

const handlerFunction = (event, context) => {
  return new Promise()
}

Note: Node.js v8.10 supports async handlers which was not available when we wrote this middleware framework. This step might be redundant in Node.js v8.10.

Middleware Orchestrator

Because we have a different signature than what lambda handler is expected to have, we created a function withMiddlewares.

It takes the core function and an array of middlewares as input and returns a function which has the same signature as the lambda handler.

export._myHandler_ = withMiddlewares(
  handlerFunction, 
  [Middleware1(), Middleware2()]
)

And this is the implementation of withMiddlewares:

Line 1: It has the same signature as the lambda middleware.

Lines 2–14: chainMiddlewares returns a function which recursively calls each middleware and finally calls the handler. If there is an error thrown when calling the middleware, it will return a rejected promise.

Lines 16–21: We call chainMiddlewares with all the middlewares and convert the resulting promise into a callback function.

Middleware Template

const Middleware = () => {
  return (event, context, next) => {
    // Before Logic
    return next(event, context)
      .then(result => {
        // After Logic
        return result
      })
      .catch(error => {
        // Error Handling
        return Promise.reject(error)
      })
  }
}

Example 1: Sentry Middleware

Integrating Sentry typically involves:

Before Section — Initialising raven library
After Section — Reporting errors to Sentry

This is the trimmed down version of how this middleware definition would look like:

captureExceptionAndReturnPromisewill wait for Sentry request to complete since it is returned as promise.

If we are not able to send the request to Sentry for various reasons like Sentry is down or network error, we currently bubble up the error. But we can also log the error and return the original error.

Example 2: Dynamodb Insert/Modify Events Filter

Some of our lambdas want to ignore Dynamodb delete events and execute only on the insert/modify events.

Before Section — Remove delete events from event.Records
After Section — No after actions

We are modifying the event.Records here. Another option is to clone the event with filtered records if we want immutability.

Example 3: Lambda Handler

This is how a lambda handler would look like using the above defined middlewares. The order of the middlewares is important. SentryMiddleware should be the first middleware to catch errors in downstream middlewares too.

Conclusion

Implementing this pattern made our code more readable and testable. You can find the full implementation with tests and sample middlewares here.

Team who worked on this: Petter Uvesten, Linus Basig, Barbora Brockova, Stéphane Bisinger.

Incremental refactoring using Scientist

Udayakumar Rayala — Thu, 09 Mar 2017 04:47:50 +0000

In one of my recent projects, we had to replace a part of the code which calculates price with a new pricing service that has more functionality.

Calculating price being one of the most critical part of our service, we were faced with the following questions—

How do we know if the new pricing service is doing same as calculations we have?
If it is not working as expected, what are the inputs for which it is not working?
How do we know if we have integrated with the new service correctly ?
What is the performance impact of calling the new service? Is it better or worse?

We could have tested with a bunch of scenarios with known inputs but production is all about unexpected situations. We wanted a way to test the integration before we can release the changes to the users. That is when we came across Scientist.

Scientist is a small ruby library which gives necessary framework to test your new code without releasing it. The main building block is an Experiment. It has two blocks of code control and candidate. Both blocks are executed every time an experiment is called. But only the result from control block is returned. Exceptions in candidate block will be rescued. And at the end, results from both the blocks can be compared and logged for further analysis.

The idea was to create a Scientist experiment with control block as the existing local price calculation and candidate block as the call to the new pricing service. And compare the results before we can go live.

Preparation

Couple of things we wanted to do before using Scientist:

Refactor the existing code into one class. This way we can use Class Polymorphism to switch between using existing implementation, new implementation or the scientist experiment. We use Figaro for managing application configuration. So this refactoring to using class polymorphism would help us pass the class to use as a configuration to our service. I will show below how we did that.

After refactoring, our existing pricing calculations were moved into the LocalPricingEngine with estimate method:

We needed a place to log the input params, what the control/candidate blocks returned, and the time it took to execute each block. There are number of places we can log this information like Redis, Postgres DB etc. But we chose to log it in our log file. We already had json logging using Lograge. So putting in log file would make it easy to retrieve this information in one line.
Although low level information of logging per input difference is useful, it would be hard to get a summarised view of what happened. We need to be able to find out how many mismatches happened and at what time of the day (our price calculation varies by time of the day). We already use DataDog for monitoring/alerting and it made sense to put this information right there. I will show code snippets on how we did this later.

Implementation

We created three polymorphic implementations of PricingEngine:

LocalPricingEngine — Existing implementation of price calculations.
PricingServiceEngine — New implementation which calls the pricing service for calculations.
ExperimentalEngine — Scientist experiment which calls the above classes for control and candidate blocks.

And also a factory which would choose the right implementation based on our environment configuration:

We replaced the call to LocalPricingEngine with the call to PricingEngineFactory.engine.

So this:

LocalPricingEngine.new.estimate(...)

changed to:

PricingEngineFactory.engine.estimate(...)

We now have the ability to change the pricing engine from our environment variable PRICING_ENGINE. It would have one of these values: LocalPricingEngine or PricingServiceEngine or ExperimentalEngine. And not having this environment variable would choose LocalPricingEngine by default.

This is how the ExperimentalEngine looked like:

I trimmed down the variable names for better formatting. e represents an experiment object and p1, p2, p3 are param1, param2, param3 respectively.

Publishing Results

This is the step I spent most time to figure our inspite of Scientist’s good documentation.

By default Scientist doesn’t publish the results anywhere. If we want to publish the results, we can do so by creating a new Scientist::Experiment and overriding publish method.

As mentioned earlier, we want to log two things:

Experiment results to log file.
Summaries to DataDog.

We needed a way to communicate the experiment results to Lograge. And since Lograge custom options can only be passed at controller level, we used RequestStore as a way to pass the results from Experiment to Controller.

This is how the controller configuration looked like:

We used statsd client to publish the summaries to DataDog.

This is how the experiment class looked like. You can replace MyService in MyServiceExperiment with your service or application name:

There are two methods to implement:

enabled? — This controls whether the candidate block should run or not. In our case, we always enabled since we controlled this behaviour from the class polymorphism we implemented.
publish — This method is called after both control and candidate blocks are run. We are publishing the results to DataDog and to Lograge through RequestStore.

Finally, we needed to tell Scientist to use this new experiment we created. You can do so with the following code:

You can put this in initializers folder if you are using Rails.

Outcome

Using Scientist for this use case helped us in the following ways:

Test whether the new service can handle the load. Our new service was production ready, so decided to take the entire load. But if you dont want to take all the load, you can use the run_if or enabled?block to control how much percentage of load you want to run the experiments for. More info here.
Uncover edge cases. We had a rule to increase the prices at certain time of the day. We misconfigured the new service to stop increase of price one minute early. We uncovered this when we saw a lot of mismatches only during this one minute. There was also a rounding issue on how the final price was rounded. Although it was small for one price calculation, it would add up to a lot when thousands of calculations were made.
It helped us give instant feedback about the difference in prices when changes were made either in local price calculation or in the new service.

Things to look out for

control and candidate blocks run sequentially. So this would increase the overall execution time. It would have been nice if they could run in parallel.
control and candidate results are compared using ==. You can override this behaviour by defining e.compare method. More info here.

Conclusion

We migrated to the new service without resulting any user issues and with greater confidence. We incrementally built the integration releasing one or two times every day without the risk of causing any inconvenience to the users. We caught bugs early and our production started giving us feedback as we built.

Using Scientist was pretty smooth. It is a very small library. You can read through its code in less than an hour. And the documentation is pretty good which covers most of the commonly asked questions.

Code Reviews vs Pair Programming

Udayakumar Rayala — Fri, 08 Jan 2016 11:29:53 +0000

Code Reviews vs Pair Programming

I’ve spent nine years working in teams which religiously follow pair programming. I’m used to working that way and appreciate the benefits it brings.

We didn’t have the luxury of pair programming all the time in my last project. This required us to do code reviews to ensure the quality of the code we delivered. This post is an attempt to consolidate the upsides and downsides of doing code reviews instead of pair programming in my three months of experience.

How do we do it?

We use Github as our source code repository. The master branch is always considered to contain production-ready code. Each story is developed on an independent branch, a pull request is created, reviewed and merged back into the master.

We use git-reflow to facilitate code review. Working on a story typically involves three steps:

Start — You do a git reflow start on master. This creates a new branch with whatever is on the master branch. You can continue working on this branch and it should not affect what is released to production since only changes that are in master go to production. Our CI(Teamcity) was configured to run the build on every branch that is created. So as and when changes were made, the tests were run and verified.
Review — After completing your changes and reviewing them yourself, you can raise a pull request by running git reflow review. This will create a pull request (PR) on Github. You can then ask any of your team members or a particular person to review. They will put their suggestions/comments as inline comments on the PR. You can have a conversation on whether any changes are required. This generally goes back and forth between the developer and the reviewer. Once all the required changes are done, the reviewer marks that the PR can be merged by adding an ‘LGTM’ comment on the PR.
Deliver — You can now merge the PR into the master branch. You might have conflicts if someone else has merged and made similar changes. After a successful merge, you have to make sure the expected functionality is working on master and all the tests pass.

You can find more information about git-reflow in these posts:

https://guides.github.com/introduction/flow/

http://scottchacon.com/2011/08/31/github-flow.html

Upsides

Specialist feedback — Although it is encouraged to have more generalists in an Agile team, it is often advantageous to have some specialists or people who are experts in some areas like CSS, database, or rails. And having a setup like this helps in getting their feedback on the code before merging. This encourages sharing of knowledge. I’ve found this to be more scalable than getting the specialists to pair with each member of the team. This also applies to teams where there are more junior developers than senior developers who may need feedback on the code written.
No keyboard hogging — Pair programming is useful as a way of sharing knowledge. But I have seen sometimes people with more experience dominate the pairing. Although it is great in the beginning to share knowledge, it doesn't help junior folks build the confidence to work on their own. Even if they are given a chance to navigate while pairing, they might feel some pressure because they are wasting their pair’s time to keep up to their pace. When following code reviews, junior folks can work on the stories by themselves. They can get occasional help if required and also spend more time reading outside core working hours without the guilt of wasting their pair’s time. They might initially take more time to deliver a story but it helps in building their confidence.
Suits distributed teams — I have done a lot of remote pairing. Even though it works well, you often have timezone and bandwidth challenges. Since code reviews with pull requests are done asynchronously, they also allow team members to follow working hours that are convenient to them.
Conversations are archived — Pull request comments are useful for someone to know why things are done in a certain way later.
Highly collaborative — The conversations in a code review are visible to the entire team. If there is something I can provide input on, I can jump into the conversation. This happens in pair programming when the entire team is sitting in one place and conversations can be overheard. But it’s not always possible for distributed teams.

Downsides

Working on stories which have dependencies is hard — There might be an in-progress branch from which you need changes, but you can’t get them onto your branch because they haven’t been merged to master. This requires the developers on dependent stories to work very closely and a lot of git merges and rebasing.
Time lag between story development and review — This often happens because people are busy developing their own stories. You sometimes have to follow up with people multiple times to get the review done.
Reviewing big changes is hard — If there are a lot of changes, it takes a lot of time and effort to review them thoroughly.
Delayed feedback from CI pipelines — CI pipelines for every new branch works great only at one level. If you have a second level build which runs end-to-end tests on multiple services, you cannot run them on all the new branches. Hence, the feedback is delayed.

Conclusion

This doesn’t mean we shouldn’t do pair programming at all. It still helps to pair when you’re on-boarding new people or setting up a new code base. And I feel it is still useful to do some form of code review in teams that follow pair programming. But make sure you keep the stories small and don’t end up having long-lived branches.

Please add your thoughts in the comments. And if you like the post, recommend it so others can find it participate in the discussion.