Reply

IntelliJ IDEA: Inspections by Sections - Abstraction Issues

At 7:58 AM on Feb 19, 2007, Rob Harwood wrote:

Intro

Static code analysis doesn't just improve your code quality, it can also teach you some cool ideas and best practices about programming. In this series of tips, titled Inspections by Sections, I'll show you highlights from over 600 code inspections that come with IntelliJ IDEA 6.0 , split up by the sections found in the settings dialog (File > Settings > Errors). Some inspections are on by default, but many are not and you may need to turn them on according to your personal or team-level preferences.

The section I'll tackle this week is the first section, Abstraction Issues. This collection of inspections deals with object-oriented design issues related to dividing up responsibilities between various classes in your project. The intent is to keep the level of abstraction as high as possible so that your design can be flexible and robust. This section contains 17 separate inspections, of which I'll show 5 representative examples.

Magic Numbers

The simplest example is the classic 'magic number' code smell. This is a common design error for beginners, as well as programmers who are in a rush. The problem is that numbers can mean anything, so if you want to have a good, understandable, and maintainable design, any significant number should be given some sort of name to identify what it is for. You don't want to use magic numbers especially because of the dreaded cut-and-paste-and-modify style of programming. For example, if the number 65 is used in several similar calculations, and you want to change that to 75, you might modify some instances but miss others, and you end up with hard-to-find mathematical errors and inconsistencies. The Magic Number inspection will find magic numbers and highlight them in your code.

In this example, we are testing a database to see if it is in a particular error state, signified by the number '65'. Unfortunately, the number 65 doesn't tell us much about what the error code is for, which makes this code difficult to understand unless you know what error 65 is supposed to represent. A better design would be to use the quick-fix intention action (Alt-Enter) to perform an Introduce Constant refactoring which will create a private static final field which we can name properly, e.g. CONNECTION_LOST_ERRORCODE.

Feature Envy

One of the central purposes of object-oriented design is to split up the various responsibilities of the code into cohesive classes. When one class does all the work and merely uses other classes as glorified data holders, you have an unbalanced design that is likely to be very difficult to modify in any significant way. To improve the design, it will be necessary to find similar responsibilities and move them into their own appropriate classes. That is what the Feature Envy inspection helps you do. It identifies any method in one class that calls methods in another class three or more times. In such cases, this is a good indication that this functionality could be profitably moved to the other class, as it appears that the other class is the more appropriate class to handle this functionality.

In this example, the Big class contains a method that accesses the Little class with three or more method calls. This design could be improved by using the Move Method refactoring to move the search() method to the Little class, since it appears that searching is more closely related to the Little class than the Big class.

Polymorphism

One of the basic techniques of object-orientation is polymorphism, which is the ability to override methods from a superclass (or implement methods from an interface) so that subclasses can have different behaviour from one another. Everybody uses polymorphic method calls, but even today many programmers miss opportunities to harness the flexibility of polymorphism in their own designs. A common way to get it wrong is to use an if-else chain of instanceof checks to perform different actions depending on the type of some variable, which is what the Chain of 'instanceof' Checks inspection will detect. Such chains are error-prone, hard to read, and hard to maintain. They also make the classes more coupled and the design harder to change. There are rare cases where instanceof is truly needed, such as in equals(), but usually polymorphic method calls are a better design.

In this example, a game is designed with many different types of GamePiece, from Player, to Monster, to Robot, and the programmer has used instanceof to determine how each piece should move, based on its type. A better design would be to use the Move Method refactoring to move the movePlayer() method to the Player class, the moveRobot() method to the Robot class, etc. Then each of these methods could be renamed to simply 'move' and the instanceof chain could be replaced with a polymorphic method call 'piece.move()'. The design is much cleaner and more flexible, especially if new kinds of GamePieces are added to the game as it is developed. Without polymorphism, each instanceof chain would need to be modified and maintained each time a new GamePiece subclass is added.

Concrete Declarations

One way you can tell if your design is flexible and robust is to try to test it. Clunky designs are hard to write unit tests for and, conversely, if you design your code to be testable, it will usually be a much better design. A perfect example is when you are trying to use mock objects in your tests, but the code under test uses concrete declarations for its fields, method parameters, method return-types, etc. With such concrete declarations, the mock object cannot easily replace the live object because it is of a different concrete class. There are several related inspections to detect such concrete declarations and highlight them for you. Depending on your preferences, you may want to turn some on and others off.

In this example (using the inspection called Method Parameter of Concrete Class) the scanDatabase() method takes an instance of the concrete class DatabaseImpl as a parameter. This ties the method to the implementation details of DatabaseImpl, and makes it difficult to do things like testing with a mock database object. Instead, you could perform an Extract Interface refactoring on DatabaseImpl to give it an interface (such as Database) with all the required methods needed by the scanDatabase() method. With this design, it is easy to write a unit test that passes in a mock database object so that the unit tests are not dependent on the actual database, but can run independently of it.

Overly-Strong Type Cast

Speaking of dependencies, advanced programmers realize the importance of minimizing unnecessary dependencies between classes, especially when it comes to organizing your code into appropriate packages and layers. The purpose of the Overly-Strong Type Cast is to identify just such unnecessary dependencies in the specific case where you are down-casting too low in a class hierarchy (such as casting to ArrayList when casting to List would be sufficient). In such cases, you should cast to the most abstract type possible which can still acheive the required functionality, and in that way you will keep your design less coupled. The deeper down you cast, the more dependencies you take on. The more dependencies you take on, the more frequently you will be affected by changes to other people's code, making your own code less stable to change. Also, the more concrete your dependencies, the more difficult to reuse the code for modified purposes. For instance if someone wants to use a LinkedList instead of ArrayList, that would cause a casting exception.

In this example, we are searching for an employee in a database to get the age of the employee. The database only returns Objects, so we need to cast the Object down to an Employee. However, the getAge() method is declared in the Person interface, which Employee extends. Therefore, the cast to Employee could be replaced with a cast to Person instead. In fact there is a quick-fix intention action (Alt-Enter) to make the replacement for you. Now this method will be able to handle other kinds of Person objects as well, such as Customers. Furthermore, this method can now be more easily moved out of the 'payroll' package and into a more stable and generic 'business' package, since it no longer contains a dependency on the Employee class.

Find out more about IntelliJ IDEA 's unparalleled code analysis features , or download IntelliJ IDEA with a free 30-day evaluation license to try it yourself.