Code Smell and Shotgun Surgery

shotgun surgery

The Code Smells are similar in concept to Development-level Anti-patterns. Sometimes in our code, we introduce code smell unintentionally those makes our design fragile.

Definition of Code smell
Code smell, also known as a bad smell, in computer programming code, refers to any symptom in the source code of a program that possibly indicates a deeper problem.

Martin fowler says:
 "a code smell is a surface indication that usually corresponds to a deeper problem in the system"


Code smell creates lot of problems while introducing new feature or maintains the codebase,
Often developer has to write repeatable code, breaking encapsulation, breaking abstraction etc.
If code smells are not corrected.

So always refactor code smell while developing.
 In this article, we discuss one of the popular code smell “SHOTGUN SURGERY
Shotgun surgery says, to introduce a small new change, a developer has to change many classes and methods, most of the time has to write duplicate codes which violate “Don’tRepeatYourself” principle.

Cause of Shotgun surgery smell:
1.    Due to poor separation of concern.
2.    Fail to understand the responsibility. Often due to misunderstanding (Single responsibility principle)
3.    Not identifying the common behavior or behavior with a slight change.
4.    Fail to introduce proper design pattern.
Consequences of Shotgun Surgery:
1.    Lots of duplicates code
2.    Taking more time to develop a small feature.
3.    Unmaintainable code base.


Refactoring strategy:

1.    We can do it by “Move Method”, “Move Field” or “Inline class

We will discuss above strategies in another article.


Let see an example, where “Shotgun Surgery” smell is present

package com.example.codesmell;

public class Account {
      
       private String type;
       private String accountNumber;
       private int amount;
      
       public Account(String type,String accountNumber,int amount)
       {
              this.amount=amount;
              this.type=type;
              this.accountNumber=accountNumber;
       }
      
      
       public void debit(int debit) throws Exception
       {
              if(amount <= 500)
              {
                     throw new Exception("Mininum balance shuold be over 500");
              }
             
              amount = amount-debit;
              System.out.println("Now amount is" + amount);
             
       }
      
       public void transfer(Account from,Account to,int cerditAmount) throws Exception
       {
              if(from.amount <= 500)
              {
                     throw new Exception("Mininum balance shuold be over 500");
              }
             
              to.amount = amount+cerditAmount;
             
       }
      
       public void sendWarningMessage()
       {
              if(amount <= 500)
              {
                     System.out.println("amount should be over 500");
              }
       }
      
      

}








package com.example.codesmell;

public class ShotgunSurgery {
      
       public static void main(String[] args) throws Exception {
              Account acc = new Account("Personal","AC1234",1000);
              acc.debit(500);
              acc.sendWarningMessage();
              //acc.debit(500);
       }

}

If we pay attention in Account.java file, we can see every operation debit(), transfer(),sendWarningMessage() has one validation, account balance should be more than 500.
And we copy the same validation in every method because of we not able to identify the common validation, so we introduce a “Shotgun Surgery” code smell.
The real problem occurs when we add another criterion in validation logic that is if account type is personal and balance is over 500 then we can perform above operations.

In this scenario, we have to make change in all methods which is not intended so let’s see how to solve it

Create a common method call isAccountUnderflow() that will solve the problem, all validation related stuff goes there.

Refactored code:

package com.example.codesmell;

public class AcountRefactored {
      
       private String type;
       private String accountNumber;
       private int amount;
      
      
      
       public AcountRefactored(String type,String accountNumber,int amount)
       {
              this.amount=amount;
              this.type=type;
              this.accountNumber=accountNumber;
       }
      
       private boolean isAccountUnderflow()
       {
              if(amount <= 500)
              {
                     return true;
              }
              return false;
             
       }
      
      
       public void debit(int debit) throws Exception
       {
              if(isAccountUnderflow())
              {
                     throw new Exception("Mininum balance shuold be over 500");
              }
             
              amount = amount-debit;
              System.out.println("Now amount is" + amount);
             
       }
      
       public void transfer(AcountRefactored from,AcountRefactored to,int cerditAmount) throws Exception
       {
              if(isAccountUnderflow())
              {
                     throw new Exception("Mininum balance shuold be over 500");
              }
             
              to.amount = amount+cerditAmount;
             
       }
      
       public void sendWarningMessage()
       {
              if(isAccountUnderflow())
              {
                     System.out.println("amount should be over 500");
              }
       }
      
      



}






Picture :


The hidden contract between equals and compareTo

Contract breaking

We know we use equals for check two objects are meaningfully equal or not, on other hand by comparable interface we compare two objects. But Has there is any relation between them?
Or we have to maintain contract between them?

We know When we override equals we have to override hashcode as they maintain a contract.
But we can implement comparable or may not. If we don’t implement comparable then we can’t compare objects in other words we can’t do sorting on them.

So enable sorting, either we introduce Comparable or Comparator( if the class is a legacy class , third-party class or needs to sort in a different way).

So in both case of Comparable and Comparator, returns int, which says
Positive:  Current instance is greater than Other.
Zero: Equals
Negative: Current instance is smaller than Other.

So if Comparable returns 0 it means two objects are same by comparison. Also By equals, we check two objects are same if it returns true.

So my question is If Comparable shows two objects are same and equals says both are un-equals then we face any problem?

Let’s, examine it by code,
package com.example.contract;

public class Glass implements Comparable<Glass>{
      
       public enum Size{
              BIG(3),MEDIUM(2),SMALL(1);
              private int size;
              Size(int size)
              {
                     this.size=size;
              }
              public int getSize()
              {
                     return size;
              }
       };    
       private String material;
       private Size size;
      
       public Glass(Size size,String material)
       {
              this.size=size;
              this.material=material;
       }
      
      

       @Override
       public int hashCode() {
              final int prime = 31;
              int result = 1;
              result = prime * result
                           + ((material == null) ? 0 : material.hashCode());
              result = prime * result + ((size == null) ? 0 : size.hashCode());
              return result;
       }



       @Override
       public boolean equals(Object obj) {
              if (this == obj)
                     return true;
              if (obj == null)
                     return false;
              if (getClass() != obj.getClass())
                     return false;
              Glass other = (Glass) obj;
              if (material == null) {
                     if (other.material != null)
                           return false;
              } else if (!material.equals(other.material))
                     return false;
              if (size != other.size)
                     return false;
              return true;
       }



       @Override
       public int compareTo(Glass o) {
              if(this.size.equals(o.size))
              {
                     return 0;
              }
              else if(this.size.getSize() > o.size.getSize())
              {
                     return 1;
              }
              else
              {
                     return -1;
              }
       }



       @Override
       public String toString() {
              return "Glass [material=" + material + ", size=" + size + "]";
       }
      
      
      
      
      
      
      

}


package com.example.contract;

import java.util.HashSet;
import java.util.Set;
import java.util.TreeSet;

public class ContractBreaker {
      
       public static void main(String[] args) {
             
              Glass plastic = new Glass(Glass.Size.BIG,"Plastic");
              Glass glass = new Glass(Glass.Size.BIG,"glass");
             
              Set<Glass> set = new HashSet<Glass>();
              set.add(plastic);
              set.add(glass);
             
              System.out.println(set);
             
              Set<Glass> treeSet = new TreeSet<Glass>();
              treeSet.add(plastic);
              treeSet.add(glass);
              System.out.println(treeSet);
             
       }

}


Output :
HashSet says [Glass [material=Plastic, size=BIG], Glass [material=glass, size=BIG]]
TreeSet says [Glass [material=Plastic, size=BIG]]


Oops, While HashSets treats them, unequal, TreeSet treats them equal
So in programming, if we change HashSet to TreeSet or vice versa, weird results shows.

The problem occurs because,
When we implement equals we consider two properties of Glass i.e. material and size, so if both are equals then we say both Glasses are meaningfully equivalent.
But in CompareTo method we only consider size. If two size is same, we treat them equals.

Here we did the wrong thing,

 HashMap,ArrayList,HashSet ,they add element based on equals method so When we use HashMap it treats two objects as different objects as their material is different.

But TreeMap ,TreeSet use Compare method so TreeSet  treats them as the same Object.

By this Program, we see that there is a hidden Contract between equals and comparable.

Contract:
If two objects are equals by equality check then those object must be equaled by Comparable or Comparator test.

In Java BigDecimal breaks this contract so when use BigDecimal, please remember the scenario stated above.

Automated webservice code generation using FreeMarker

Soap Web-service template


Sometimes we need to generate, template source code upon which developer do the modifications.
It will save a lot of time for developers. Without Automated generation of source code developers needs to create source files and require methods repetitively, tools will generate files for them, only developers has to put their logic on generated files.

Say, for all JPA entities in a project basic CRUD operations will expose as web service. Either Developer creates Web service manually or we can create the same by the tool which will generate the same.

In this article, will generate a simple java web service using Free Marker. Later you can modify the same to fit it in your project.

Free marker is a java template engine which will dynamically create an output. To generate Output
Free Marker needs a FTL file (Free marker template language), Where instructions and place holder are written.
And a Java file where you provide the actual data which will replace the place holder.

Free maker engine combines them and generates the desired Output.

FreeMarker output generation Process

Benefits of Free marker (taken from free marker website):
1.    Powerful template language: Conditional blocks, iterations, assignments, string and arithmetic operations and formatting, macros and functions, including other templates, escaping by default (optional), and many more.

2      Multipurpose and lightweight: Zero dependencies, any output format, can load templates from any place (pluggable), many configuration options
3      Internationalization/localization-aware: Locale sensitive number and date/time formatting, localized template variations.
4      XML processing capabilities: Drop XML DOM-s into the data-model and traverse them, or even process them declaratively
5      Versatile data-model: Java objects are exposed to the template as a tree of variables through pluggable adapters, which decides how the template sees them.


To generate a Java Webservice file usinf FreeMarker following steps are needed.

1, Create an FTL file, file extension will be  . ftl. Here we define commands for  output  define palace holder, which will be replaced by the actual value from java objects.

${package}

import javax.jws.*;

@WebService()
public class ${name}
{

@WebMethod()
public ${return} ${methodname}(${params})
{
   ${body}
       return ${val}
}

}

2.    Next ,create a java file which will provide data for placeholder and says the location where Output will be saved.(To run this file freemarker*.jar is required)
package com.example;

import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.io.OutputStreamWriter;
import java.io.Writer;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import freemarker.template.Configuration;
import freemarker.template.Template;
import freemarker.template.TemplateException;

public class WebServiceGenerator {
           
            private static WebServiceGenerator engine = new WebServiceGenerator();
            private Template template=null;
            Map<String, Object> dataMap = new HashMap<String, Object>();
           
            private WebServiceGenerator()
            {
                        init();
            }
           
            private void init()
            {
                       
                        Configuration cfg = new Configuration();
                        try {
                                    template = cfg.getTemplate("src/javaGen.ftl");
                        } catch (IOException e) {
                                    // TODO Auto-generated catch block
                                    e.printStackTrace();
                        }
                       
            }
           
            public static WebServiceGenerator get()
            {
                        return engine;
            }
           
            public WebServiceGenerator buildData()
            {
                        dataMap.put("package", this.getClass().getPackage()+";");
                        dataMap.put("name", "HelloWorldservice");
                        dataMap.put("return", "String");
                        dataMap.put("methodname", "hello");
                        dataMap.put("params", "String name");
                        dataMap.put("body", "String res= \"Hi\" + name;\n System.out.println(res);");
                        dataMap.put("val", "res;");
                        System.out.println("Preparing Data");
                       
                       
                       
                        return engine;
            }
           
            public void  writeFile()
            {
                        Writer file=null;
                        try {
                                    file = new FileWriter (new File("C:\\installer\\HelloWorldservice.java"));                   
                                    template.process(dataMap, file);
                                    file.flush();
                                    System.out.println("Success");
                       
                        }catch (Exception e) {
                                    // TODO Auto-generated catch block
                                    e.printStackTrace();
                        }
                        finally
                        {
                                   
                                    try {
                                                file.close();
                                    } catch (IOException e) {
                                                // TODO Auto-generated catch block
                                                e.printStackTrace();
                                    }
                        }
                       
            }
           

           

            public static void main(String[] args) {
                       
                        WebServiceGenerator.get().buildData().writeFile();
                       
            }

}





Output :

package com.example;

import javax.jws.*;

@WebService()
public class HelloWorldservice
{

@WebMethod()
public String hello(String name)
{
   String res= "Hi" + name;
 System.out.println(res);
       return res;
}



}