Identifying the Need for Refactoring (Code Smells)

Key Insight

Refactoring is a crucial strategy for upholding the Cardinal Rule of Software Evolution, defined as 'a change made to the internal structure of the software to make it easier to understand and cheaper to modify without changing its observable behavior.' Various 'smells' or warning signs indicate where refactoring is necessary. Prominent among these is duplicated code, which almost invariably signifies a failure in initial design factoring and directly violates the 'Don't Repeat Yourself' (DRY) principle, leading to burdensome parallel modifications; 'Copy and paste is a design error.' Routines that are excessively long, such as object-oriented routines exceeding a screen in length or legacy routines like one reduced from over 12000 lines to about 4000 lines, severely hinder modularity. Similarly, long or deeply nested loops are prime candidates for conversion into dedicated routines to improve code structure and reduce complexity.

Class design issues also manifest as code smells. Poor class cohesion, where a class assumes ownership of unrelated responsibilities, dictates breaking it into multiple classes, each with a focused and cohesive set of tasks. Class interfaces can lose their initial consistency due to expedient modifications, evolving into 'Frankensteinian maintenance monsters' that impair intellectual manageability. An abundance of parameters in a routine's list indicates that its abstraction has not been thoroughly considered. Furthermore, if changes within a class are frequently compartmentalized to only one part, it signals the class likely has multiple distinct responsibilities and should be split. Parallel modifications across multiple classes, such as modifying 15 classes for each new output type, or similar changes to inheritance hierarchies and 'case' statements, strongly suggest that the code could be reorganized, potentially using polymorphism, to localize changes.

Additional smells include related data items used together but not structured into a class, or a routine that predominantly uses features of another class, implying it should be moved. Overloading primitive data types, like using an integer for both 'Money' and 'Temperature,' can lead to erroneous assignments ('bankBalance = recordLowTemperature') and warrants conversion to dedicated classes for type checking and added behavior. Classes that perform very little might be eliminated by reassigning their responsibilities. The presence of 'tramp data'—data passed through routines only to be forwarded—suggests inconsistent interface abstractions. Middleman objects that merely delegate calls to other classes might be redundant. Overly intimate classes violate encapsulation. Poor routine names, public data members, subclasses using only a small percentage of parent routines (indicating a 'has-a' relationship should replace 'is-a'), comments explaining difficult code (suggesting 'Don't document bad code—rewrite it'), and the use of global variables all signal opportunities for internal quality improvement. Routines requiring extensive setup or takedown code (e.g., multiple Set and Get calls for a WithdrawalTransaction) indicate a poorly abstracted interface that could benefit from passing an object or specific fields directly. Lastly, writing speculative 'design ahead' code for future, uncertain requirements is problematic, often resulting in wasted effort, increased complexity, and project slowdowns; clarity in current code is the most effective preparation for future needs.