preface

What is good code? Good code should be named properly, readable, extensible, and robust…… What are the typical characteristics of bad code? Here are 25 bad code smells to watch out for

  • Public number: a boy picking up snails
  • Making the address

1. Duplicated Code

Duplicate code is the same program structure in different locations. Generally, it is caused by the rapid iteration of requirements, and the developer partners worry about affecting the existing functions, so they copy and paste. Duplicate code is difficult to maintain, and if you want to change the logic of one piece of code, you will need to change it many times, and it is very likely that there will be omissions.

How do you optimize duplicate code? There are three cases to discuss:

  1. Two functions of the same class have the same expression
class A {
    public void method1() {
        doSomething1
        doSomething2
        doSomething3
    }
    public void method2() {
        doSomething1
        doSomething2
        doSomething4
    }
}
Copy the code

Optimization: You can use Extract Method to Extract repetitive code logic into a common Method.

class A {
    public void method1() {
        commonMethod();
        doSomething3
    }
    public void method2() {
        commonMethod();
        doSomething4
    }
    
    public void commonMethod(){
       doSomething1
       doSomething2
    }
}
Copy the code
  1. Two sibling subclasses contain the same expression
class A extend C {
    public void method1() {
        doSomething1
        doSomething2
        doSomething3
    }
}

class B extend C {
    public void method1() {
        doSomething1
        doSomething2
        doSomething4
    }
}

Copy the code

Optimization: Use Extract Method for both classes and place the extracted functions in the parent class.

class C {
    public void commonMethod(){
     doSomething1
     doSomething2
   }
}
class A extend C {
    public void method1() {
        commonMethod();
        doSomething3
    }
}

class B extend C {
    public void method1() {
        commonMethod();
        doSomething4
    }
}
Copy the code
  1. Duplicate code appears in two unrelated classes

If duplicate code occurs in two unrelated classes, you can use Extract Class to Extract the duplicate code into a single Class. This new class can be a generic class or a utility class, depending on how the business is divided.

2.Long Method

Long function refers to a function method hundreds or even thousands of lines, greatly reduced readability, not easy to understand. For example:

public class Test { private String name; private Vector<Order> orders = new Vector<Order>(); public void printOwing() { //print banner System.out.println("****************"); System.out.println("*****customer Owes *****"); System.out.println("****************"); //calculate totalAmount Enumeration env = orders.elements(); Double totalAmount = 0.0; while (env.hasMoreElements()) { Order order = (Order) env.nextElement(); totalAmount += order.getAmout(); } //print details System.out.println("name:" + name); System.out.println("amount:" + totalAmount); . }}Copy the code

You can use Extract Method to Extract a single piece of code and form a small, cleanly named function to solve a long function problem, for example:

public class Test { private String name; private Vector<Order> orders = new Vector<Order>(); public void printOwing() { //print banner printBanner(); //calculate totalAmount double totalAmount = getTotalAmount(); //print details printDetail(totalAmount); } void printBanner(){ System.out.println("****************"); System.out.println("*****customer Owes *****"); System.out.println("****************"); } double getTotalAmount(){ Enumeration env = orders.elements(); Double totalAmount = 0.0; while (env.hasMoreElements()) { Order order = (Order) env.nextElement(); totalAmount += order.getAmout(); } return totalAmount; } void printDetail(double totalAmount){ System.out.println("name:" + name); System.out.println("amount:" + totalAmount); }}Copy the code

3. Large Class

A class that does too much, maintains too many features, becomes less readable, and its performance deteriorates. For example, order related functions are put in A class, inventory related functions are put in A class, points related functions are put in A class… For example:

Class A{public void printOrder(){system.out.println (" order "); } public void printGoods(){system.out.println (" goods "); } public void printPoints(){system.out.println (" points "); }}Copy the code

Think of all the random code blocks crammed into a class. Use Extract Class to separate the code into a single responsibility, as shown below:

Class Order{public void printOrder(){system.out.println (" Order "); } Class Goods{public void printGoods(){system.out.println (" Goods "); }} Class Points{public void printPoints(){system.out.println (" "); }}}Copy the code

4. Long Parameter List

A method with too many parameters is not readable. If you have multiple overloaded methods with a lot of arguments, sometimes you don’t know which one to call. In addition, if there are many parameters, it is more troublesome to do the old interface compatibility processing.

Public void getUserInfo (String name,String Age,String sex,String mobile){// Do something... }Copy the code

How to solve the problem of too long argument columns? To encapsulate a parameter as a structure or class, for example we encapsulate a parameter as a DTO class as follows:

Public void getUserInfo (UserInfoParamDTO UserInfoParamDTO){// do something... } class UserInfoParamDTO{ private String name; private String age; private String sex; private String mobile; }Copy the code

She has Divergent changes in her life.

When a modification component is added to a program in maintenance that involves simultaneously modifying multiple methods in a class, the Divergent Change is recognized. For example, a car manufacturer produces three brands of cars: BMW, Benz and LaoSiLaiSi, each of which can choose from fuel, pure electric and hybrid. For example:

/** * public class Car {private String name; void start(Engine engine) { if ("HybridEngine".equals(engine.getName())) { System.out.println("Start Hybrid Engine..." ); } else if ("GasolineEngine".equals(engine.getName())) { System.out.println("Start Gasoline Engine..." ); } else if ("ElectricEngine".equals(engine.getName())) { System.out.println("Start Electric Engine"); } } void drive(Engine engine,Car car) { this.start(engine); System.out.println("Drive " + getBrand(car) + " car..." ); } String getBrand(Car car) { if ("Baoma".equals(car.getName())) { return "BMW"; } else if ("BenChi".equals(car.getName())) { return "Benz"; } else if ("LaoSiLaiSi".equals(car.getName())) { return "LaoSiLaiSi"; } return null; }}Copy the code

If you add a new brand of neV that starts with a nuclear engine, you need to Change the Start and getBrand methods of the Car class. This is the code Divergent Change.

How do you optimize it? In a word: break things down and put together things that always change together.

  • The act of extracting classes.
  • Extract Superclass and Extract Subclass if different classes have the same behavior.

For example:

Because Engine changes independently, extract an Engine interface, and if you add a startup Engine, add an implementation class. As follows:

//IEngine
public interface IEngine {
    void start();
}

public class HybridEngineImpl implements IEngine { 
    @Override
    public void start() {
        System.out.println("Start Hybrid Engine...");
    }
}
Copy the code

The drive method depends on the Car, IEngine, and getBand methods. The getBand method is variable and also associated with Car, so you can create an abstract Car class that each brand of Car inherits from, as follows

public abstract class AbstractCar { protected IEngine engine; public AbstractCar(IEngine engine) { this.engine = engine; } public abstract void drive(); } public class BenzCar extends AbstractCar {public BenzCar(IEngine engine) {super(engine); } @Override public void drive() { this.engine.start(); System.out.println("Drive " + getBrand() + " car..." ); } private String getBrand() { return "Benz"; Public class extends AbstractCar {public BaoMaCar(IEngine engine) {super(engine); } @Override public void drive() { this.engine.start(); System.out.println("Drive " + getBrand() + " car..." ); } private String getBrand() { return "BMW"; }}Copy the code

If you are careful, you can see that the drive method in different subclasses BaoMaCar and BenzCar still have the same code, so we can extend another abstract subclass to push drive into it, as follows:

public abstract class AbstractRefinedCar extends AbstractCar { public AbstractRefinedCar(IEngine engine) { super(engine); } @Override public void drive() { this.engine.start(); System.out.println("Drive " + getBrand() + " car..." ); } abstract String getBrand(); } public class AbstractRefinedCar extends AbstractRefinedCar {public class BaoMaRefinedCar(IEngine engine) {super(engine);  } @Override String getBrand() { return "BMW"; }}Copy the code

If you want to add a new brand, make a subclass that inherits AbstractRefinedCar. If you want to add a startup engine, make a class that implements the IEngine interface

6. Shotgun Surgery (Shotgun Modification)

When you implement a small feature, you need to make small changes in many different classes. That’s Shotgun Surgery. The Divergent Change differs from Divergent Change in that it involves making a single Change in multiple Divergent classes at once, or multiple changes in a single class. For example:

public class DbAUtils { @Value("${db.mysql.url}") private String mysqlDbUrl; . } public class DbBUtils { @Value("${db.mysql.url}") private String mysqlDbUrl; . }Copy the code

Db.mysql. url is used by multiple classes. If you need to switch mysql to another database, such as Oracle, you will need to change this variable for multiple classes.

How do you optimize it? All the change points are abstracted together into a new class.

You can use Move Method and Move Field to put all the code you need to change into the same class. If you don’t have a suitable class, you can create a new one.

For example:

public class DbUtils { @Value("${db.mysql.url}") private String mysqlDbUrl; . }Copy the code

7. Feature Envy

A function calls almost half a dozen value functions from another object to evaluate a value. In layman’s terms, a function that uses a large number of members of other classes is called a cheating function. For example:

public class User{ private Phone phone; public User(Phone phone){ this.phone = phone; } public void getFullPhoneNumber(Phone phone){ System.out.println("areaCode:" + phone.getAreaCode()); System.out.println("prefix: "+ phone.getPrefix()); System.out.println("number: "+ phone.getNumber()); }}Copy the code

How to solve it? In this case, you might consider moving the method to the class it uses. For example, getFullPhoneNumber() is moved from the User class to the Phone class because it calls many of the Phone class methods.

8. A Clumps of Data

Data items, like children, like to stay together in groups. If some data items always appear together and it makes more sense to do so together, consider encapsulating the data as data objects in terms of their business meaning. For example:

public class User {

    private String firstName;
    private String lastName;

    private String province;
    private String city;
    private String area;
    private String street;
}

Copy the code

Is:

public class User {

    private UserName username;
    private Adress adress;
}

class UserName{
    private String firstName;
    private String lastName;
}
class Address{
    private String province;
    private String city;
    private String area;
    private String street;
}
Copy the code

9. Primitive Obsession

Most programming environments have two types of data, structural and primitive. The basic types, if you mean the Java language, include not only the eight basic types, but also strings and so on. If you have primitive types that often come together, consider wrapping them as objects. I personally felt it was a bit like a Data Clumps, for example:

Public class Order {private String customName; private String address; private Integer orderId; private Integer price; }Copy the code

Is:

Public class Order {private Custom Custom; private Integer orderId; private Integer price; } public class custom {private String name; private String address; }Copy the code

Of course, not all primitive types are recommended to be encapsulated as objects, associated or present together.

10. Switch Statements

Switch statements include not only switch-related statements, but also multiple layers of if… Else statement. A lot of times, the problem with switch statements is repetition. If you add a new case statement to it, you have to find all the switch statements and change them.

Example code is as follows:

String medalType = "guest"; If ("guest".equals(medalType)) {system.out.println ("guest"); } else if (" VIP ".equals(medalType)) {system.out.println (" member "); } else if ("guard".equals(medalType)) {system.out.println ("guard"); }...Copy the code

This scenario can be considered using polymorphic optimization:

Public interface IMedalService {void showMedal(); } public class GuardMedalServiceImpl implements IMedalService {@override public void showMedal() { System.out.println(" display guard medal "); Public class GuestMedalServiceImpl implements IMedalService {@override public void showMedal() { System.out.println(" guest medal "); }} public class MedalServicesFactory {private static final Map<String, IMedalService> map = new HashMap<>(); static { map.put("guard", new GuardMedalServiceImpl()); map.put("vip", new VipMedalServiceImpl()); map.put("guest", new GuestMedalServiceImpl()); } public static IMedalService getMedalService(String medalType) { return map.get(medalType); }}Copy the code

Of course, polymorphism is only one solution, one direction of optimization. It is not recommended to use dynamics if there are simple selection examples for a single function, because it is a bit of an overkill.

Hierarchies of Parallel Inheritance

Parallel inheritance is a special case of Shotgun Surgery. When you subclass Ax of class A, you must add A subclass Bx of class B.

Solution: In this case, remove the reference between the two inheritance systems. There is a class that can remove the inheritance relationship.

12. Lazy Class

Merge the logic in these no longer important classes into related classes and delete the old ones. A common scenario is that the system already has a date utility class DateUtils, some friends in the development, need to use the date conversion, etc., no matter what happens, and then implement a new date utility class.

13. Speculative Generality

Try to avoid over-designed code. Such as:

  • There’s only an if else, so you don’t have to teach yourself how to use polymorphism;
  • If an abstract class doesn’t do much, useCollapse Hierarchy(Folded inheritance system)
  • If some parameter of the function is not used, remove it.

14. Temporary Field(confusing Temporary Field)

Code that is confusing when an instance variable is set only for a particular case is called a Temporary Field. For example:

public class PhoneAccount { private double excessMinutesCharge; Private static final double RATE = 8.0; Public double computeBill(int minutesUsed, int includedMinutes) {excessMinutesCharge = 0.0; int excessMinutes = minutesUsed - includedMinutes; if (excessMinutes >= 1) { excessMinutesCharge = excessMinutes * RATE; } return excessMinutesCharge; } public double chargeForExcessMinutes(int minutesUsed, int includedMinutes) { computeBill(minutesUsed, includedMinutes); return excessMinutesCharge; }}Copy the code

Is the temporary field excessMinutesCharge redundant?

15. Message Chains

When you see users requesting one object for another object, and then requesting another object for that object, and then requesting another object for that object… This is the message chain. In real code, you might see a long list of getThis () or a long list of temporary variables. For example:

A.getB().getC().getD().getTianLuoBoy().getData();
Copy the code

If A wants to get the data it needs, it must know B, C, and D… In fact, A needs to know too much, back to think about encapsulation, hee hee. In fact, it can be solved by splitting or moving the function, such as B as the proxy, to create A function that directly returns the data A needs.

16. The Middle Man

One of the basic characteristics of objects is encapsulation, that is, hiding their internal details from the outside world. Encapsulation often comes with delegates, and overusing delegates is bad: half the functions of a class interface are delegated to other classes. This can be optimized using Remove Middle Man. For example:

A.B.getC(){
   return C.getC();
}
Copy the code

In fact, A can directly obtain C from C, but does not need to obtain C from B.

17. Inappropriate Intimacy.

If two classes are so intimate and so voluptuousness that they have me in them and you in me, and they use each other’s private stuff, it’s a bad code smell. We call it Inappropriate Intimacy.

It is recommended that associated methods or attributes be removed as much as possible and placed in a common class to reduce associations.

18. Alternative Classes with Different Interfaces

Interface A of class A, and interface B of class B, do the same thing, or something similar. Let’s call A and B identical classes.

It can be optimized by renaming, moving functions, or abstract subclasses

19. Incomplete Library Class

Most objects are just good enough, and if the library isn’t well constructed, we can’t modify the classes to do what we want them to do. This can be wrapped in a layer of functions or a new class.

20. Data Class (pure Data Class)

What is a Data Class? They have fields and functions to access (read and write) those fields. These classes are simple, with only common member variables, or functions that operate simply.

How do you optimize it? Encapsulate related operations and reduce public member variables. Such as:

  • If you have a public field ->Encapsulate Field
  • If these classes contain container-class fields, you should check that they are properly encapsulated ->Encapsulate Collection encapsulated
  • For fields that should not be modified by other classes ->Remove Setting Method -> Find out where the value/set function is used by other classes ->Move MethodMove the call behavior toData ClassTo come. If you can’t move the whole function, apply it Extract MethodProduces a moveable function ->Hide MethodHide these value/setup functions.

21. The Refused Bequest

Subclasses should inherit data and functions from their parent class. Subclass inheritance gets all the functions and data, but only uses a few, which is an inheritance architecture design error that needs to be optimized.

  • Need to create a new sibling for this subclass ->Push Down MethodandPush Down FieldBy pushing down all functions that are not needed to the sibling class, the superclass holds only what all subclasses share. All superclasses should be abstract.
  • If subclasses reuse the implementation of the superclass, but do not want to support the interface of the superclass, it is ok. But don’t tinker with the inheritance system ->Replace Inheritance with DelegationDelegate instead of inheritance.

22. Comments (Too many Comments)

This point is not to say that code should not be commented, but to avoid using comments to explain code and avoid excessive comments. These are the bad smells of common comments:

  • Superfluous explanation
  • Log comment
  • Explain variables, etc with comments
  • .

How do you optimize it?

  • Methods, functions, and variables should be named in a standard, easy to understand, and avoid using comments to explain the code.
  • Use clear, concise notes for critical, complex business

23. Magical naming

Methods, functions, variables, class names, modules, etc., need to be simple and easy to understand. Avoid making names out of your own mind.

Example:

boolean test = chenkParamResult(req);
Copy the code

Is:

boolean isParamPass = chenkParamResult(req);
Copy the code

24. Magic number

In everyday development, you’ll often come across code like this:

if(userType==1){
   //doSth1
}else If( userType ==2){
   //doSth2
}
...
Copy the code

What does this 1 and 2 mean in this code? What about the 1 in setStatus(1)? When you see bad code like this, you can optimize it in two ways:

  • Create a constant class, put some constants in it, manage them, and write comments;
  • Create an enumeration class that manages related magic numbers together.

25. Confusing code hierarchy calls

Our code typically has a DAO layer, service layer, and Controller layer.

  • The DAO layer mainly does the work of the data persistence layer and deals with the database.
  • The Service layer is responsible for business logic processing.
  • The Controller layer is responsible for the control of specific business module processes.

So the controller calls the Service, and the service calls the DAO. If you see the Controller calling the DAO directly in your code, consider tuning. For example:

@RestController("user") public class UserController { Autowired private UserDao userDao; @RequestMapping("/queryUserInfo") public String queryUserInfo(String userName) { return userDao.selectByUserName(userName); }}Copy the code

Reference and thanks

  • Soft engineering experiments: Common code bad taste and refactoring examples
  • 22 bad code smells, in one sentence
  • The code smells Bad
  • Code Smell
  • Refactoring improves the design of Existing Code