Category: CodingAdventures

The title is a little bit misleading, because this post is going to cover both a Git and a Bash tip. But this is my blog, so I title my posts the way I want.

Please, bring on some tips!

This is a tiny little trick that may be useful only if you ask Git to fulfil your wishes using the command line. Which you should probably be doing at this point, if you want to call yourself a real developer.

Git commands are totally long and totally hard to remember. True. But what if they could be shortened?

Well, in a way, git commands can be shortened. Or at least, adapted to your personal preferences, thanks to aliases.

Aliases can be global or specific to a repository. For example, if you are bored of typing:

Shell

1

$git pull origin

You can create a “local” alias:

Shell

1

$git config alias.popull origin

And from that moment on, you can just do this:

Swift

1

$git po

And voilà.

If you want to apply the alias to all your repos:

Shell

1

$git config--global alias.popush origin

And voilà again.

My favorite? This one:

Shell

1

2

$git config alias.last'show -s HEAD^commit'

$git last

Cool, but what about the bash tip?

Yep, I have one of those as well.

bash (the only true Unix shell) allows the creation of shortcuts to certain commands, called alias. For example, if you find yourself typing a thousand times a day the following command:

Shell

1

$git checkout master

Wouldn’t it be neat to be able to just type the following?

Shell

1

$go master

Well, that’s easy to do in just to steps. First, edit the bash configuration:

Shell

1

$pico/.bashrc

Now add the following alias to that file:

Shell

1

aliasgo="git checkout"

Save and exit, and force bash to refresh:

Shell

1

$./.bashrc

(Note the space between the first dot and the curly-thingy-that-we-use-in-Spanish-on-top-of-the-letter-n-to-make-it-sound-funny)

Git tags can be annotated. An annotation is a message that will be associated to the tag, and that can be used to embed some extra information that might help remembering, months later, why you decided that a particular commit was worth tagging:

Shell

1

$git tag-av1.5.RC1-m'version 1.5 Release Candidate 1: iPhone support'

Tags can be listed:

Shell

1

$git tag-l

But the result is a little bit disappointing, specially if you added annotations (hint: annotations will not be shown).

If you want to see your extra-meaninful messages added to your tags, you can do something like this:

Shell

1

$git tag-l-n1

That basically provides either the annotation or the first line of the latest commit message associated with each tag.

You already know that searching for a particular tag, by tag name, is easy:

Shell

1

$git tag-l'v1.5.*'

But, what if you only remember part of the annotation, and you want to know the tag it was associated to? Well, it’s a matter of combining some of the previous examples, and a little bit of grep-foo:

Magic? No! All it takes in some adventurous development backed up by the proper amount of tea or coffee.

How does it work?

Thanks to a combination of NSUserActivity and the OS that sends instances of those activities back and forth between devices. But that is not the best part. The best part is that NSDocument, UIDocument, NSResponder and UIResponder (and therefore all UI classes on both OS X and iOS) have a new property called userActivity which is, surprise, of NSUserActivity type.

So basically, all you have to do, is create a new instance of NSUserActivity in the view / view controller / window / document that you want to be advertising itself as allowing Handoff. The system will take care of sending that activity to other instances of your app in other devices.

After that, you will also need to add a couple of methods to your application delegate, and done!

By the way, Handoff is allowed between multiple instances of iOS apps, between iOS apps and OS X apps, and even between apps and websites.

Neat! How can I support Handoff in my app?

In order to be able to implement Handoff in your Mac / iOS app, all you need to do is…

The system will call becomeCurrent in the view controller automatically if said view controller is in the view hierarchy, but if it is in transition, it will call it after the transition has been completed. Why should you care about that? Because calling becomeCurrent, starts advertising the activity (which is what you are supposed to want, by the way).

Also, when the view controller is removed, or in general, when the system decides that the activity is not needed anymore, it will call invalidate in the activity.

Put information in the activity

Usually, you would want to put some context into the activity. After all, that is the way you will be able to recreate the state in the destination app. You can do that by putting information in the activity’s userInfo dictionary.

In order to do that you should override updateUserActivityState in your UIResponder subclass:

You can put in that dictionary anything that can be serialised to a plist file, in other words, the usual suspects: NSArray, NSDictionary, NSString, NSURL… you get the idea. Be frugal, though, the information you put in the dictionary has to travel the interwebs to reach other devices, you might want the payload to be as light as possible.

Allow your app to reconstruct its state from an activity

This is the neatest part of the whole thing. In your app delegate, the system will call application:willContinueUserActivityWithType. That’s the moment when you need to provide visual feedback to the user indicating that something is about to happen. For example, that would be the perfect moment to start transitioning to a new view controller.

Now, for each call to willContinueUserActivityType, the system guarantees that there will be a call to either a success or an error handler.

The success handler will be called when the system fetches the user activity. The system will pass a block that you will have to call yourself, passing it an array of whatever you want to respond to that activity (in most cases, that would be an instance of the VC you created in the previous step).

Now, in your MainListingViewController, the system will call restoreActivityState. That’s where you can recreate your UI state, according to whatever you receive in the activity’s userInfo. You can also call this method manually if you need to.

Swift

1

2

3

4

5

6

7

8

public overridefuncrestoreUserActivityState(activity:NSUserActivity)

{

//don't forget to call super!!!

super.restoreUserActivityState(activity)

letuserInfo=activity.userInfo

loadDataWithCategory(userInfo["category"])

}

If the system can not fetch the user activity, instead of continueUserActivity, it will call didFailToContinueUserActivity in your app delegate

Warning! The post title might be a little bit misleading. Or not. Read on, and decide.

In the coming paragraphs I am going to look at a few different issues like Swift-Objective-C interoperability, Swift extensions, and the one that actually provides the post title: how relying upon the right abstractions improves code quality and therefore maintainability.

The problem

The difficult solution

I hate (not) to make this about me, but I spend most of my time dealing with native clients that consume content from online video platforms (aka OVPs). In other words, I spent an awful lot to time building lists of Movies, TV Shows and whatnots.

For this particular difficult solution, I need to display a list like this:

The list contains two different kinds of “stuff”: movies and tv shows. To make things a little bit more (opening air quotes) interesting (closing air quotes), both Movies and TVShows are objects modelled as good old Objective-C classes, and I am not allowed to modify the source code of any of those two classes.

Ugly, yes, but Marines don’t do, Marines make do, and so on and so forth.

Now, seriously, this is more evidence supporting a concept I am trying to get across in almost every post. Start with something simple, something that gets the job done. Then look at it carefully, with a critical eye, and you will see all the code smells and bad design decisions jump off the screen, right towards you.

What’s wrong with that? Why is that ugly?

Here is the culprit: isKindOfClass. If you write Java for a living, it would be instanceof. In Swift, it would be the is operator.

YMMV, but I am going to say it as clear as I can: checking for a type at runtime is a code smell. A nasty, stinky code smell.

I know, in theory, there is a reason why isKindOfClass, instanceof and similar are part of the frameworks. In practice, I still have to find myself in a situation where relying on checking the actual type of an object does not stink, and I still have never found myself in a situation where checking the type at runtime was the best possible solution.

How can this be solved in an elegant way?

Well, as usual, taking a step back, and looking at the big picture. What is it that I am trying to achieve? Displaying Movies and TV Shows in the same listing, right?

Yes, and no. What I actually want to do is list a collection of strings. I should not care about what is actually producing those strings, about what it actually is, I should only care about how it behaves.

That is the key concept here: your abstractions would not focus on what things are, but on how things behave.

So, back to my example. If I shift from Movies and TVShows, to something-that-holds-the-data-needed-to-be-presented-in-the-listing, my listing would only have to use one entity, one abstraction.

Step 1. Declare a protocol.

I assume naming might be off. The goal is providing a method that returns the content that we want to display in the cells, already formatted properly as a single string.

Swift

1

2

3

4

protocolListItemPresenter

{

varcellContent: String{get}

}

Step 2. Declare a couple of extensions to Movie and TVShow

Extensions can be used to enforce protocol compliance. This is what The Book has to say about that:

Extensions add new functionality to an existing class, structure, or enumeration type. This includes the ability to extend types for which you do not have access to the original source code (known as retroactive modeling).

That is just what we did. Without touching Movie and TVShow, we enforced them to implement a protocol. Even better, notice how Movie and TVShow are written in Objective-C, while the extension is Swift. Neat!

Step 3. Implement the listing data source in Swift.

This is necessary. Only Swift code can use the new features in Swift (like Generics or Extensions). So this one is kind of a no brainer. But Swift is fun, so I call that a win-win!

Oh, noes! Is this another rant about semantic code?

Actually, no, it is not, thanks for asking. I would not say it is a rant, I would say it is a more like unwanted advice, but about something else.

The offending code.

This code, slightly edited to protect the innocent, was extracted from an actual codebase my team has to maintain.

Java

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

importjava.util.Map;

importjava.util.HashMap;

classSmartClass{

publicstaticvoidsmartMethod(Map<String,String>metadata){

for(Map.Entry<String,String>entry:metadata.entrySet()){

if(entry.getKey().equals("key1")){

/* Do whatever you need to do with the value

that corresponds to key1

*/

System.out.println(entry.getKey()+" - "+entry.getValue());

}

if(entry.getKey().equals("key3")){

/* Do whatever you need to do with the value

that corresponds to key3

*/

System.out.println(entry.getKey()+" - "+entry.getValue());

}

}

}

}

classSurprise{

publicstaticvoidmain(String[]args){

Map<String,String>metadata=newHashMap<String,String>();

metadata.put("key1","value1");

metadata.put("key2","value2");

metadata.put("key3","value3");

SmartClass smartClass=newSmartClass();

smartClass.smartMethod(metadata);

}

}

There are plenty of WTFs here. But the one that stands out the most is on smartMethod: why on earth would I loop all the entries in a Map only to act upon those that match a set of constant, immutable, known keys?

Isn’t our life complicated enough, as it is? Why do we want to make our code more difficult to read and understand, for the shake of a flexibility that may or might not be necessary? A flexibility that, at least in this example, is not even flexibility at all?

Do the simple thing first.

Instead of looping the map keys looking for something we already know is there… why not just ask for it?

Java

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

importjava.util.Map;

importjava.util.HashMap;

classSmartClass{

publicstaticvoidsmartMethod(Map<String,String>metadata){

/* Do whatever you need to do with the value

that corresponds to key1

*/

System.out.println("key1"+" - "+metadata.get("key1"));

/* Do whatever you need to do with the value

that corresponds to key3

*/

System.out.println("key3"+" - "+metadata.get("key3"));

}

}

classLeastSurprise{

publicstaticvoidmain(String[]args){

Map<String,String>metadata=newHashMap<String,String>();

metadata.put("key1","value1");

metadata.put("key2","value2");

metadata.put("key3","value3");

SmartClass smartClass=newSmartClass();

smartClass.smartMethod(metadata);

}

}

Now, if you look at that code carefully, if you look at any piece of simple, dumb code, carefully, you will instantly hear it screaming at you, shouting everything is wrong with itself.

In this example, the smartMethod has an expectation on which keys in a Map it will need to use. That tells you that a Map is not the right type for that parameter: maybe two strings would do?

Summary

Keep it simple. Do not try to impress anyone. Seriously, your skills as developer are not directly proportional to how obscure your code is.

What on earth is Dependency Injection?

Every time I have to explain what is Dependency Injection, I can’t avoid picturing myself as one of those cheap comedy characters, torn between two little instances of myself siting on my shoulders, one wearing all white, the other one wearing all red.

The one in white is whispering to my ear “provide some background, start the Dependency Inversion Principle”. The one in red,though, will be jumping up and down, shouting “tell him about passing everything your class might need in the constructor”.

Yeah, I have a rich complex inner life.

The Dependency Inversion Principle

Let’s do the right thing, and start from scratch.

A. High-level modules should not depend on low-level modules. Both should depend on abstractions.

B. Abstractions should not depend on details. Details should depend on abstractions.

There you have it, the Dependency Inversion Principle in all its glory. Obviously, if you throw that to anyone’s face, you should expect at least some eye-rolling, if not some head banging (them banging your head against the nearest wall).

In a nutshell, what the principle states is that both high and low level components should depend on the same abstraction. More about that later…

Riiiiiight, but what is Dependency Injection?

Basically, it is a pattern. Not one of the patterns in the GoF, but a pattern nonetheless. A pattern employed to facilitate that both high level and low level components depend on the same abstraction, aka the Dependency Inversion Principle.

Riiiiiiight, would you mind providing an example, smartass?

First, no problem. Second, mind you colourful language, please.

But you have a point. Here is an example of a very simple multi-layer app. There is a service layer that performs operations against an specific service (a service might be an actual remote service, or just a file on disk, let’s assume for the shake or the argument that, in this case, it attacks a remote service), and a business layer on top of it that provides a domain-specific API.

Swift

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

/*

* Service Layer. This is the lower layer in the app's

* layer design. It knows about the specific way data

* is stored in a particular service.

*/

class ConfigurationService {

func getValue(key: String)->String {

//attack configuration service and return the value

return "valueForKey" + key

}

func getValues(keys: [String])->[String] {

//attack configuration service and return the value

return keys.map({return "valueForKey" + $0})

}

func getValue(key: String, defaultValue: String)->String {

//attack configuration service and return the value

return key

}

}

/*

* Business layer. This is the top layer, and provides

* semantic, domain specific APIs. It uses the

* ConfigurationService

*/

classConfigurationManager{

private final letservice=ConfigurationService()

funcgetAuthenticationEndPoint()->String{

//Do some business logic, like checking caches and wahtnot

returnservice.getValue("authenticationEndPoint")

}

funcgetLocalCurrency()->String{

//Do some business logic, like checking caches and wahtnot

returnservice.getValue("localCurrency")

}

funcgetIconURLs()->[String]{

leticons=["menuIcon1","menuIcon2","menuIcon3"]

returnservice.getValues(icons)

}

}

Quite straightforward, right? Indeed. The problem here is, what happens if we need to change the way the service layer works? What if we want to grab the configuration from a file on disk, instead of a remote service, or even worse, decide what to do at runtime?

Yeah, we are in a bad situation. Why? Because there is a hard dependency here. The ConfigurationManager depends directly on the ConfigurationService. Those two classes are tightly coupled, the manager can not work without the service.

Sweet. You are good at describing code smells. What about a solution, please?

Sure, no problem. Dependency Injection to the rescue! No, wait! Dependency Inversion Principle to the rescue!

First, I will try to find an abstraction both the manager and the service can depend upon.

Swift

1

2

3

4

5

6

7

8

/*

* The abstraction everyone will rely upon

*/

protocolConfigurationService{

funcgetValue(key:String)->String

funcgetValues(keys:[String])->[String]

funcgetValue(key:String,defaultValue:String)->String

}

Second, I will make my service implement the ConfigurationService protocol.

Swift

1

2

3

4

5

6

7

8

9

10

11

12

13

14

15

16

17

18

19

20

21

22

23

/*

* Service Layer. This is the lower layer in the app's

* layer design. It knows about the specific way data

* is stored in a particular service.

*

* It implements ConfigurationService

*/

classLocalConfigurationService: ConfigurationService{

funcgetValue(key:String)->String{

//attack configuration service and return the value

return"valueForKey"+key

}

funcgetValues(keys:[String])->[String]{

//attack configuration service and return the value

returnkeys.map({return"valueForKey"+$0})

}

funcgetValue(key:String,defaultValue:String)->String{

//attack configuration service and return the value

returnkey

}

}

Now here comes the part where we all facepalm in sync, because suddenly, the concept “Dependency Injection” makes sense. The Dependency (in this example, remember, it was the ConfigurationService) will be injected into the ConfigurationManager as a parameter in the constructor. But wait, there is more! The ConfigurationManager will not hold a reference to an specific dependency, but to an abstraction (the Configuration interface).

Swift

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

/*

* Business layer. This is the top layer, and provides

* semantic, domain specific APIs. It is not aware of

* any specific class, but of an abstraction of the service

* (in this case a protocol)

*

* Note how the reference to the service is "injected"

* in the initializer

*/

classConfigurationManager{

private final letservice:ConfigurationService

init(service:ConfigurationService){

self.service=service

}

funcgetAuthenticationEndPoint()->String{

//Do some business logic, like checking caches and wahtnot

returnservice.getValue("authenticationEndPoint")

}

funcgetLocalCurrency()->String{

//Do some business logic, like checking caches and wahtnot

returnservice.getValue("localCurrency")

}

funcgetIconURLs()->[String]{

leticons=["menuIcon1","menuIcon2","menuIcon3"]

returnservice.getValues(icons)

}

}

This last bit is important, so I am going to repeat it. The ConfigurationManager depends on an abstraction now. It is not aware of which actual service it is using, and it actually does not give a damn about it.

Summary

Dependency Injection decreases coupling, and improves the flexibility and modularity of all designs. And it is a neat term that will make you look cool if you use in front of your colleagues.

Today we are going to go through one of the five tools that every Object Oriented Designer, or just anyone not wanting his code to become a loaded gun pointing at her head, should have in her toolset: the Open/Closed Principle.

The Open/Closed Principle is one of the most mentioned, but yet most misunderstood, of the five SOLID principles. I am sure you are fed up with reading the definition all around the interwebs, but here it is anyway:

Software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification.

Say what!?

Open for extension, but closed for modification. Neat. But, what does that mean? Let’s rephrase the Principle a little bit:

We should be able to modify the behaviour of a piece of code without changing the actual code that implements it.

In other words, we should have magic powers, or superpowers, or be able to bend the space-time continuum.

Having superpowers would be neat. Can I have some?

Sure, it would be neat. But it is definitely out of the scope of this post.

Let me rephrase. How can my code be open for extension but closed for modification?

Let’s go through a real life example. Real life, like in “code extracted from an actual codebase my team has to maintain”. Let’s pretend that the fact that I personally wrote the offending piece of code does not matter.

The problem

I mostly work with OVPs (that’s Online Video Platforms). I build those pesky apps that some TV providers deliver to their subscribers, and allow said subscribers to watch TV on their devices.

An Electronic Program Guide is a list of what is going to be broadcasted during a particular range of dates/times. Let’s pretend this is what I need to display:

Bear with me on this one, please.

As you can see, the list needs to display Movies, TV Shows, and Live events. When the user taps a particular Program, the app needs to transition to a “details” view, which is, obviously, different (different content and different layout, and even different business logic) according to the type of Program the user selected.

For historical reasons, and some practical reasons, the model object that was provided with in order to build that listing looks similar to this:

Swift

1

2

3

4

5

6

7

8

9

10

11

12

13

14

15

16

17

18

19

20

/*

The backend provides a Program object in the services response.

Also, it provides a "type" that identifies the type of program.

*/

enumProgramType{

caseMovie

caseEpisode

caseLiveProgram

}

// Simplified Program, for the shake of the example.

classProgram{

private(set)vartitle:String

private(set)vartype:ProgramType

init(title:String,type:ProgramType){

self.title=title

self.type=type

}

}

Yes, you can argue than all my troubles will end if I looked at the right abstraction, and if I did not need to rely on that “type” property. And I would agree with you: remember, there are historical reasons behind this. Anyway, all this “look at the right abstraction” thing is probably matter of another post.

But, on the other hand, relaying on the right abstraction does not change the fact that I need to navigate to different views according to the type of content I need to display.

The simplest possible solution.

I am a big fan of keeping things simple. When you keep things simple, code smells, bad designs, and bad solutions just popup in front of you when you look at the code, jumping out of the monitor, waving their arms, screaming “refactor me, refactor me!”

So, the simplest implementation that would get the job done would be:

Swift

1

2

3

4

5

6

7

8

9

10

11

12

13

14

15

16

17

18

19

20

21

/*

Function to be called when the user taps a program.

This shall launch the a different view controller according

to the content that needs to be displayed.

*/

funcnavigateToDetails(program:Program){

switchprogram.type{

case.Movie:

println("It is a movie, navigate to Movie Details")

case.Episode:

println("It is an Episode, navigate to Episode Details")

case.LiveProgram:

println("It is a LiveProgram, navigate to Live Program Details")

}

}

letsampleMovie=Program(title:"The Quiet Man",type:.Movie)

letsampleEpisode=Program(title:"CSI: S47E03",type:.Episode)

navigateToDetails(sampleMovie)

//navigateToDetails(sampleEpisode)

Riiiiiiiight, a switch.

Switch is bad

No matter what they tell you, no matter what you see, switch clauses are usually a code smell. And in this particular case, ermahgawd, it is just bad. I wrote that code, and I still want to punch myself in the face overtime I see it. But why?

Well, this will not escalate well. First, and most obvious, Program is poorly designed, it is the wrong abstraction, and to workaround that bad design, it exposes a type to help identify what it actually represents. That is clearly meant to change.

But, the worst is about to come. What if I need to deal with a new type of program? I would need to update that switch. There is no way to extend this code that does not imply editing the navigateToDetails function.

This code is not open for extension and closed for modification.

Again, the simplest possible solution clearly exposes two code smells: first, Program is the wrong abstraction, or at least it is poorly designed, and second, the initial design is not easy to extend.

The better solution, open for extension and closed for modification

Let’s assume that Program is not going to change now. It will, but not at this precise moment.

If you look at the first implementation of the navigateToDetails function, you will see how all the business logic that deals with navigation is in there. That function checks the program type, and launches the command needed to navigate to the desired destination. What I want, is find a way to delay those decisions as much as possible. That way, I can deal with the problem in a generic way here.

The first step to fix my problem (remember, I want to be able to navigate to different views according to the type of Program the user selects) starts with declaring a protocol that abstracts the details of how I want to perform that navigation.

Swift

1

2

3

4

5

6

// Protocol that abstract the behaviour:

navigating tothe view that contains the Program'sextended info.

protocolRouter{

funccanHandleProgram(program:Program)->Bool

funcnavigateTo(program:Program)

}

Then, I will have different implementations of that protocol. Each of those concrete implementations will deal with the details of how to navigate to the desired destination, and more importantly, they will provide a way to decide if that particular implementation is supposed to handle a Program or not.

Swift

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

// Implementation of the Router protocol that navigates to the

Movie extended info

final classMovieRouter: Router{

funccanHandleProgram(program:Program)->Bool{

returnprogram.type==.Movie

}

funcnavigateTo(program:Program){

println("It is a movie, navigate to Movie Details")

}

}

// Implementation of the Router protocol that navigates to the

Episode extended info

final classEpisodeRouter: Router{

funccanHandleProgram(program:Program)->Bool{

returnprogram.type==.Episode

}

funcnavigateTo(program:Program){

println("It is an Episode, navigate to Episode Details")

}

}

// Implementation of the Router protocol that navigates to the

LiveProgram extended info

final classLiveProgramRouter: Router{

funccanHandleProgram(program:Program)->Bool{

returnprogram.type==.LiveProgram

}

funcnavigateTo(program:Program){

println("It is a LiveProgram, navigate to Live Program Details")

}

}

That last part is the key of the whole refactoring. Notice how I can inject (or in less fancy words, pass as a parameter) a list of all the available implementations of the Router. My navigateToDetails function will loop those, passing them the Program. If it finds any implementation that can deal with the Program passed as parameter, it will ask the implementation found to proceed and navigate to… where? That’s the thing! The navigateToDetails function does not know where, because it doesn’t need to know.

Delay all decisions!

I recall reading, some time ago, that good Object Oriented Design consisted in finding ways to delay decisions. I only agree with that up to a point (I guess I have been a victim of over-engineered systems more than once), but I think it makes a good rule of thumb. If a particular part of your system knows too much about others, maybe it is time to rethink it.

But I digress. The whole point of this post was implementing a solution to a problem in a way that allows easy maintenance, and simplifies scalability.

Summary.

Object Oriented Design is all about trade-offs. Simple code usually gets the job done, but sometimes simplicity in the code brings along poor maintainability. Sometimes, ten lines of code get the work done, but sometimes you need to substitute those ten lines by a hundred lines, as long as those hundred lines provide a solid, clean and maintainable solution.

And, did I mention that unit testing the second solution is way easier?

Code samples.

You can get a couple of Swift playgrounds containing the source code to this post on Github

Code reviews are awesome. Seriously, if you are not doing code reviews you should start doing them right away. There are many good reasons for doing so.

Code reviews are a learning opportunity

When done properly, code reviews are awesome learning opportunities for junior developers. There is no better way of growing as a developer, than having someone well seasoned in front of you, explaining to you, line by line, how to make your code better.

But wait! Code reviews are awesome learning opportunities for senior developers as well. You know that saying “an old dog does not learn new tricks”, don’t you? Well, there is nothing better for a senior developer than a newcomer to the craft asking all kinds of questions about a codebase. If makes you reconsider each and everyone of your decisions, and most times, it makes you realise how many things you do that you should probably not be doing. And how you do those things without any particular reason.

Code reviews help improve the code quality

This should be obvious, but apparently it is not. The main goal of a code review is being sure that the code quality is up to the team’s standards. Now, what are those standards might be a theme for another post (or a thousand posts), but the key concept here is that opening up the code for review, everybody gets to read it, discuss it, and improve it through suggestions and comments. Which leads to…

Code reviews enforce collective ownership of the code

You have probably heard it a thousand times: code should be owned by the whole team. There is no better way to make the team feel like the code is owned by everyone than making everyone actually own the code. And that happens when everyone’s suggestions and comments are incorporated into the code base. More importantly, opening up the code for review avoids what I call dark zones, chunks of code that none has actually seen, and that no one knows what they actually do or how they actually do it. Again, collective ownership FTW!

Code reviews help share knowledge about the code base

Here comes Captain Obvious! Yes, if everybody is supposed to read everybody else’s code, understand it, and being able to comment about it and suggests improvements to it, as a side effect, that means that everybody understand the whole code base. And that is awesome!

Code reviews avoid nasty surprises.

We have all been there. James did work on the new feature, and his code was complete. Until we tried to integrate Jame’s feature into the new release. That’s when we found that, apparently, James has a different definition of “code complete”. Code reviews will rise to the surface issues with the code as early as possible. If the feature is not implemented as it is supposed to, a good code review will catch that.

Code reviews motivate developers to write their best code

If you know that everyone else is going to read your code, you are going to find that extra pinch of motivation to avoid duplication, or to come up with better names for your methods and variables, or to break down that very long method into smaller methods.

Code reviews create a team’s culture

Yeah, the company culture. That thing every company on the face of earth wants to create, that thing that is supposed to act as a mesh, as a framework, that supports all the engineers, that binds them together, that makes easier for new hires to catch up and blend into their teams. A healthy system of code reviews makes wonders when it comes to create that. There is no better mentoring for a new developer that his first pull request open for review. There is no better way to make that new hire feel as he is growing as a developer than receiving continuous feedback about his code and being able to provide feedback to his colleagues about their code.

All that is cool, and everything, but how can you make sure that everybody understand code reviews as all of the above?

Well, I am very sceptic about this whole Scrum and Agile Cult. But there is something in that process that I really like: the idea that teams should be self-organised. Give a team the opportunity to figure out how and why they want to do their code reviews, and they will come up with something good. Good, I say, because it will be something they all will stand by, and thy will all abide by.

That is what my team did, And this is what we came out with (edited to remove some company-specific items)

Code reviews: rules and regulations

Rule #1

Code reviews are a learning opportunity, both for the person opening up his code for review, and the persons doing the actual review, and they should be approached as such.

Rule #2

Code reviews are done for one reason: deliver the best possible code.

Rule #3

Comments in the code reviews shall be performed with rules #1 and #2 in mind.Corollary 1: Comments in the code reviews should be backed up by arguments and facts, not by personal preferences.Corollary 2: Refrain from adding comments that provide no value. If a single comment can achieve the same result of 10 comments, it is better to make that one comment vs. 10 different comments in 10 different places.

Rule #4

The result of a good review should provide:
– A list of things to update. There should be a clear definition of what to update.
– A list of things that require further discussion.

Rule #5

In order of importance (descending) a review should focus on:
– Global intent of the code (e.g. does it do what it is supposed to be doing?)
– Architecture (e.g., does it follow the technical design?)
– Gaps in the unit tests (e.g., does it cover all the requirements?)
– Code readability (e.g., does it conform to code clean guidelines?)

Corollary: While code style is also an integral part of code readability, it is not considered an important aspect of the code review unless it blatantly violates the agreed upon standard.