Bad smell of code
Long Method (too long function)
//for if multiple nesting
public class PriceCalculatorServiceV1 {<!-- -->
public int calculateTotalPrice(List<Product> products, List<Discount> discounts, Customer customer) {<!-- -->
//Discount calculation
int total = 0;
for (Product product : products) {<!-- -->
if (customer.isEligibleForDiscount()) {<!-- -->
int discountedPrice = product.getPrice();
for (Discount discount : discounts) {<!-- -->
if (discount.isApplicable(product)) {<!-- -->
int amountOff = discount.getAmountOff();
discountedPrice -= amountOff;
}
}
total + = discountedPrice;
} else {<!-- -->
total + = product.getPrice();
}
}
return total;
}
}
// dismantle
public class Calculator {<!-- -->
public int calculateTotalPrice(List<Product> products, List<Discount> discounts, Customer customer) {<!-- -->
int total = 0;
for (Product product : products) {<!-- -->
if (customer.isEligibleForDiscount()) {<!-- -->
int discountedPrice = calculateDiscountedPrice(product, discounts);
total + = discountedPrice;
} else {<!-- -->
total + = product.getPrice();
}
}
return total;
}
private int calculateDiscountedPrice(Product product, List<Discount> discounts) {<!-- -->
int discountedPrice = product.getPrice();
for (Discount discount : discounts) {<!-- -->
if (discount.isApplicable(product)) {<!-- -->
int amountOff = discount.getAmountOff();
discountedPrice -= amountOff;
}
}
return discountedPrice;
}
}
Duplicated Code
// method1 method2 repeats code doSomething1, doSomething2
class Test {<!-- -->
public void method1() {<!-- -->
doSomeThing1;
doSomeThing2;
doSomeThing3;
}
public void method2() {<!-- -->
doSomeThing1;
doSomeThing2;
doSomeThing4;
}
}
// Positive example: First extract it as a public function within the class, and then consider whether it can become a global public function.
class Test {<!-- -->
public void method1() {<!-- -->
commonMethod();
doSomeThing3;
}
public void method2() {<!-- -->
commonMethod();
doSomeThing4;
}
public void commonMethod(){<!-- -->
doSomeThing1;
doSomeThing2;
}
}
Mysterious Name
public class MysteriousNamingExample {<!-- -->
public static void main(String[] args) {<!-- -->
//Mysterious field naming
int p1 = 1000;
int r1 = 20;
int q1 = 5;
//Mysterious method naming
int t1 = calculate(p1, r1, q1);
System.out.println("The mysterious total price is: " + t1);
}
public static int calculate(int p, int r, int q) {<!-- -->
int o1 = p * r;
int z1 = o1 * q;
return z1;
}
}
//Variable naming surfaces variable meanings, method naming surfaces method behavior
public class ImprovedNamingExample {<!-- -->
public static void main(String[] args) {<!-- -->
//Clearly name the field meaning
int productPrice = 1000;
int discountRate = 20;
int quantity = 5;
//Clear method behavior naming
int totalPrice = calculateTotalPrice(productPrice, discountRate, quantity);
System.out.println("The calculated total price is: " + totalPrice);
}
public static int calculateTotalPrice(int price, int discount, int quantity) {<!-- -->
int discountedPrice = price * discount / 100;
int total = discountedPrice * quantity;
return total;
}
}
Large Class (too large class)
interface class OrderService {<!-- -->
createOrder();
queryOrder();
updateOrderStatus();
snapshotGoods();
snapshotAddress();
queryGoodsSnapshot();
queryAddressSnapshot();
createPay();
queryPay();
updatePayStatus();
}
// How large a class should be split according to a single responsibility
interface class OrderService {<!-- -->
createOrder();
queryOrder();
updateOrderStatus();
}
interface class OrderSnapshotService {<!-- -->
snapshotGoods();
snapshotAddress();
queryGoodsSnapshot();
queryAddressSnapshot();
}
interface class PayService {<!-- -->
createPay();
queryPay();
updatePayStatus();
}
Long Paramenter List (too long parameter list)
//Generally, parameters exceeding 3 are considered too long.
interface class OrderService {<!-- -->
queryOrder(String orderId,String orderNo,Long spuId,Long skuId,Long addressId,Long startTime,Long endTime);
}
interface class OrderService {<!-- -->
queryOrder(OrderSearchDto dto);
}
// If the parameters are too long, the caller must fill them in in the order of the parameters. It is easy to fill in the wrong parameters, difficult to expand, and the readability of the parameters is not good if they are too long.
class OrderSearchDto {<!-- -->
String orderId;
String orderNo;
Long spuId;
Long skuId;
Long addressId;
Long startTime;
Long endTime;
}
//If there are too many dimensions of single-class design data after splitting, you can continue to split the class.
class BaseSearchDto {<!-- -->
Long startTime;
Long endTime;
}
class OrderSearchDto extends BaseSearchDto {<!-- -->
String orderId;
String orderNo;
}
class OrderGoodsSearchDto {<!-- -->
Long spuId;
Long skuId;
Long addressId;
}
interface class OrderService {<!-- -->
queryOrder(OrderSearchDto orderSearchDto,OrderGoodsSearchDto orderGoodsSearchDto);
}
Divergent Change
//Place orders, cancel orders, track orders, generate invoices and send email notifications (one-stop code)
interface class OrderService {<!-- -->
void createOrder();
void cancelOrder();
void trackOrder();
void generateInvoice();
void sendEmailNotification();
}
//Separation of responsibilities
public class OrderPlacer {<!-- -->
void createOrder();
}
public class OrderCanceler {<!-- -->
void cancelOrder();
}
public class OrderTracker {<!-- -->
void trackOrder();
}
public class InvoiceGenerator {<!-- -->
void generateInvoice();
}
public class EmailNotifier {<!-- -->
void sendEmailNotification();
}
Shotgun Surgery
//Objects that need to monitor data changes in the user field and order field are split using the dependency inversion principle.
class class User {<!-- -->
private String userId;
private String name;
private List<Order> orders;
//Constructor and other methods
public void confirm() {<!-- -->
//Logic that needs to be added
// ...
}
}
public class Order {<!-- -->
private String orderId;
private Date orderDate;
private double totalAmount;
//Constructor and other methods
public void confirm() {<!-- -->
//Logic that needs to be added
// ...
}
}
// Depend on the observer mode of inversion to remove changes in confirm
public interface OrderConfirmationObserver {<!-- -->
void notifyOrderConfirmation(Order order);
}
public class User implements OrderConfirmationObserver {<!-- -->
// Other properties and methods
public void confirm() {<!-- -->
//Execute user confirmation logic
// ...
// Notify order confirmation
for (Order order : orders) {<!-- -->
order.notifyOrderConfirmation();
}
}
@Override
public void notifyOrderConfirmation(Order order) {<!-- -->
//Execute order confirmation logic
// ...
}
}
public class Order {<!-- -->
private String orderId;
private Date orderDate;
private double totalAmount;
private OrderConfirmationObserver confirmationObserver;
// Other properties and methods
public void confirm() {<!-- -->
//Execute order confirmation logic
// ...
// Notify the order confirmation observer
confirmationObserver.notifyOrderConfirmation(this);
}
public void setConfirmationObserver();
}
Feature Envy (attachment complex)
A certain piece of code is too dependent on other objects or modules and lacks independence and reusability.
public class UserController {<!-- -->
private UserRepository userRepository;
public void createUser(String name) {<!-- -->
// Depends on specific UserRepository implementation
User user = new User(name);
userRepository.save(user);
sendWelcomeEmail(user);
}
private void sendWelcomeEmail(User user) {<!-- -->
// Code to send welcome email
// ...
}
}
// After reconstruction
//Adopt dependency injection emailService
public class UserController {<!-- -->
private UserRepository userRepository;
private EmailService emailService;
public void createUser(String name) {<!-- -->
User user = new User(name);
userRepository.save(user);
emailService.sendWelcomeEmail(user);
}
}
Data Clumps
There is a large amount of repetitive, redundant, and difficult-to-maintain data in the code. This data may be scattered throughout various parts of the code rather than being stored in one place
public class OrderProcessor {<!-- -->
public void processOrder(Order order) {<!-- -->
// Get the tax rate from the database
double taxRate = DatabaseHelper.getTaxRate();
// Get the discount rate from the configuration file
double discountRate = ConfigHelper.getDiscountRate();
// Calculate tax amount
double taxAmount = order.getTotalAmount() * taxRate;
// Calculate discount
double discount = order.getTotalAmount() * discountRate;
//Apply taxes and discounts
order.setTotalAmount(order.getTotalAmount() + taxAmount - discount);
//Update order status
order.setStatus("Processed");
//Save the order to the database
DatabaseHelper.saveOrder(order);
}
}
//After reconstruction
//Tax rate and discount rate are handed over to OrderProcessor for processing
public class OrderProcessor {<!-- -->
private double taxRate;
private double discountRate;
public OrderProcessor(double taxRate, double discountRate) {<!-- -->
this.taxRate = taxRate;
this.discountRate = discountRate;
}
public void processOrder(Order order) {<!-- -->
double taxAmount = order.getTotalAmount() * taxRate;
double discount = order.getTotalAmount() * discountRate;
order.setTotalAmount(order.getTotalAmount() + taxAmount - discount);
order.setStatus("Processed");
DatabaseHelper.saveOrder(order);
}
}
Primitive Obsession (basic type of obsession)
Excessive use of primitive data types in code instead of using objects or custom types
public class OrderProcessor {<!-- -->
public void processOrder(double totalPrice) {<!-- -->
// Code for processing the total price
// ...
}
}
//After reconstruction
//Use the Order class to encapsulate order information
public class Order {<!-- -->
private double totalPrice;
public Order(double totalPrice) {<!-- -->
this.totalPrice = totalPrice;
}
// getter and setter methods
}
public class OrderProcessor {<!-- -->
public void processOrder(Order order) {<!-- -->
// Code to process the order
// Use order.getTotalPrice() to get the total price
// ...
}
}
Switch Statements(Switch shows up in horror)
Excessive use of switch statements in the code results in lengthy code that is difficult to expand and maintain.
If you need to add a new payment method, you need to modify the code of the switch statement, which violates the opening and closing principle.
public class OrderProcessor {<!-- -->
public void processOrder(String paymentMethod) {<!-- -->
switch (paymentMethod) {<!-- -->
case "creditCard":
// Logic for processing credit card payments
break;
case "paypal":
// Logic for processing PayPal payments
break;
case "bankTransfer":
// Logic for processing bank transfer payment
break;
case "cash":
// Logic for handling cash payments
break;
default:
// Process logic for unknown payment methods or throw exceptions
break;
}
}
}
//After reconstruction
//Modify using polymorphism and object-oriented design principles
public class PaymentProcessor {<!-- -->
public void processOrder(PaymentMethod paymentMethod) {<!-- -->
paymentMethod.processPayment();
}
}
public interface PaymentMethod {<!-- -->
void processPayment();
}
public class CreditCardPayment implements PaymentMethod {<!-- -->
public void processPayment() {<!-- -->
// Logic for processing credit card payments
}
}
public class PaypalPayment implements PaymentMethod {<!-- -->
public void processPayment() {<!-- -->
// Logic for processing PayPal payments
}
}
public class BankTransferPayment implements PaymentMethod {<!-- -->
public void processPayment() {<!-- -->
// Logic for processing bank transfer payment
}
}
public class CashPayment implements PaymentMethod {<!-- -->
public void processPayment() {<!-- -->
// Logic for handling cash payments
}
}
Parallel Inheritance Hierarchies (parallel inheritance system)
There are multiple parallel inheritance relationships in the code, resulting in increased code complexity
public class Shape {<!-- -->
//Shape properties and methods
}
public class Circle extends Shape {<!-- -->
// Properties and methods of Circle
}
public class Rectangle extends Shape {<!-- -->
// Properties and methods of Rectangle
}
public class RedCircle extends Circle {<!-- -->
// RedCircle-specific properties and methods
}
public class BlueCircle extends Circle {<!-- -->
//BlueCircle-specific properties and methods
}
public class RedRectangle extends Rectangle {<!-- -->
// RedRectangle-specific properties and methods
}
public class BlueRectangle extends Rectangle {<!-- -->
// BlueRectangle-specific properties and methods
}
//After reconstruction
//Use techniques such as object composition, interfaces and design patterns to restructure code and reduce the inheritance hierarchy of classes
public interface Shape {<!-- -->
void draw();
}
public class Circle implements Shape {<!-- -->
// Implementation of Circle's properties and methods
public void draw() {<!-- -->
//The logic of drawing a circle
}
}
public class Rectangle implements Shape {<!-- -->
// Implementation of Rectangle's properties and methods
public void draw() {<!-- -->
//The logic of drawing a rectangle
}
}
public class ColorShape implements Shape {<!-- -->
private Shape shape;
private String color;
public ColorShape(Shape shape, String color) {<!-- -->
this.shape = shape;
this.color = color;
}
public void draw() {<!-- -->
shape.draw();
System.out.println("Color: " + color);
}
}
Lazy Class (redundant class)
There are invalid, redundant, or unnecessary classes in the code that do not actually contribute to the functionality of the code, but instead increase the complexity and maintenance cost of the code.
public class Calculator {<!-- -->
public int add(int a, int b) {<!-- -->
return a + b;
}
}
public class AdvancedCalculator extends Calculator {<!-- -->
public int subtract(int a, int b) {<!-- -->
return a - b;
}
}
public class ScientificCalculator extends AdvancedCalculator {<!-- -->
public double sin(double angle) {<!-- -->
return Math.sin(angle);
}
}
public class Main {<!-- -->
public static void main(String[] args) {<!-- -->
ScientificCalculator calculator = new ScientificCalculator();
int result1 = calculator.add(1, 2);
int result2 = calculator.subtract(5, 3);
double result3 = calculator.sin(Math.PI / 2);
System.out.println(result1);
System.out.println(result2);
System.out.println(result3);
}
}
//After reconstruction
public class Calculator {<!-- -->
public int add(int a, int b) {<!-- -->
return a + b;
}
public int subtract(int a, int b) {<!-- -->
return a - b;
}
public double sin(double angle) {<!-- -->
return Math.sin(angle);
}
}
public class Main {<!-- -->
public static void main(String[] args) {<!-- -->
Calculator calculator = new Calculator();
int result1 = calculator.add(1, 2);
int result2 = calculator.subtract(5, 3);
double result3 = calculator.sin(Math.PI / 2);
System.out.println(result1);
System.out.println(result2);
System.out.println(result3);
}
}
Speculative Generality(exaggerating about the future)
The code uses technologies or designs that are overly complex, highly abstract, or may be used in the future but are not actually needed at the moment.
@Service
public class FutureEcommerceService {<!-- -->
private ProductService productService;
private OrderService orderService;
public CompletableFuture<Order> placeOrderAsync(String productId, int quantity) {<!-- -->
CompletableFuture<Product> productFuture = CompletableFuture.supplyAsync(() ->
productService.getProductById(productId)
);
CompletableFuture<Order> orderFuture = productFuture.thenCompose(product ->
CompletableFuture.supplyAsync(() ->
orderService.createOrder(product, quantity)
)
);
return orderFuture.thenCombine(productFuture, (order, product) -> {<!-- -->
orderService.sendOrderConfirmation(order);
productService.updateStock(product, quantity);
return order;
});
}
}
//After reconstruction
//Simplify the functionality of asynchronous operations and merging results to make them universal
@Service
public class EcommerceService {<!-- -->
private final ProductService productService;
private final OrderService orderService;
public EcommerceService(ProductService productService, OrderService orderService) {<!-- -->
this.productService = productService;
this.orderService = orderService;
}
public Order placeOrder(String productId, int quantity) {<!-- -->
Product product = productService.getProductById(productId);
Order order = orderService.createOrder(product, quantity);
orderService.sendOrderConfirmation(order);
productService.updateStock(product, quantity);
return order;
}
}
Temporary Field (confusing temporary field)
@Service
public class ConfusingEcommerceService {<!-- -->
@Autowired
private ProductService productService;
@Autowired
private OrderService orderService;
private String productId;
private int quantity;
public void setProductId(String productId) {<!-- -->
this.productId = productId;
}
public void setQuantity(int quantity) {<!-- -->
this.quantity = quantity;
}
public void placeOrder() {<!-- -->
Product product = productService.getProductById(productId);
Order order = orderService.createOrder(product, quantity);
orderService.sendOrderConfirmation(order);
productService.updateStock(product, quantity);
}
}
//After reconstruction
//The internal fields of the class become method input parameters
@Service
public class EcommerceService {<!-- -->
private ProductService productService;
private OrderService orderService;
public Order placeOrder(String productId, int quantity) {<!-- -->
Product product = productService.getProductById(productId);
Order order = orderService.createOrder(product, quantity);
orderService.sendOrderConfirmation(order);
productService.updateStock(product, quantity);
return order;
}
}
Message Chains (overcoupled message chains)
Call chains between multiple layers of objects appear in the code, making objects too tightly coupled.
@Service
public class OverlyCoupledEcommerceService {<!-- -->
public void processOrder(String userId, String productId, int quantity) {<!-- -->
User user = userService.getUserById(userId);
Product product = productService.getProductById(productId);
Cart cart = user.getCart();
cart.addItem(product, quantity);
Order order = cart.checkout();
orderService.placeOrder(order);
}
}
//After reconstruction
//The order processing method is divided into steps: 1. Prepare data, 2 create the order, 3 submit the order
@Service
public class EcommerceService {<!-- -->
public void processOrder(String userId, String productId, int quantity) {<!-- -->
User user = userService.getUserById(userId);
Product product = productService.getProductById(productId);
Order order = createOrder(user, product, quantity);
placeOrder(order);
}
private Order createOrder(User user, Product product, int quantity) {<!-- -->
Cart cart = user.getCart();
cart.addItem(product, quantity);
return cart.checkout();
}
private void placeOrder(Order order) {<!-- -->
orderService.placeOrder(order);
}
}
Middle Man(middleman)
A code that overuses delegation (more than half of the methods in a class are delegated to other classes)
Inappropriate Intimacy
The relationship between the two classes is too close, and one class depends on the internal implementation details of the other class, resulting in a high degree of coupling.
public class Customer {<!-- -->
private List<Order> orders;
public void addOrder(Order order) {<!-- -->
// Depends on the internal logic of the Order class
if (order.getTotalAmount() > 1000) {<!-- -->
// Process large orders
}
orders.add(order);
}
}
public class Order {<!-- -->
private double totalAmount;
public double getTotalAmount() {<!-- -->
return totalAmount;
}
}
//After reconstruction
//The Customer class relies too much on the internal implementation details of the Order class, resulting in an intimate relationship.
//The direct dependence of the Customer class on the Order class should be reduced and an intermediate layer can be introduced.
public class Customer {<!-- -->
private List<Order> orders;
private OrderProcessor orderProcessor;
public void addOrder(Order order) {<!-- -->
// Call OrderProcessor for order processing
orderProcessor.processOrder(order);
orders.add(order);
}
}
public class Order {<!-- -->
private double totalAmount;
public double getTotalAmount() {<!-- -->
return totalAmount;
}
// other methods and properties
}
public class OrderProcessor {<!-- -->
public void processOrder(Order order) {<!-- -->
// Process order logic and do not directly rely on the internal implementation details of Order
if (order.getTotalAmount() > 1000) {<!-- -->
// Process large orders
}
// other processing logic
}
}
Alternative Classes with Different Interfaces (similar approaches but the same purpose)
Refers to the existence of multiple classes in a system that provide similar or identical functions but have different interfaces, resulting in code confusion
Use abstract classes to carry common methods and use interfaces to define common methods
Incomplete Library Class (imperfect library class)
It means that a class in a library or external dependency lacks certain functions and needs to be supplemented in the application.
For example, StringUtil lacks a method to convert strings to lowercase.
Refactor: add this method
Refused Bequest(rejected legacy)
For a certain subclass, it only wants to inherit some functions and data of the base class, and does not need all the content provided by the base class.
public class Product {<!-- -->
private String name;
private String description;
private double price;
public Product(String name, String description, double price) {<!-- -->
this.name = name;
this.description = description;
this.price = price;
}
public double calculateDiscountedPrice(double discountPercentage) {<!-- -->
return price * (1 - discountPercentage / 100);
}
// Getters and setters for name, description, and price
}
public class ElectronicProduct extends Product {<!-- -->
// ElectronicProduct-specific attributes
//Constructor
// Getters and setters for ElectronicProduct-specific attributes
}
//After reconstruction
//Use composition instead of inheritance
public class Product {<!-- -->
private String name;
private String description;
private double price;
public Product(String name, String description, double price) {<!-- -->
this.name = name;
this.description = description;
this.price = price;
}
public double calculateDiscountedPrice(double discountPercentage) {<!-- -->
return price * (1 - discountPercentage / 100);
}
// Getters and setters for name, description, and price
}
public class ElectronicProduct {<!-- -->
private Product product; // Use composition instead of inheritance
public ElectronicProduct(String name, String description, double price) {<!-- -->
this.product = new Product(name, description, price);
}
// ElectronicProduct-specific attributes and methods
public double calculateDiscountedPrice(double discountPercentage) {<!-- -->
// Implement your own price calculation logic in ElectronicProduct
return product.calculateDiscountedPrice(discountPercentage);
}
// Getters and setters for ElectronicProduct-specific attributes
}