Shotgun Surgery

Description (What)

Shotgun Surgery is when making a small change to one class leads you to make many small changes to other classes.

How to Locate It (Where)

Look for an extreme split up of responsibility: One responsibility that has been split up among many files.

How It Manifests(Why)

This code smell can occur because of overuse of treating Divergent Change code smell

In an attempt to follow Single Responsibility principle, you might be tempted to create a class for every small responsibility.

In reality this may seem normal, but you have to add a logical minimum limit for defining a single responsibility.

Creating many small classes that might together define a single responsibility can lead to having to make changes to many classes when trying to make a change to one class (one part of the larger responsibility).

How to Fix It (Possible Treatments)

In Example 1, Move Method is used to move methods back to a larger responsibility class. You should move methods that belong together in the larger logical grouping to one class.

In Example 2, a combination of Move Method and Move Field is used since the fields used in the methods need to go with the methods.

Other treatments are also possible based on the specific scenario, they can be found here

Examples

Example 1

Before:

Both CheckProcessor class and DirectDepositProcessor classes print information about the BankAccount class. If there is any change in the way print is to be handled or another field is added to BankAccount class, change will need to be made to the other two classes as well.

Observed Code Smells:
- Shotgun Surgery (lines 59, 94-95)
- Feature Envy (lines 59, 94-95)

After:

Applied Move Method to move the printing logic to BankAccount class itself which also removes Feature Envy and now any change made to printing logic or any new fields that need to be printed (ex: balance) can be made in one class.

Refactoring Applied:
- Shotgun Surgery
    - Move Method (printAccountDetails)

Observed Code Smells After Refactoring:
- None

Example 2

Before:

Imagine this system consist of many types of sensors (now 3), and each of the sensors has a battery inside of it.

But essentially, batteries and sensors are separate entities. So it makes no sense, when we change something about the

battery, we should go into every sensor classes to have that done. It leads to Shotgun issue.


Observed Code Smells:
- Shotgun Surgery (line 26, lines 28-29, line 38, line 42, )

After:

Applied Move Method and Move Field to the SensorData class to have the constants and the check logic managed

in a centralized data system.


Refactoring Applied:
- Shotgun Surgery 
  -  Move Field (lines 129-131)
  -  Move Method (lines 146-160)

Observed Code Smells After Refactoring:
- None

When to Ignore

None

More about Shotgun Surgery

Example 1
Example 2

Before
After

001  import java.time.LocalDateTime;
002  import java.util.HashMap;
003  import java.util.Map;
004  
005  class BankAccount {
006      private String name;
007      private int id;
008      private double balance;
009  
010      public BankAccount(String name, int id, double balance) {
011          this.name = name;
012          this.id = id;
013          this.balance = balance;
014      }
015  
016      public void withdraw(double withdrawAmount) throws IllegalArgumentException {
017          if (withdrawAmount > balance) {
018              throw new IllegalArgumentException("Not enough balance");
019          }
020          this.balance -= withdrawAmount;
021          System.out.println("Successful deposit. New balance: " + this.balance);
022      }
023  
024      public void deposit(double depositAmount) throws IllegalArgumentException {
025          if (depositAmount < 0) {
026              throw new IllegalArgumentException("Negative deposit amount");
027          }
028          this.balance += depositAmount;
029          System.out.println("Successful withdrawal. New balance: " + this.balance);
030      }
031  
032      public String getName() {
033          return this.name;
034      }
035  
036      public double getBalance() {
037          return this.balance;
038      }
039  
040      public int getId() {
041          return this.id;
042      }
043  
044  }
045  
046  class DirectDepositProcessor {
047      private BankAccount account;
048      private Map<LocalDateTime, Double> depositHistory;
049  
050      public DirectDepositProcessor(BankAccount account) {
051          this.account = account;
052          this.depositHistory = new HashMap<>();
053      }
054  
055      public void depositSalary(double salary) {
056          LocalDateTime currentDateTime = LocalDateTime.now();
057          account.deposit(salary);
058          depositHistory.put(currentDateTime, salary);
059          System.out.println("Successfully put salary into account:\n" + "Id: " + account.getId() + "\nName: " + account.getName());
060          System.out.println("New balance: " + account.getBalance());
061      }
062  
063      public void printSalaryHistory() {
064          for (Map.Entry<LocalDateTime, Double> entry : depositHistory.entrySet()) {
065              System.out.println(entry.getKey() + " : " + entry.getValue());
066          }
067      }
068  
069  }
070  
071  class CheckProcessor {
072      private BankAccount account;
073  
074      public CheckProcessor(BankAccount account) {
075          this.account = account;
076      }
077  
078      public void validateCheque(BankAccount sender, double amount) throws IllegalArgumentException {
079          if (sender.getBalance() < amount) {
080              throw new IllegalArgumentException("Insufficient funds. Cheque bounced");
081          }
082          if (sender.getId() == account.getId()) {
083              throw new IllegalArgumentException("Cannot send cheque to receiver");
084          }
085          sender.withdraw(amount);
086          account.deposit(amount);
087          generateReceipt(sender, amount);
088      }
089  
090      private void generateReceipt(BankAccount sender, double amount) {
091          System.out.println("-------------------");
092          System.out.println(LocalDateTime.now().toString() + "\n");
093          System.out.println("===================");
094          System.out.println("Sender Information:\n" + "Id: " + sender.getId() + "\nName: " + sender.getName());
095          System.out.println("Receiver Information:\n" + "Id: " + account.getId() + "\nName: " + account.getName());
096          System.out.println("Amount transferred: " + amount);
097          System.out.println("-------------------");
098      }
099  
100  }
101  
102  public class SSBE1 {
103      public static void main(String[] args) {
104          BankAccount john = new BankAccount("John Appleseed", 1, 1000);
105          BankAccount yash = new BankAccount("Yash Burshe", 2, 50);
106          DirectDepositProcessor ddp = new DirectDepositProcessor(john);
107          CheckProcessor cp = new CheckProcessor(yash);
108          ddp.depositSalary(1000);
109          ddp.depositSalary(1000);
110          ddp.printSalaryHistory();
111          try {
112              cp.validateCheque(yash, 25);
113          } catch (IllegalArgumentException e) {
114              System.out.println(e.getMessage());
115          }
116  
117          try {
118              cp.validateCheque(john, 50000);
119          } catch (IllegalArgumentException e) {
120              System.out.println(e.getMessage());
121          }
122  
123          try {
124              cp.validateCheque(john, 500);
125          } catch (IllegalArgumentException e) {
126              System.out.println(e.getMessage());
127          }
128      }
129  }
130