What is the best practice? And, should I care?

Is there ever a “right way” to do something?  Or is it all a matter of preference?  Should we be dictating that code is written a certain way, and that any other way is wrong?

I am not proposing that we rid ourselves of the “shackles” of coding standards, or that we throw out everything we have learned about patterns that work, and those that don’t.

However, I think that we often miss the important part of best practices.  And that’s the why.

Story time

I was recently talking with one of my coworkers about some code and he needed to create an if statement that only had one statement in it.  As I watched him type it in, I wanted to scream!  He had left out the curly braces.

I restrained myself and calmly told him that he should add in the curlies.  I explained that it is perfectly legal without them, and the reasons I like to add them (readability and makes logic errors later less likely).

He said he had been wondering whether he should use them or not.  He had seen some code without and thought that was a “best practice”.

The Moral?

When we have coding standards (which I think we should) or best practices, we should make sure to explain why we want the code that way.  It is not always obvious.  When the developer knows the why, he/she is much more able to determine when to break these standards/rules/practices (because sometimes it is best to do so).

 

Advertisements

Design Patterns: DRY

A while ago, I was working with a younger man and as I was doing a code review, I came across some code that looked kind of like this:

readColumnA () {
      columnA = columnReader("columnA");
      processColumn(columnA);
}

readColumnB () {
      columnB = columnReader("columnB");
      processColumn(columnB);
}

It isn’t really important what was in the file, or what the processing being done was.  These two methods are basically identical, the only difference is the value passed to columnReader.

I suggested that he change this code to look more like this:

columnProcessor (String columnName) {
      column = columnReader(columnName);
      processColumn(column);
}

Then, you could call it for each column you need to process.

When you remove duplication like this you are making your code DRY.  DRY stands for don’t repeat yourself.  In the first example you can see that we repeated the algorithm for processing the columns for each column we needed to process.  We can see that the only thing that changed was the column name, and extract that part out, and then have a generic method to process columns.

My colleague had heard of DRY, but thought that he shouldn’t use it here since he was pretty sure that we would change and want to process columnA differently than columnB in the future.  While this argument makes a certain amount of sense, we need to remember that we are not psychic, and while we can speculate on how a software project might evolve, we are never really sure.  The requirements and priorities change under our feet, and our guesses are often wrong.

In my opinion, and in my experience it is always better to keep your code clean because you want to be able to make changes easily.  If, at some point, in the example above, column A and column B processing diverge, then (and only then) you should change the code to accommodate this.

 

Code Reviews

The idea of code reviews is fairly well known.  Many businesses practice them regularly, many others agree that it is a good idea.  But, there aren’t a lot of resources out there to help a business understand how to do good reviews.

Why do them?

Let’s start off with a quick review seeing as most of us understand why.

The most common reason people state for doing code reviews is to catch bugs.  When you have a second set of eyes look at a piece of code, they may see some edge cases that you did not account for, or see other silly mistakes that we all make.  This is important, but I think there are other very important benefits of doing code reviews.

Code reviews are an excellent learning platform for both the developer and the reviewer. It is pretty clear why code reviews are a learning opportunity for the developer. The developer gets a chance to gain feedback from his/her colleagues on the code they produced, which sometimes reveals new techniques and new “best practices” that they may not have known.

However, perhaps less clear is why this can be a learning opportunity for the reviewer.  All of the benefits that the original developer gets from having someone review their code can be applied to the reviewer as well.  They may see new things they have not encountered before.  Additionally, the reviewer gets an opportunity to learn about the new feature that was added.  This leads to shared understanding of the code, and greater collective code ownership.

Code standardization can also be a benefit of code reviews, if you decide to have people review on this.  There are a lot of tools that can help make sure you are applying coding standards as well.

Catching defects at this stage will allow us to fix them with minimal effort.  Sometimes a design defect will be built upon and then it could impact a much wider part of the system so when we go back to fix it, it may cause ripple effects throughout the code-base.

What should we review?

There are a lot of discussions about what should be reviewed in a code review.  One that often comes up is coding standards.  As I said above, there are a lot of automated tools that can help ensure that standards are being applied across the board, but they won’t catch things like bad method names or bad variable names to name a few.  I think this is valuable input.  Since many of these “violations” are easily fixed, I think it is reasonable to ask that they are fixed before the task is considered complete.

An obvious think to look for in a code review is bugs.  If you see a bit of code that may cause an error in certain circumstances, please call it out, even if you aren’t sure.  It is better to have the conversation about the piece of code and decide that it is fine than it is to let it slide and later find out that it was an issue all along.

Lastly, if you see some code that could be improved, I think it is acceptable to mention this in your review.  Those changes may not need to happen at this time, but it is a learning opportunity.  If there isn’t time to address the refactor (and people agree that this is a good change), make sure to note it down somewhere (preferrably your ticket tracking software).

Who should review?

I hear this one quite often. I think any developer that might touch the software in question should be involved in the review.  I often hear pushback that this will take too long for the indivdual developers, or that the review might sit out there too long and we will not be able to merge the code.

Doing the code review before the code goes into production may take a little longer upfront, but there are a lot of benefits (as I described above) to doing so.  We can potentially catch errors before we start building on top of them and relying on the faulty code for other parts of the system.

As for the review sitting out too long, there are lots of things that you can put in place to ensure that this doesn’t happen.  Daily stand-ups (or at the very least team check-ins) allow the team to mention that a code review is waiting for reviewers.  You can set up rules for how many people need to review and approve before the code is merged so that you don’t have to wait for the entire team.  Generally speaking, this is usually not an issue because the team wants to get work done.

What about pairing?

In many cases pairing can take the place of the traditional code review.  You get two sets of eyes on the code as it is being written, which can avoid many of the issues we are trying to catch in a code review.

However, I would recommend that code reviews are good practice when pairing especially in a few key circumstances:

a) When the pair is two junior developers.   A more experienced set of eyes might be able to shed some light on potential issues the junior pair would not have known to look for.

b) The new feature is very complex.  If a new feature is very complex having additional eyes on it is a good thing.  Doing this could catch potential bugs the developers did not see, but it also helps the entire team to understand the new code.

How should we review?

This is probably the most important part of this whole article.  We are all a little scared of the code review.  We don’t want to look dumb or bad in front of our colleagues and showing our work can be a little intimidating.

I have heard a couple of really good pieces of advice on this front.

  1. Always assume best intentions.  This goes for the reviewer and the reviewee.  The person who wrote the code is trying to do the right thing…perhaps there is a good reason it is written the way it is.  The person doing the reviewing is also trying to do the right thing, tone doesn’t carry well in written language.
  2. When you are commenting on a piece of code, be nice.  Use “we should” instead of “you should”.
  3. Ask questions about the code rather than accusing them of doing something wrong. “Why did you do this?  Would this other thing have been better?”
  4. Talk to the person face to face.  Sometimes a written discussion leads you in circles, or inflames one or both parties.  Have a face to face if there is an issue that you are unable to resolve.

 

High Cohesion

When I first started learning about object oriented programming, and really digging into the best practices, loose coupling and high cohesion kept cropping up.  I had a hard time trying to keep them straight and remember which one I wanted more of and which one I wanted to reduce.  My biggest difficulty was I couldn’t keep the definitions of coupling vs cohesion straight!

Coupling refers to how much a set of classes rely on one another.  If we have two classes, A and B that each use methods from the other, these classes would be tightly coupled.  If we have those same two classes and only one needs methods from the other, then those are more loosely coupled.

The little metal ends on a garden hose that you can twist together are called couplings.  You use them to join two pieces of hose together.  I like to use this to help me remember that coupling is about how two (or more) classes interact with one another.

Cohesion refers to how much the methods in a particular class belong together.  For example, let’s say we have a class representing a soup can.  The soup can should know about how big it is, what it is made out of, what shape it is and what color it is.  If we start adding information about the soup that is stored inside the can, then we are breaking the cohesion.  Information about the soup is not important to the actual can.  The information about the soup should be contained in a separate class that the can could know about.

Thinking about a cohesive group of people helps me remember what this means.  A cohesive group of people are people that work very well together and really seem to belong together, much the same way as a cohesive class design has methods that really belong together.

I have another post about coupling here.

High Cohesion

When we talk about cohesion, we are really talking about how well the ideas in a class or a data structure belong together.  I mention data structure  here because this principle can and should be used when designing database tables, or any data storage scheme, as well as when you are designing a class.  We are gong to focus on cohesion in code in this article.

Now that we have a better understanding of what cohesion means, why is it important?  Why should we worry about creating things that are really cohesive?  The program will work even if we throw a bunch of different concepts into one pile, right?

Technically, yes.  It is possible to write an entire application in one file.  And…you might even be able to hold all of the context of the application in your head at once while you are writing it.  Just because you can , doesn’t mean you should.

Why not put it all into one class?

There is a term for a class that ends up knowing too much about too many things…”god class”.  This is not the good kind of god, it is the spiteful, vengeful type of god, and you really don’t want to go creating them.

When you have a class that knows too much about too many things, it makes changing that class REALLY dangerous.

When a class knows too much, and does too many things, making a small change can impact many parts of the application, since it is likely used there too!  When we break things out into small classes, those classes are often used in fewer places, and their methods are very well defined.

Also, when everything is in one class, there is often a fear of touching that class.  We cannot understand everything that it is trying to do, so we don’t want to make a change that will cause unknown issues.  When we are afraid to touch classes, we don’t try to improve them, we just try to patch the hole and get out as quickly as possible.  This is not good for the overall health of our application, and is not good for our overall mental health.

 

Single Responsibility

High cohesion and the SRP (Single Responsibility Principle) go hand in hand.  When you design your classes, they should have one main purpose, one main reason to change.

Breaking your classes down this way not only means that each individual class will be changed less often, but it also helps us humans reason about the system as a whole.  Often systems are very complex, and contain a lot of concepts.  If we can break down the ideas into smaller and smaller parts, we can more easily understand these tiny parts, and can then build up our mental model much more easily.

 

Bad Code – Snippets from production

True, false … or maybe?

Lets start with something on the funny side. Some of the code that I have come across in our system.

boolean isFutureDeal = Boolean.parseBoolean(request.getParameter("loadFuture"));
if(isFutureDeal != true){
       isFutureDeal = false;
}

If it’s not true, set it to false. I guess this is to make sure it isn’t set to that other boolean value – true, false, maybe.

Give me that.  What?!  I don’t need that.

public ShoppingCart createNewCart(ShoppingCart cart, HttpSession session,
                     MiniUserBean member) throws Exception {

              if (cart == null) {
                     session.setAttribute("shoppingCart", new ShoppingCart());
                     cart = (ShoppingCart) session.getAttribute("shoppingCart");
                     cart.setCurrentRedemptionType(member.getRedemptiontype());
              }
              cart = (ShoppingCart) session.getAttribute("shoppingCart");

The cart object is passed in, but the first thing we do is read it from the session, so why even pass it?

Day of the week calculation

[pseudo-code...actually seen in production]
if (date == "11/28/2016" || date == "12/5/2016" || date == "12/12/2016" 
     || date == "12/19/2016" ...) {
          dayOfWeek = "Monday";
} else if ( date=="11/29/2016"...

You get the idea.  There were about 1/2 year’s dates in the if statements.  I found out about this gem when a customer called and complained that their script was not setting the day of the week any longer.  This was in perl, and I replaced that huge block with a few lines that would forever compute the day of the week.

Code Coverage

We all want to achieve 100% code coverage.  If we have 100% of our code executed in tests, we can feel confident that we are testing our code well. Right?

Not so fast!

We wanted to share some examples of tests that, while they may boost your code coverage, aren’t testing much.  Below are some examples we have actually seen (code has been changed to protect the guilty).

Trust, but verify

This is a code snippet using Mockito.

public void testSomething() {
   classInTest.doStuff();
   verify(classInTest).doStuff();
}

For those of you unfamiliar with what verify does in Mockito…it will pass if the method in question was called.  That’s right…this test is verifying that we called the “doStuff” method…which we did in the previous line.

Unfortunately for us, our code coverage tools will look at this and see that we executed lines inside the doStuff method and count them as covered, even though we didn’t actually test anything!

Preparation is key

public void testStuff() {
  when(thing.doStuff()).thenReturn("hi");
  when(otherThing.doStuff()).thenReturn("there");
  .... lots more setup ....

  classInTest.methodInTest();

}

That’s right…they took the time to do a whole bunch of setup and even called the method in test…but forgot to actually test anything!  There are no asserts or verifies.  Again, our helpful code coverage will show that lines inside of methodInTest were covered since they were executed in a test, even though we didn’t test a single thing.

Green means Go

public void testTest() {
  ... improper setup ...
  ... call method ...
  ... assert the wrong thing ...
}

Sometimes we make mistakes in our setup code and when we run the test, it fails, even though we are sure that the code is working correctly.  That leaves us with two options.  1) Figure out why our test is failing…step through, debug, print stuff out, etc.  See if it is a bug in the production code that we hadn’t noticed or if there is an issue with our test.  OR 2) Change the expected values in our test.

Option 2 is obviously the easier answer.  Yes, we’ve seen this.  No, this is not correct.  Tests are supposed to give you confidence that your code is working correctly.  It can also act as a sort of documentation for future developers.  Do you want future coders to look at the test and think that what you are testing is expected behavior?

What have we learned?

Code coverage is not actually telling the whole story.  As we have seen above, if you have poorly written tests lines of code covered don’t mean anything.  Anyone can be clever and find ways to execute more code in their tests…the smart programmer figures out how to actually test the code.

Please be vigilant in writing your tests and actually ensuring that your code is doing what you expect it to.  When doing code reviews, if you see any blunders like we show above, or other not so great tests, please help the other person write better tests.  You, your team and your code will be better off for it.

photo credit: <a href=”http://www.flickr.com/photos/62975503@N04/8227882239″>Facepalm</a&gt; via <a href=”http://photopin.com”>photopin</a&gt; <a href=”https://creativecommons.org/licenses/by/2.0/”>(license)</a&gt;

 

Hiding utilities behind a facade

Recently, at work, I got into a passionate discussion about whether we should hide things like StringUtils behind a facade.

I was helping a coworker add some tests around a monstrous method and noted that it used StringUtils static methods quite a few times in the method.  Initially I thought that this was a utility class that we had written, so rather than changing the class to have non-static methods, I opted to try to hide it behind a facade (in this case it was a simple pass-through object, but the methods on this object were non-static, thus allowing me to mock it with Mockito).  Upon investigation, it turned out we were using commons.lang StringUtils.  Still, I thought it might be good to put it behind a real object so we could mock it out.

I explained to my colleague that the reason I wanted to do this was so that we didn’t have to set up our tests with realistic data.  We could pass in very fake data and we could make sure that we were only testing the method in question (which was VERY important since it was a behemoth).  By putting it behind the facade, we were able to make it return whatever we wanted.

We spent the morning getting this all set up and she was ready to start off on her own in the afternoon.  Later that afternoon, she was talking with the other developer (and our boss), who works in the code base in question regularly about the tests she was writing.  He did not agree at all about us mocking out StringUtils.

A very heated discussion ensued.  In the end, since I didn’t work in the code in question, I left it up to them how they wanted to test it.

After the discussion, I was reflecting on why I felt so strongly about mocking out StringUtils, and I think I have pinpointed some of the reasons.

  1. The method under test was doing a LOT of things, including calling out to StringUtils nearly a dozen times for different things.  By mocking out StringUtils I felt more comfortable that we were tackling the code that was in the method without having to worry about what StringUtils was going to do with the data we passed it.  I was trying to reduce the complexity of the method in question by ensuring I was using things I had complete control of.
  2. I saw StringUtils as a collaborator and wanted to make sure that I was testing only the method in question, and not the collaborating code.  I believe I saw it this way due to point 1.
  3. I knew that we had some other classes in the code base that had a bunch of static methods on them.  I would treat these the same way.  Rather than treating each different class with static methods differently based on some gut feeling, I wanted to be consistent and treat any class that offered static methods the same way.  This allows me to focus on the tests, rather than spending mental cycles on deciding whether this particular things should be mocked or not.
  4. Hiding a 3rd party tool behind a facade is a best practice I have heard many times.  It allows you to swap out the tool for a new one with little effort.  It can also make it much easier to upgrade to a new version, even if the new version has changed the API, since if everyone using it is actually using the facade, you only need to make the change in one place, and everyone else just continues using the facade in the same way as they always had.

 

 

Simply refactoring the monolithic method into smaller methods that could do each individual task would have left me testing it much differently.