Refactoring - How do I start?

According with the Software-craftsmanship legend Sandro Mancuso we should start:

• Refactoring from the deepest nested branch of the code and work our way inside out.
• Test from the shortest/less nested branch of the code and work our way from outside in.

If you break something you have to pay for it!

Many years ago in Spain(as a kid) I was probably in second or third year of primary school, I had to buy a gift for somebody's birthday.
All I had was 500 pesetas(a bit less than 3 euros) and I wanted to buy something cheap, but at the same time beautiful and also decorative.
I was walking around the neighbourhood and not far away from my house, I found a glass craft shop.
I went into the shop and started walking the corridors, there were all sort of glass crafts: from ashtrays to lamps... and everything was all over the place, not just in selves also there were dozens of crafts on the ground.
Quickly I noticed a little figure with a price tag that said 500, that was the gift I wanted to buy. Unfortunately It was in the ground placed at least 2 meters away from were I was, and there were lots of other glass crafts on the way to it. If I wanted to reach it all I could do was move away each of those other crafts that were on the way. I carefully started doing it until I've got the figure I wanted. When I was ready to go back and turned around the shop attendant was looking at me and said: "If you break something you have to pay for it!", I've got a bit scared and... well... is up to your imagination how this story ended.

This story reminds me a bit to the feeling that we all have sometimes when unit testing some code: "Is this really a unit test?".
Sometimes we think we tested correctly but if we are not extra careful with our unit tests we might unconsciously be creating a poisonous integration test.

Let's have a look at an example:

1:  public class ProductDiscountCalculator {  
2:       public void apply50PercentDiscount(Product product) {  
3:            ProductManager manager = new ProductManager();  
4:            if(product.isExpired()) {  
5:                 product.setPrice(product.getPrice() - product.getPrice() * 0.5);  
6:            }  
7:            manager.updateProduct(product);  
8:       }  
9:  }  

We want to unit test the method in the above class and this is what we do:

1:  public class ProductDiscountCalculatorSpecification {  
2:       private ProductDiscountCalculator calculator;  
3:       private Product sampleProduct;  
4:       @Before  
5:       public void arrange() {  
6:            calculator = new ProductDiscountCalculator();  
7:            sampleProduct = new Product();  
8:            sampleProduct.setPrice(10);  
9:            sampleProduct.setExpired(true);  
10:       }  
11:       @Test  
12:       public void priceDiscounted50Percent() {  
13:            calculator.apply50PercentDiscount(sampleProduct);  
14:            assertThat(sampleProduct.getPrice(), is(5D));  
15:       }  
16:  }  

When we run this test, it will go green, but... Is this really a unit test?
What about this line:

 manager.updateProduct(product);  

That line is very dangerous, maybe it is accessing a database and we are poisoning it every time we run our tests.
That's why we need to have extra careful and make sure that we use techniques to break dependencies, stubs, fakes, spies, mocks... or anything
that takes, to make sure at 100% that we are unit testing.
Lets see how we can break the ProductManager dependency and create a more testable alternative:

1:  public class ProductDiscountCalculator {  
2:       private ProductManager manager;  
3:       public ProductDiscountCalculator(ProductManager manager) {  
4:            this.manager = manager;  
5:       }  
6:       public void apply50PercentDiscount(Product product) {  
7:            if(product.isExpired()) {  
8:                 product.setPrice(product.getPrice() - product.getPrice() * 0.5);  
9:            }  
10:            manager.updateProduct(product);  
11:       }  
12:  }  

In the above example we extract the dependency and make sure that we can set it from outside of the class.  We could do it different ways I just used composition for this example.

1:  public class ProductDiscountCalculatorSpecification {  
2:       class ProductManagerStub extends ProductManager {  
3:            @Override  
4:            public void updateProduct(Product product) {  
5:            }  
6:       }  
7:       private ProductDiscountCalculator calculator;  
8:       private Product sampleProduct;  
9:       @Before  
10:       public void arrange() {  
11:            calculator = new ProductDiscountCalculator(new ProductManagerStub());  
12:            sampleProduct = new Product();  
13:            sampleProduct.setPrice(10);  
14:            sampleProduct.setExpired(true);  
15:       }  
16:       @Test  
17:       public void priceDiscounted50Percent() {  
18:            calculator.apply50PercentDiscount(sampleProduct);  
19:            assertThat(sampleProduct.getPrice(), is(5D));  
20:       }  
21:  }  

In what regards to the test I used the subclass and override technique to make sure that we are not poisoning a the database if the real object was passed. Since the object that we are passing to the constructor of ProductDiscountCalculator is just an stub, we are now 100% sure that this was correctly tested without poisonous integration tests.

How to verify that void methods were called using Mockito

You can also read this article in our brand new medium space. Click here!  Remember follow Javing on medium to make sure you don't miss out on some of the great new upcoming content.

Mockito is one of the most popular mocking frameworks for java.
This post Is just little miscellaneous where I will show you how to mock and verify a void method call.

Sometimes when we test a call to a void method all we want to do is just make sure that at some point in its life cycle, another method will be called with certain parameters.
Let's see the following example:

 public class SomeClass {  
      private OtherClass otherClass;  
     
      public SomeClass(OtherClass otherClass) {  
           this.otherClass = otherClass;  
      }  
      protected void firstMethod(int value) {  
           if(value > 0) {  
                secondMethod("Yes!");  
           }  
           else {  
                secondMethod("No!");  
           }  
      }  
      private void secondMethod(String value) {  
           otherClass.someMethod(value);  
      }       
 }  

In the above piece of legacy code the most important part is a method called "someMethod()". We must make sure that it is called in a proper way, but unfortunately it belongs to a dependency to which we have no access and also to make it more complicated it is inside a private method.

Mockito framework could help us mock and verify that method call.
Here is how we can do it:

 package demo;
 import static org.mockito.Mockito.verify;  
 import org.junit.Before;  
 import org.junit.Test;  
 import org.mockito.Mock;  
 import org.mockito.MockitoAnnotations;  
 import code.OtherClass;  
 import code.SomeClass;  
 public class TestClass {  
      @Mock  
      private OtherClass otherClass;  
      //Class under test  
      private SomeClass someClass;  
      @Before  
      public void prepareDependencies() {  
           MockitoAnnotations.initMocks(this);       
           someClass = new SomeClass(otherClass);  
      }  
      @Test  
      public void is_the_value_greater_than_zero() {  
           someClass.firstMethod(8);  
           verify(otherClass).someMethod("Yes!");  
      }  
      @Test  
      public void is_the_value_smaller_than_zero() {  
           someClass.firstMethod(-1);  
           verify(otherClass).someMethod("No!");  
      }  
 }  

The most important things to highlight in this test class are:

• The class under test is never mocked.
• The dependencies of the class under test need to be mocked.
• By calling a method on a mock object we will mock that method call
• By using the verify() method we will test that at some point the method from the mock was called  with the exact same parameters.
• The test class can access the protected method because the package name is the same.(But of course in your project structure test will be under src/test/java and production code under src/main/java)

The legacy code that whispered to the developer

I was practicing my legacy code refactoring skills a bit this weekend (Highly on demand in the industry lately hehe..  ;) ) when I came across this fascinating line of code:

 System.out.println("Process complete!!!");  

My first thought was: "Whatever, just another silly console output nobody cares about!"
As usual, I run my eCobertura code coverage plugin to check how I was getting on with the level of coverage.
That line was red, it didn't bother me much so I continued with the rest of my testing and I said to myself, "I will come back to it latter...".

I was almost done when my torrent software notified me that a movie just finished downloading.
Also I started to feel hungry...  I wanted to go make lunch and enjoy the movie. "Let's continue with the practice another day..." I said to myself.

After making a sandwitch I came back to my laptop with the intention of watching the movie, but my eclipse IDE was still open. And that line that eCobertura painted red, was still there looking at me:

Suddenly it started talking to me and we had a very interesting conversation:

Line 43(In a thretening tone): "Psss!... hey you!..."

Djordje: "Are you talking to me?"

Line 43: "Do you see somebody else in the room? Yes, i am talking to you" 

Djordje: "Who do you think you are taliking to like that?"

Line 43: "I am a piece of untested code in your software. If you are adding code coverage I also have the right to get some."

Djordje: "Why should I unit test you? You are just a piece of old legacy nobody cares about"

Line 43: "You are such a dumb ass, you don't get it!"

Djordje: "Get what? Leave me alone I want to enjoy my sandwitch and watch my pirate movie. Don't make an scene because I will close the IDE and I will not practice with this program never again"

Line 43: "Ok, ok... take it easy! I just want to help you. Please, let's make a deal."

Djordje: "What deal?"

Line 43: "Just spend 5 more minutes trying to unit test me and I will teach you something important."

Djordje: "Fine, just 5 minutes. This better be good, because I am very hungry"

Poor Line 43, probably it just wanted to talk to somebody. What can I learn about TDD from such a simple line of code?  Anyway I decided to make it's wish come true and paint it green with eCobertura. This is what I did:

   private PrintStream originalConsoleOutput = new PrintStream(System.out); 
   private final ByteArrayOutputStream outContent = new ByteArrayOutputStream(); 
   @Before  
   public void setUpStreams() {  
     System.setOut(new PrintStream(outContent));  
   }  
   @After  
   public void cleanUpStreams() {  
     System.setOut(originalConsoleOutput);  
   }  
      @Test  
      public void  
      when_the_process_is_complete_show_a_message  
      ()   
      {
           //..   
           statistics.process();  
           assertEquals("Processing completed!!!\n", outContent.toString());  
      }  

Basically  what I did was just asserting that the console output is the same that what the println() command has in its argument. The process() method is where Line 43 was, so its content will go to the console when statistics.process() gets executed. At the end the method @After will restore the console output to the original stream that was previously saved.

Djordje: "I painted you green. Are you happy now?"

Line 43: "Thank you very much!....Thanks, thanks.... I am so happy now other lines of code will not look me over the shoulder or whisper to each other. They use to say _There is Line 43, did you know that the developer who created it was drunk... I can now walk onto the compiler with my head up..."


Djordje: "Whatever!, You are green, please just be quiet. Are you leaving me alone now?..."

Line 43: "Yes I will leave you alone, bye bye..."

Djordje: "Hey wait a second!... You told me that you would tell me something important if i get you green. What is it?"

Line 43: "Sorry I almost forgot."

Djordje: "Spit it out!"

Line 43:  "Many times when working in legacy software, developers use the so called _poor man technique. They fill the code with print commands to see the contents of the objects, the problem is that they often forget to remove them. But other times there are other print commands that are valid and really are meant to be there. In occasions, it is hard to distinguish what should be tested and what should not be. The rule is: If your application outputs valid information to the console and this is the only way you can test it, then it is correct to do it. But if you can avoid testing the console or other UI's by testing some related methods(sometimes in controllers), then you don't need to test the UI's. Remember that UI's, databases, frameworks and others... should be considered just addons, to our business layer(which at the end of the day is what provides the real business value)." Djordje: "Aaaaghhh... I hate you!"

Line 43:  "But why?"

Djordje: "My sandwitch is cold!"