Dealing with a class that has organically grown too large

Disclaimer: This is live, production code for my hobby project: LSML (GitHub). Due to the size of the code I'm unable to include everything in this review.

The application is a tool for configuring loadouts for an online mech game.

Brief summary of the problem domain

A mech has 8 components: Head, Lt/Rt/C Torso, Lt/Rt Leg and Lt/Rt Arm that each can be individually configured with equipment and armor.

A mech's effectiveness (discounting tactics and strategy of the player) is defined by what equipment it has and how it is distributed.

This software allows creation of different loadouts with equipment and armor and performing statistical analysis on them.

Constraints

There are three main constraint types on loadouts.

• Space (Slots) - Equipment occupy space, and space is limited in components.

• Weight (Mass) - The mech has a max carrying capacity which must not be exceeded.

• Hard points - Certain equipment require the presence of a matching hard point on the component to be equippable.

Hard points and maximum weight is defined by a chassis. Each chassis has different characteristics when it comes to movement, hard points, and other constraints.

Upgrades

A mech can have so called "upgrades" which affect global properties of the mech. For example the internal structure can be "standard" or "endo-steel" where endo steel is a space/mass trade off. Similarily other upgrades for mech's cooling, armor and missile guidance can be installed that will affect the performance of the mech.

Modifiers and weapon groups

It is worth noting that many attributes of items and the mech can be altered by modifiers. These may be anything from special equipment that enhances some properties of other equipment, to player level (called Efficiencies) based.

Weapons can also be grouped and fired in groups. This is a vital part of game play and effective piloting, these weapon groups are modeled and statistics are calculated per group.

All of the above, weapon groups, player

Code amount is substantial. So you need to refactor in small steps in order not to break everything.

If a class has grown organically large, it probably has to many responsibilities. Identify those and move them out.

For example, loadoutXstream seems like some sort of serialization method. Serialization is an orthogonal concern and should be moved out of a class whenever possible. The fact that it's static is both an indication and also makes it easy to move it out.

Keeping with the static topic, some methods are practically static. Public methods that do not implement a (necessarily) abstract method from a super type, which need not have intimate knowledge of the private fields, but instead interacts with its class through its public interface; namely a low-cohesion method; is a method that exhibits all tenets of a static method but is not made one.

Take a look at how I started to change canAddModule method:

public EquipResult canAddModule(PilotModule aModule) {
    if (getModules().contains(aModule))
        return EquipResult.make(EquipResultType.ModuleAlreadyEquipped);
    if (!aModule.getFaction().isCompatible(getChassis().getFaction()))
        return EquipResult.make(EquipResultType.NotSupported);

    boolean notEnoughSlots = new SlotCapacityPolicy().notSatisfied(this, aModule);
    if (notEnoughSlots)
        return EquipResult.make(EquipResultType.NotEnoughSlots);

    // TODO: Apply any additional limitations on modules
    return EquipResult.SUCCESS;
}

public static class SlotCapacityPolicy {
    public boolean notSatisfied(LoadoutBase loadout, PilotModule module) {
        final boolean canUseHybridSlot = module.getSlot() == ModuleSlot.WEAPON || module.getSlot() == ModuleSlot.MECH;

        final boolean isHybridSlotFree = !(loadout.getModulesOfType(ModuleSlot.MECH) > loadout.getModulesMax(ModuleSlot.MECH)
                || loadout.getModulesOfType(ModuleSlot.WEAPON) > loadout.getModulesMax(ModuleSlot.WEAPON));

        boolean notEnoughSlots = loadout.getModulesOfType(module.getSlot()) >= loadout.getModulesMax(module.getSlot())
                && (!canUseHybridSlot || !isHybridSlotFree);

        return notEnoughSlots;
    }
}

If you keep moving things out, you can change this:

EquipResult.SUCCESS == currentLoadout.canAddModule(module)

to this:

rules.canAddModule(currentLoadout, module)

getQuirkHtmlSummary is view logic (and evidently so is modifier.describeToHtml(sb)), and just as serialization logic, should be moved out of the model class. Because it's abstract, it requires different techniques to be removed. You can construct a Loadout in a factory using components. In this case one such component would be StandardHtmlRenderingStrategy or OmniSomethingHtmlRenderingStrategy. Or you can lookup loadout rendering strategy at runtime for a given loadout. (just as abstract does, but explicitly)

Eventually, instead of loadout.getQuirkHtmlSummary() do a quirksRenderer.render(loadout).

StackExchange Code Review Q#107320, answer score: 4