20 Tips for an Effective Code Review

It is a well-established fact that most of the bugs in the Software Development life cycle could be prevented literally right at the source (code). Since Code Review is almost an inevitable process in the Agile paradigm, keep in mind these 20 tips/guidelines (in no particular order) to become an effective reviewer of code. This is not restrictive to any one language but applicable to all. I've been reviewing code for many years and one of my core successes lie in stressing these points across the team. This is also the only way to effectively nurture and scale teams across the organisation.

1. Identify the right tool: Identifying the right tool is very important. Because one should not be thrown off for adding a review just because the tool is not efficient enough. There are many open source tools out there. In most cases, you may have to host it yourself or you can also opt for services that do the hosting for you. If you are an Open Source Contributor, you would know how effective Github can be which also happens to be my personal favorite.
2. Pre-conditions/Checklist: Any patch or a pull request that is submitted should have a minimum set of pre-conditions like it should have a Green build. A lot of Review tools have hooks to be configured to poll the SCM automatically and run the build. Build tools like Jenkins, Travis support these with minimal to no configuration. Ensure that you use them! Because it will definitely save you time and heartache instead of seeing stuff getting pushed to your trunk/master/production branch.
3. Avoid Repeat Mistakes: As Gustavo Fring from The Breaking Bad rightly says "Never Repeat the same mistake twice", it is crucial that repetitive patterns are broken. In the context of code review, this means the developers should not get the same review comment that they had received earlier. This ensures that with each Iteration - the Quality of the patches improves so that if at all any new review comments are there - they are only new and anything that is given in the past are assumed to have been implemented in the later ones. If this is not happening, it is up to the Reviewer to go and identify to see where the leak is.
4. Self Review: The person who is submitting the diff should first review it him/herself. Many obvious things like debugger statements, extra/missing files, ignorable files could be identified here. And I would recommend even doing a full fledged review of his/her own code as if he/she would do another one's. This culture also reduces the burden on the reviewer of concentrating on the Meat of the patch and not the obvious ones.
5. Design Review: The Reviewer should also be able to decode the design introduction/changes that the Pull Request has and should be in a position to judge and give appropriate feedback. This is very important. 
6. UI Review: Although software developers look at just the code and give feedback (because that is all they can be seeing in a Pull Request or a diff), they often neglect how the end product would look in a browser or the device where the code was intended to. It is extremely difficult to guess on how it will look. I recommend everyone to go the extra mile of looking how it looks and whether it relates to the original functionality. This is going to take some extra time. In my experience, this has insane returns in terms of identity and squashing obvious UI related issues. 
7. Non-Logical Checklist: Code Review does not only involve in vetting the Logical Integrity but also some non-logical things like Naming convention, Spacing/Indentation, Object oriented compliance checks. Ensure that there is such a checklist in the first place.
8. Keeping a pulse on the industry: This is true not only in this context but also on the overall wholeness of a Programmer. You should be up to date on what is going on in the Programming world, at least in the particular language you are part of. Knowledge of things like critical security patches, feature additions, language enhancements, performance improvements proves to be really powerful in assisting an effective review process.
9. Encourage feedback: One does not always have to agree with what is said in the Review. If there are some contradictions - it is best they are addressed between the Reviewer and the Reviewed (or Reviewee). I also encourage that all the review comments are responded to. This process gives confidence to the entire team that any review comment will not go unanswered. 
10. Avoid Oral Reviews: When a patch or pull requested gets created that too for teams and developers co-located or sitting next to each other, it is tempting to just go through it and give all the feedback orally. This may be fine if the team is small (only 2) and they fully own the codebase. However, this has some negative effects in terms of follow up and broadcasting. What I mean by broadcast is that there could be a review comment which could be applicable for the entire team.
11. Learn from other reviews: Encourage to team members to not just read your own reviews and apply however to read the other reviews within the team. I've heard this famous quote - 'An intelligent person learns from their own mistakes, but a genius learns from the mistakes of others'. Let's make everyone in the team a Genius! 
12. Dual Reviews: Similar to a Doubly refined sugar or Oil, the throughput and Quality of the code review could improve if it has a Second reviewer if that's possible.
13. Review the Reviewer: It is a bit over-zealous to expect anyone coming new to the team who is relatively younger to the software development or who has not involved in Review process in the past to quickly catch up to all the nuances in the code review process. It would be nice if these guidelines are slowly implemented and mentoring/onboarding is in the organisation's culture. In simple terms, there could be a reviewer who can review whether reviewer complies to all the best practices out there.
14. Over Engineering: At times it is tempting for Reviewers to comment on things that may look like Over Engineering work. These cases it is okay to voice your opinion to the reviewer.
15. Enterprise Adherence: An Enterprise will have an adherence in different horizontals in terms on what tools they need to use, what style guide they need to follow, what frameworks has been used across different projects. It is up to the Enterprise Architect or the Senior Member of the team to proactively absorb all these facts and ensure that the entire review process is in Adherence with the overall Enterprise. This is crucial because each Atomic commit may slowly introduce things that could stray away from what the Enterprise would want. It may not look like a problem at all in the initial phase. However, should there be a consolidation happen across various projects - having multiple stacked apps across the enterprise would result in painful Refactors and often ends up leaving a huge amount of technical debt behind.
16. Dependency Injection: Be wary of addition or removal of a new Library to the code. This again falls under the adherence of standards across the projects. Make sure that any introduction of a new library is well evaluated across the team and that it has enough support both in the near and long run. I have seen a lot of libraries which were started by individual contributors go unmaintained for years. Ensure that there is a strong community following and is very active.
17. Against the Right branch: This may seem like something that may not belong here but in my personal experience I've faced this issue multiple times where a Reviewed creates a pull request against a different (or default) branch instead of the one that it actually has to go.
18. Tech Debt Identification: During the course of the review, the reviewer may stumble upon an issue which involves a good amount of effort. In such cases, it is not advised to block it and hamper the delivery commitments. Instead, the right thing to do here is to add these things to a technical debt backlog where it could be groomed and picked up in future.
19. Copy Paste excuses: "I did not do this - it was already there - I just copied/moved it" - Yes this is a very common statement every developer says when his code is challenged - however ensure that any code that is touched has to comply to the coding standards set by the team.
20. Make Guidelines explicit: It is a very good process for all the developers on-boarding to a new team to have a set of guidelines (you could use this) explicit and review it from time to time. This could be done across the organisation.

The above list may look overwhelming. However, if you have the knack and right drive to implement some or all of these - the productivity of the Engineering team would increase by multi-folds.

Cheers!
Braga

Hotel Automation Controller - Interview coding problem

This is one of the problems I got via a friend who recently faced a company called Sahaj Software a clone company to Thoughtworks.

Problem Statement:

Hotel Automation Controller Problem  Statement

A very prestigious chain of Hotels is facing a problem managing their electronic equipments. Their equipments, like lights, ACs, etc are currently controlled manually, by the hotel staff, using switches. They want to optimise the usage of Power and also ensure that there is no inconvenience caused to the guests and staff.

So the Hotel Management has installed sensors, like Motion Sensors, etc at appropriate places and have approached you to program a Controller which takes inputs from these sensors and controls the various equipments.

The way the hotel equipments are organised and the requirements for the Controller is below:

• A Hotel can have multiple floors
• Each floor can have multiple main corridors and sub corridors
• Both main corridor and sub corridor have one light each
• Both main and sub corridor lights consume 5 units of power when ON
• Both main and sub corridor have independently controllable ACs
• Both main and sub corridor ACs consume 10 units of power when ON
• All the lights in all the main corridors need to be switched ON between 6PM to 6AM, which is the Night time slot
• When a motion is detected in one of the sub corridors the corresponding lights need to be switched ON between 6PM to 6AM (Night time slot)
• When there is no motion for more than a minute the sub corridor lights should be switched OFF
• The total power consumption of all the ACs and lights combined should not exceed (Number of Main corridors * 15) + (Number of sub corridors * 10) units of per floor. Sub corridor AC could be switched OFF to ensure that the power consumption is not more than the specified maximum value
• When the power consumption goes below the specified maximum value the ACs that were switched OFF previously must be switched ON

Motion in sub corridors is input to the controller. Controller need to keep track and optimise the power consumption.

Write a program that takes input values for Floors, Main corridors, Sub corridors and takes different external inputs for motion in sub corridors and for each input prints out the state of all the lights and ACs in the hotel. For simplicity, assume that the controller is operating at the night time. Sample input and output below.

Initial input to the controller: Number of floors: 2

Main corridors per floor: 1

Sub corridors per floor: 2

Since the hotel management is trying this for the first time, they would be changing the requirements around which electronic equipments are controlled and the criteria based on which they are controlled, so the solution design should be flexible enough to absorb these requirement changes without significant change to the system.

The solution to this problem involves approaching in an object oriented manner. Also we need to see here that we should use a Command/Strategy Pattern given there could be changes in the behavior based on external factors. I have not included the timings from the problem but from on here it should be easily extensible.

Code below:

Cheers!
Braga

Write a program to find the number corresponding to a alphabet and vice-versa with the given rule

Problem:
A = 1
B = A*2 + 2
C = B*2 + 3...
1. Write a program to find the number corresponding to a alphabet and vice-versa.
2. Given a series like GREP find the total in numbers. Given a number like 283883, find the shortest alphabetical series corresponding to it.
Compute above recursively when required and do not pre-compute and store beforehand.

Solution:
The solution is straightest-forward. Java code given below.

Cheers!
Braga

Write a program to build a small knowledge base about top "N" movies listed at IMDB

This was an interesting problem that I came across with. This problem is like an open book exam and race against time. The problem had to be solved within an allocated amount of time with a technology stack that is limited only to Ruby/Python/Node.js/Perl.

Few googling here and there and with some knowledge about Ruby I was able to solve this problem within an hour.

Problem Statement:

Build a small knowledge base about top "N" movies listed at http://www.imdb.com/chart/top. Each movie should mention the cast written on the individual page of the movie.
Given the name of a cast member, you should be able to return the movies out of the top "N" movies he/she has acted in. "N" will be passed by the user.
The knowledge base should be built during the runtime and stored in a data structure of your choice.
Q1. For example, if N=3, then you should parse the first 3 movies' individual pages from that page (http://www.imdb.com/chart/top) and build the knowledge base of the cast (there are about 15-20 on every page). Upon querying for "Morgan Freeman", you should return "The Shawshank Redemption". Do not use any external api for this.
Use only Ruby/Python/Node.js/Perl.

Solution:


Cheers!
Braga

Novel finder Program

This was one of the problems that I had faced during one of my interviews. The problem sounded interesting and I was given 1 hour to solve it which I did quite comfortably.

Problem:

The K-doublets of a string of characters are the ordered pairs of characters that are K distance from each other. A string is K-singular if all its K- doublets are different. A string is Novel String if it is K-singular for all possible K distances.

For e.g. take the string FLBL, its 0-doublets are FL, LB and BL. Since all these are different, FLBL is 0-singular. Similarly, its 1-doublets are FB and LL, since they are different FLBL is 1-singular as well. Lastly, the only 2-doublet of FLBL is FL, so FLBL is 2-singular. Hence, FLBL is a Novel String.

Note that the fact that FL is both a 0-doublet and 2-doublet is insignificant as zero and two are different distances.

Input:

The input is one or more non-empty strings of at most 100 capital letters, each string on a line by itself, followed by a line containing only two dollars ($$) signaling the end of the input.

Output:

For each input line, output whether or not it is a Novel string using the exact output format shown below.

Sample Input: (Input File: novel.in)

FLBL

FFLL

ORDINARY

R

QYQRQYY

$$

Sample Output: (Output File: novel.out)

FLBL is a Novel string

FFLL is not a Novel string

ORDINARY is a Novel string

R is a Novel string

QYQRQYY is not a Novel string

Solution:

To attack this problem, you first have to find all the doublets for each distance starting from 0 to the maximum distance which would string's length minus one. Then keep pushing them into a hash set. Whenever the add() method of the Set returns false, it means we are adding a duplicate hence, its not a Novel text. Here is the Java source code.


Cheers!
Braga

Exception Handling in Windows Phone 7 - When all else fails

I was recently testing something with Toast Notifications with Windows Phone 7.1. Visual Studio that I use has amazing debugging features like any other IDE available IDE out there. Here was scenario. I need to send two types of Notifications Raw and Toast Notifications to my mobile and need to assert scenarios. Debugging the Raw Notification was very simple. However, I was not able to debug/step into when I launch the application after clicking the toast Notifcation.

It was really painful not to be able see any exception message. It was very annoying. Then I found a way to at least know what is happening. To do that, you can just go the App.xaml.cs file which has all the main entry and exit points to the application. To read more about this, just google life cycle of a windows phone app.

All I had to do was add a message box to the method Application_UnhandledException in the App.xaml.cs.

private void Application_UnhandledException(object sender, ApplicationUnhandledExceptionEventArgs e)
{
    MessageBox.Show(e.ExceptionObject.Message + "\n" + e.ExceptionObject.StackTrace);
    if (System.Diagnostics.Debugger.IsAttached)
    {
        // An unhandled exception has occurred; break into the debugger
        System.Diagnostics.Debugger.Break();
    }
}

Problem Solved. Now I could see the error messages/exception stack trace in a neat Message box!

Cheers!
Braga

Reconstruct a binary tree given its Inorder and Preorder traversals in Java

This is one of the interesting problems that I encountered recently. Given two sequences one being an in-order traversed sequence and other being a pre-ordered traversed sequence, a binary tree has to be reconstructed out of it.

This is theoretically possible. This problem can be cracked if we form a common pattern between these two traversed values. In a pre-order traversal, the root element will always be the first element. This element when searched in an in-order traversal will have all elements to the left on the left side of the array and all elements to the right on the right side of the array.

Applying this solution recursively will give the binary tree for us. I have included a Swing UI so as to visualize the output easily.

Example,
Pre Order Sequence = A B D E C F G H I
In Order Sequence = D B E A F C H G I

I tweaked a bit from this source for UI.

The Node structure

The code for UI

Hope you learnt something today along with me.

Cheers!
Braga

Given a sorted array, find k in least possible time in Java

Given a sorted array and a value k, write a program to find whether that number is present in that array or not.

The first solution that comes to the mind for this problem is to traverse through the entire array and find whether it is having the value k and return true or false. This takes O(n) running time. But then, we are presented with a hint that the input array is sorted (lets assume in ascending order). This problem can be attacked by doing a divide and rule analysis.

This can be done recursively. See whether k is inbetween the last and first element in the array. If it is, then divide the array into two and repeat the same. If its not, simply return false. Yes, its as simple as that. Following is the java code for the same.

Leave some comments if you have anything to say.

Cheers!
Braga

Find middle element in a circularly sorted array

Given a circularly sorted array, find its middle element in best possible time.

A circularly sorted array is an array that is obtained by shifting an array k times. For example, the following is a circularly sorted array,

[5, 6, 7, 8, 9, 1, 2, 3, 4]

If you sort this array and find the middle element it will be 5. But then, sorting is a painful operation which will take O(nlogn) timing. So it is out of the equation right away. If you closely observe this problem, it is enough to find the shifting index, the place where the sorting varies. For example, the shifting index in the above example is 5.  Once that is found, then it is a matter of applying the bias over the length.

This program can be attacked in a recursive manner. Take the first and last elements in the array and compare them. If they are sorted, then the last element is greater than the first element. If this condition is not met, we will have to divide this array into two halves and try applying the same logic. If you do this, at some point you will arrive at a situation where you are left with only two elements or lesser. The end value is the shift index. Once the shift index is found, it is easy to find the middle element(s) for odd and even cases. Java code is below.

Leave comments if something is unclear.

Cheers!
Braga

Queue using Stack in Java

A Queue can constructed with its underlying data structure being a stack. Typically, we will need two Stacks one for enqueue and the other for dequeue.

During every enqueue() operation over a queue, we will keep pushing value to a first stack, lets say stack1.

During every dequeue() operation, we will have to simply pop an element from stack2 if stack2 is not empty. If stack2 is empty, we will need to pop all elements from stack1 one by one and push it to stack2. And then pop an element from stack2.

Take for example, I am inserting elements from 1 to 6 to a queue. This is how the following queue and the corresponding stack will look like,

Then trying a dequeue() operation will try to immediately pop values from stack 2.

So, it will pop all values from stack1 and push them to stack2 as follows,

Now the dequeue() operation shall be performed with ease since stack2 is not empty.

Finally, more enqueue will add or keep pushing values to the stack.

Following is the Java code. Note, I have not comprehensively covered the entire methods in a Queue, but this should be well more than enough.



And a test class for this with output


Cheers!
Braga

Program to check whether a tree is a BST

Write a program to check whether a given tree is a binary search tree.

We all know that a binary search tree follows the rule : for any given node with value N, all the values in the left of that node is lesser than N and all the values in the right of that node will be greater than or equal to N. For example,

This is a tricky question. We may try to immediately run through all the nodes checking only the immediate left and right nodes and coming to a conclusion that it is a binary search tree. But this program may not work for this scenario.

The simplest solution to this problem is to do an inorder traversal through the entire tree and find whether it is sorted or not. Typically we can do that by pushing into any array. Even better is to calculate the minimum value at each step. Program is as follows. Please leave some comments if something is confusing. I will get back immediately.

Cheers!
Braga

Minimal Markatable Subset

It has been a long time since my last technical post. Yes, 6 months!! I wanted to write something on iPhone Application development. But before doing that, I want to tell you guys on something that I learnt during my two day training on clearing the Certified Scrum Master.
During this training, I heard this term called "Minimal Marketable Subset". After hearing to what it meant, my view as a developer changed drastically. Basically, I want my code to be perfect. Whenever I develop something, I want it to be highly user interactive, highly easier and the highly optimized. But know what, that is one of the huge mistakes I have done so far. Because of this attitude, I can never be satisfied and declare my product/project as completed. I felt that was the right way to think till I heard of "Minimal Marketable Subset". It is the minimum thing that is needed to get something up and running. You can develop in increments as the application develops. That is the basis of any Agile practice and I have started to adopt it to my programming practice.
To put in simple terms, "do what you can with what you have and do it quick"

Print % using printf in C

Problem:

How can you print % using the printf function? (Remember % is used as a format specifier!!!)
Very Simple, following cases are examples


Cheers!!
Jack

Find Output for this C Program

Problem:

What would be the output of the following C program? (Is it a valid C program?)


Explanation:

This will produce output “4321”. The return type of printf is “int” which is the number of characters written to stdout.


Cheers!!
Jack

Addition without using the + operator in C

Write a C function which does the addition of two integers without using the '+' operator. You can use only the bitwise operators.(Remember the good old method of implementing the full-adder circuit using the OR and XOR gates....)

Now, for the code implemented in C.

Cheers!
Jack

Print a Matrix in diagonal zig zag order

Problem:
Given a square matrix, write a program to print the items in zig zag diagonal order.

Following is an example,

So the output expected for the above example is,

1 -> 2 -> 6 -> 3 -> 7 -> 11 -> 4 -> 8 -> 12 -> 16 -> 5 -> 9 -> 13 -> 17 -> 21 -> 10 -> 14 -> 18 -> 22 -> 15 -> 19 -> 23 -> 20 -> 24 -> 25

Solution:

This program is a bit tricky and is very hard to solve immediately when asked in an interview. People who try to attack problems involving matrices always think they need at least two loops to arrive at the solution. But the surprise here is the problem can be solved in just a single loop with a very simple logic. I have provided the solution in C++ below, you may also try to solve the same in the language of your choice!!


Cheers!!
Jack

Project Euler : Considering natural numbers of the form, ab, finding the maximum digital sum.

Problem 56

A googol (10¹⁰⁰) is a massive number: one followed by one-hundred zeros; 100¹⁰⁰ is almost unimaginably large: one followed by two-hundred zeros. Despite their size, the sum of the digits in each number is only 1.
Considering natural numbers of the form, a^b, where a, b < 100, what is the maximum digital sum?

Solution (in Ruby)

Nothing much to discuss about the solution, I've implemented it directly as given in the problem statement.

Hover here to see the solution

Cheers!!
Bragaadeesh

Project Euler : Evaluate the sum of all amicable pairs under 10000.

Problem 21

Let d(n) be defined as the sum of proper divisors of n (numbers less than n which divide evenly into n).
If d(a) = b and d(b) = a, where a b, then a and b are an amicable pair and each of a and b are called amicable numbers.
For example, the proper divisors of 220 are 1, 2, 4, 5, 10, 11, 20, 22, 44, 55 and 110; therefore d(220) = 284. The proper divisors of 284 are 1, 2, 4, 71 and 142; so d(284) = 220.
Evaluate the sum of all the amicable numbers under 10000.

Solution (in Ruby)

For this problem, it is vital to write an efficient method to find the sum of divisors of a number. The divisor finding logic is same as the one stated in Problem 12. Rest is an as-is implementation of the problem statement.

Hover here to see the solution

Cheers!!
Bragaadeesh

Project Euler : How many letters would be needed to write all the numbers in words from 1 to 1000?

Problem 17

If the numbers 1 to 5 are written out in words: one, two, three, four, five, then there are 3 + 3 + 5 + 4 + 4 = 19 letters used in total.
If all the numbers from 1 to 1000 (one thousand) inclusive were written out in words, how many letters would be used?

NOTE: Do not count spaces or hyphens. For example, 342 (three hundred and forty-two) contains 23 letters and 115 (one hundred and fifteen) contains 20 letters. The use of "and" when writing out numbers is in compliance with British usage.

Solution (in Ruby)

We will have to first try to come out with the unique words that are present in the numbers. We start with the ones which are one, two upto nine. Then the elevens starting from eleven to nineteen. Then the tens, twenties upto nineties. We should also define a hundred and thousand. After that the logic is directly readable from the ruby code given below.


Hover here to see the solution

Cheers!!
Bragaadeesh.

Project Euler : Discover all the fractions with an unorthodox cancelling method.

Problem 33

The fraction ⁴⁹/₉₈ is a curious fraction, as an inexperienced mathematician in attempting to simplify it may incorrectly believe that ⁴⁹/₉₈ = ⁴/₈, which is correct, is obtained by cancelling the 9s.
We shall consider fractions like, ³⁰/₅₀ = ³/₅, to be trivial examples.
There are exactly four non-trivial examples of this type of fraction, less than one in value, and containing two digits in the numerator and denominator.
If the product of these four fractions is given in its lowest common terms, find the value of the denominator.

Solution (in Ruby)

This problem unlike the other problems does not ask us to go to millions and trillions and the scope is such that this can be solved in the least running time. All this problem demands careful inspection of the problem statement itself. Its clear we need to iterate through numbers from 1 to 99. For the outer loop (numerator), its enough we run from 12 since its the first double digit number which does not end with a 0 or their digits being equal. We run upto 97 since the numerator cannot be greater than equal to denominator since the fraction should be less than 1
Same is the case for the inner loop. We should always continue to the next iteration if the last number of either numerator or denominator is 0, or if both digits or equal or if the numerator is greater than or equal to denominator. Rest of the solution is just a language specific implementation of finding whether the two different fractions are indeed equal.

Hover here to see the solution

Cheers!!
Bragaadeesh.