Code Smells and Refactorings - ViralKaos/Frozen-Bubble-Android-port-x86 GitHub Wiki

Bloater -FrozenBubble.java

The FrozenBubble class has a Bloater code smell. We notice a lot of the Bloaters’ symptoms such as Large Class, Data Clumps and Primitive Obsession. For instance, there’s a Data Clumps code smell from the fields adsOn to targetMode. We can use Extract Class and Move Method to replace these fields by a single field representing the Preferences class.

  1. We select the fields from adsOn to targetMode and extract a class that we call Preferences.
  2. In the Preference class, we change the attributes’ protection level to private.
  3. Since there are set and get methods for each attribute that has been extracted into the Preference class, we get create a get and a set method (i.e. getPreference(Preference preference), setPreference(Preference preference)) in the FrozenBubble class.
  4. We then move the getters and setters of each attribute into the Preference class.

Message Chain - onPreparingOptionsMenu()

In the FrozenBubble class, a Message Chain code smell can be observed in the onPreparingOptionsMenu() method. Using Extract Method and Move Method will allow us to isolate the chain and move it the class from which the chain starts.

  1. We are going to use Extract Method to extract the chain findItem(x).setVisible(y) into a method called setItemVisible(item x, bool y);
  2. We are going to use Move Method to move the method into the Menu class. This way, the FrozenBubble class only calls one Menu class method.

Switch Statement - FrozenBubble.java

Still in the FrozenBubble class, we notice that there is a Switch Statement code smell in the onOptionsItemSelected(MenuIntem item) method. We can use Replace Type Code with State/Strategy and Replace Conditional with Polymorphism to remedy this.

  1. Using Replace Type Code with State/Strategy, we create a class EditorType and subclasses of the EditorType class because it is an object of the class Editor that acts differently in the switch statement. Each subclass will correspond to a case in the switch statement.
class EditorType{...}
    class EditorNewGame extends EditorType{...}
  1. In the EditorType class, we create an abstract getTypeCode() method.
  2. In the Editor class, we create an instance variable type of belonging to the EditorType class and different constant values for each switch statement case. We also create a setType() method which will call a factory method from the EditorType class to create the appropriate subclass object.
class Editor
    {
      EditorType type;
      public final static int MENU_COLORBLIND_ON = 1;
      public final static int MENU_NEW_GAME = 8;
      
      int setType(int value)
      {
        type = EmployeeType.newType(value);
      }
    }
    
    class EditorType
    {
      newType(int value)
      {
        switch(value)
        {
          case MENU_NEW_GAME: return new EditorNewGame();
                              break;
          …
        }
      }
    }
  1. In each subclass, we override the getTypeCode() method to return one of the constant values respective to the appropriate type of subclass.
class EditorNewGame extends EditorType
    {
      int getTypeCode()
      {
        return Editor.MENU_NEW_GAME;
      }
    }
  1. In the Editor class, we create a getType() method which returns one of the constant values respective to the appropriate type of subclass by calling getTypeCode() of the variable type.
int getType()
    {
      type.getTypeCode();
    }

  1. Using Replace Conditional with Polymorphism, we create an abstract method onEventItemSelected() in the EdirtorType.

  2. In each subclass, we override the onEventItemSelected() method with the appropriate class behavior.

  3. We change the switch statement method to call the onEventItemSelected() of the type variable of the Editor object, and it will call the appropriate subclass method.

editor.type.onEventItemSelected();
  1. Since we now have a Message Chain code smell, we will use Extract Method to create a class in the Editor class as follows:
void onEventItemSelected
    {
      this.type.onEventItemSelected();
    }

Bloater - BubbleSprite.java

The BubbleSprite class also has a Bloater code smell. We can observe Data Clumps, Large Class, Long Parameter List, Long Method and Primitive Obsession. We notice the Data Clumps for the variables moveX and moveY, which are always used together, as well as realX and realY. We will use Extract Class to replace the fields with proper Coordinates object.

  1. We select the moveX and moveY fields and extract a class that we call Coordinates with the fields as attributes. The new field in the BubbeSprite class is called moveCoordinates.
  2. We replace the fields realX and realY with the field realCoordinates and replace the variables realX and realY with realCoordinates.x and realCoordiantes.y respectively.

Data Clumps

Similarly, in FrozenBubble, there’s a Data Clumps code smell from the fields adsOn to targetMode. We can use Extract Class to replace these fields by a single field representing the Preferences class.

  1. We select the fields from adsOn to targetMode and extract a class that we call Preferences.
  2. In the Preference class, we change the attributes’ protection level to private.
  3. Since there are set and get methods for each attribute that has been extracted into the Preference class, we get create a get and a set method (i.e. getPreference(Preference preference), setPreference(Preference preference)) in the FrozenBubble class.
  4. We then move the getters and setters of each attribute into the Preference class.

Long Method - SplashScreen.java

Long Class

There exist only a few classes doing most of the work and this resulted in classes with large amounts of attributes, several of which are public constants, lots of unrelated methods, and several very long methods. This also leads to several other smells which will be described below and refactoring for those should help reduce the length of several classes.

Primtive Obsession

The reason for the large amount of attributes comes from a large number of primitives being used, mostly of type int to store ID numbers, or type codes, to be used when setting up the environment. For example, the FrozenBubble class contains 30 such attributes while the FrozenGame class contains 16 plus several other attributes needed by each class. By using Replace Type Code with Class, the amount of primitive type attributes being used can be greatly reduced as well as reducing the overall length of the classes.

Innapropriate Intimacy

A side effect from building classes that try to do everything is that they can sometimes end up retaining data used primarily by other classes. In this case, FrozenBubble, the main class responsible for setting up the environment in which the game runs, also holds the set of ID numbers that will be used by SoundManager, the class responsible for playing the sounds in the game. Therefore, whenever a sound needs to be played a called has to be made to SoundManager and the ID held by FrozenBubble is passed as a parameter. This creates an unnecessary coupling between the two classes.

From FrozenBubble.java:
  public final static int SOUND_WON       = 0;
  public final static int SOUND_LOST      = 1;
  public final static int SOUND_LAUNCH    = 2;
  public final static int SOUND_DESTROY   = 3;
  public final static int SOUND_REBOUND   = 4;
  public final static int SOUND_STICK     = 5;
  public final static int SOUND_HURRY     = 6;
  public final static int SOUND_NEWROOT   = 7;
  public final static int SOUND_NOH       = 8;
  public final static int SOUND_WHIP      = 9;
  public final static int NUM_SOUNDS      = 10;
  
Sample call - from FrozenGame.java:
  soundManager.playSound(FrozenBubble.SOUND_NOH);
  
From SoundManager.java - Constructor:
  public SoundManager(Context  context)
  {
    this.context = context;
    soundPool = new SoundPool(4, AudioManager.STREAM_MUSIC, 0);
    sm = new int[FrozenBubble.NUM_SOUNDS];
      
    sm[FrozenBubble.SOUND_WON]       = ...;
    sm[FrozenBubble.SOUND_LOST]      = ...;
    sm[FrozenBubble.SOUND_LAUNCH]    = ...;
    sm[FrozenBubble.SOUND_DESTROY]   = ...;
    sm[FrozenBubble.SOUND_REBOUND]   = ...;
    sm[FrozenBubble.SOUND_STICK]     = ...;
    sm[FrozenBubble.SOUND_HURRY]     = ...;
    sm[FrozenBubble.SOUND_NEWROOT]   = ...;
    sm[FrozenBubble.SOUND_NOH]       = ...;
    sm[FrozenBubble.SOUND_WHIP]      = ...;
  }

The main refactoring that needs to be applied here is Move Field to move the IDs into the SoundManager class so that it may have all the data it needs available from within itself.

Table of Contents

Add a custom footer
⚠️ **GitHub.com Fallback** ⚠️