1,184 reads

The Dirty Code Problem: Improve Your Game with Good Naming Practices

by Chad BefusJanuary 29th, 2020

Too Long; Didn't Read

The Rule: Whenever you name a variable, function, or class, ask if it is concise, honest, expressive, and complete. Using good names means we allow our reader to stop reading at higher levels of abstraction, without digging into details to understand how something works. There are some different rules for naming variables, functions, and classes, but there are 4 basic commonalities. Names should be: Concise,Honest, Expressive, Complete, Concise and Concise. Names for variables, classes, and actors should be named after what they are intended to do.

Company Mentioned

featured image - The Dirty Code Problem: Improve Your Game with Good Naming Practices

The Rule: Whenever you name a variable, function, or class, ask if it is concise, honest, expressive, and complete.

The Explanation

Consistently writing good names is hard, but reading and understanding bad names is harder. Using good names means we allow our reader to stop reading at higher levels of abstraction, without digging into details to understand how something works. In a small code base it isn't too expensive to dig into internals, but once a code base grows into the tens of thousands of lines or more, it's unreasonable for us to expect our readers to read it all.

While there are some different rules for naming variables, functions, and classes, there are also 4 basic commonalities. Names should be:

Concise: They should not be wasteful. This means we don't add on meaningless words or unnecessary context.
Honest: They should not lie. This means a name should not hide, or misinform on any part of what it represents.
Expressive: They should express intent. This means a name should accurately and clearly describe what it represents.
Complete: The shouldn't use short forms or acronyms. This means we don't expect our readers to know the same encodings that we do.

That being said, we should discuss the difference between names for variables, names, and classes.

Variable Names

A variable name should be based on how the variable should be used and not with it's type (I will follow more on this in a later article about not encoding type into names). This not only helps readability but also lets a reader what can be done with the variable.

An example of a terrible variable name might look like:

final List<Integer> valueList = new ArrayList<>();

Here the author used the word

value

, which is meaningless (breaking Rule 1, be Concise) and the word

List

which encodes type. Together, these words convey no intent, breaking rule 3, be Expressive. Furthermore, encoding the type like this opens us to the risk where we end up with code that looks like:

final Set<Integer> valueList = new HashSet<>();

Because someone decided using a set was better than a list, we have a meaningless word combined with a lie (Violating my second rule; be Honest). Remembering to also rename the variable would have been nice, but it's an easy mistake to make.

A better name would look like:

final List<Integer> accountIdsToLookup = new ArrayList<>();

This name indicates that there are multiple values through the plurality of

accountIds

but also does not tie us to an underlying data structure. I recommend this for all iterable type objects. Furthermore, this name is expressive, as it indicates what the variable contains and what it should be used for.

For key-value data structures like maps (dictionaries, etc.) I recommend naming schemes like

keysToValues

valuesByKeys

final Map<Integer, Account> accountsMap = new HashMap<>(); // bad
final Map<Integer, Account> accountsById = new HashMap<>(); // good

Note how the second line informs you about the meaning of both the key and the value. The first also encodes the map type into the name which will have to change if we switch to a different Key-Value storage scheme (Don't Encode Type into names).

Function Names

A function name should be an action (a Verb) not a thing (a Noun). Functions execute change, naming them after things is dishonest. Furthermore, functions are the driving steps of the story or your program, so when we read each step, we want it to read as if something is changing. Actions denote change.

A bad function name can be seen in this class:

public class Company {
    private final String name;
    private final int taxId;

    public Company(final String name, final int taxId) {
        this.name = name;
        this.taxId = taxId;
    }

    public String json() {
        return new StringBuilder()
            .append("{")
            .append("'name': "+this.name)
            .append(",")
            .append("'taxId': "+this.taxId)
            .append("}")
            .build();
    }
}

The function

json

here reads as if it is a property of the class instead of a function. It is not named as a doer, but rather a thing, which is dishonest (Breaking rule 2). A better name would be something like

to_json

which indicates that the function is taking some action on the

Company

class of which it is a member.

As a side note, the authors are using a

StringBuilder

here to convert this class to JSON (Javascript Object Notation).

This makes for an easy example, and I have indeed seen this in the real world, but it is not the recommended method.

Instead I would recommend using a Serialization/De-serialization library like FasterXML's Jackson or Google's Gson.

Class Names

There are two types of classes, data structures and actors (I'll discuss this further in a later article about not building hybrids), and each should be named differently.

Actor Classes

Actors should be named after what they are intended to do. This is much easier if they are only able to do one thing (and I'll discuss this later in an article about doing one thing).

An example of a poorly named actor might be:

public class Authentication {
    ...
    public boolean validateSession(final Session session) {
        ...
    }
}

In this case, it appears that the class denotes an authentication data-structure, and as such, we expect it to have some accessors and/or properties but not any abilities. When we see the function call-site

validateSession

it will read poorly like:

final Authentication authentication = new Authentication(...);
...
if (authentication.validateSession(session)) {
    ...
}

This would read significantly less awkward if I swapped out the class name to

Authenticator

final Authenticator authenticator = new Authenticator(...);
...
if (authenticator.validateSession(session)) {
    ...
}

By its name, it is clear that the

Authenticator

is an actor, where having responsibilities makes sense.

Data Structures

Data structures can be generic or specific. A generic data structure can hold and give access to any other type of data (for example a List can be a List of Integers). A specific data structure holds a very carefully designed scheme of information (for example a Customer object).

A generic should be named after how they can be accessed. Words like Map, List, or Set are helpful in this way. Most languages come with built in generics like

HashMap

TreeSet

, and

LinkedList

in Java.

Specific data structure objects should be named after the object they encapsulate, and not Actors.

A badly named specific data structure might look like:

public class Authenticator {
    ...
    public String getSessionKey() {
        return this.sessionKey;
    }
}

From the name, this class appears to be an actor, which means a call to

getSessionKey

makes us think this function is doing something (like accessing a database). Instead, a better class name would be

Authentication

so that the call-site reads nicely as:

final Authentication authentication = new Authentication(...);
...
final String sessionKey = authentication.getSessionKey();

Note how it's much more apparent this function is only accessing the

sessionKey

property of the

authentication

data structure.

The Example

In our example we are looking at code designed to calculate a score for a given bowling game. A game of bowling is defined as a series of ten frames. For each frame a player gets two rolls to attempt to knock down as many pins as possible. Usually, there are a total of ten pins. If a player knocks down 4 pins on their first roll, and 4 pins on their second, they are given a score of 8 for that frame.

If a player knocks down all the pins on the first roll it is called a strike. A strike is worth 10 points plus the value from the next two rolls. If a player doesn't knock down all of their pins in their first roll, but proceeds to do this with their second roll, it is called a spare. A spare is worth 10 points plus the value from the next roll.

The final frame of the game (the tenth frame) works a bit differently. A player can roll up to three times, getting a fresh set of pins if they strike or spare. If the player gets neither a strike or spare in the tenth frame they will only get to roll twice.

To model the input of this problem into our example we are taking a list of frames, each of which is represented by a list of the count of pins knocked down. An example input (represented in JSON) might look like:

[[4, 4], [10], [3, 7], [10], [10], [10], [10], [10], [10], [2, 8, 5]]

In this game, the user knocked down a total of 8 pins in their first frame. They then got a strike in the second frame, and a spare in the third. The player then bowled a series of perfect frames (all strikes) until the last frame where they got a spare followed by knocking 5 pins down. The total score for this game is 225.

Because this code was written to power a game board display at a bowling alley, the example also handles incomplete games. The input for an incomplete game might look like:

[[4, 4], [10], [3]]

This is just the beginning of the completed game above, but at this point the score is 24.

I am going to start by showing you the complete code written to solve this problem. This code works, and has an okay design, but the naming is atrocious. I will then iteratively walk through the parts of the code, cleaning up the naming with discussions on why I make the changes I do. Finally I will display the cleaned up code in its entirety.

The hope is that, when you compare the original code to the cleaned up code, you will agree it is much easier to follow, despite the fact that I am only applying the rules of good naming from this chapter.

The Dirty Code

public static class Consts {
    public static int PPF = 10;
    public static int NOF = 10;
}

public class BowlingScore {

    private final FrameBuilder frameBuilder;
    private final RollPropagation rollPropagation;

    public BowlingScore(
            final FrameBuilder frameBuilder, 
            final RollPropagation rollPropagation
    ) {
        this.frameBuilder = frameBuilder;
        this.rollPropagation = rollPropagation;
    }

    public static BowlingScore build() {
        return new BowlingScore(
            new FrameBuilder(), 
            new RollPropagation());
    }

    public int calcScore(final List<List<Integer>> rollPairList) {
        final List<Frame> values = new ArrayList<>();
        for (final List<Integer> rollPair : rollPairList) {
            this.rollPropagation.add(rollPair);
            values.add(
                this.frameBuilder.func(
                    rollPair, 
                    this.rollPropagation)
            );
        }
        return values.stream().reduce(0, (s, f) -> s + f.score());
    }
}

public class RollPropagation {

    private List<Subscriber> strikes = new ArrayList<>();

    public void add(final Subscriber subscriber) {
        this.strikes.add(subscriber);
    }

    public void callbacks(final List<Integer> rollValues) {
        for (final int r : rollValues) {
            for (final Subscriber strike : this.strikes) {
                strike.run(r)
            }
        }
    }
}

public class FrameBuilder {

    private int count = 0;

    public Frame func(
            final List<Integer> rollValues, 
            final RollPropagation rollPropagation
    ) {
        this.count += 1;

        if (this.lf()) {
            return new Final(rollValues);
        }

        if (this.ifr(rollValues)) {
            return new Incomplete(rollValues[0])
        }

        if (this.sf(rollValues)) {
            final Strike strike = new Strike();
            rollPropagation.add(strike);
            return strike;
        }

        if (this.sf2(rollValues)) {
            final Spare spare = new Spare();
            rollPropagation.add(spare);
            return spare;
        }

        return new NonTen(rollValues[0], rollValues[1])
    }

    private boolean lf() {
        return this.count == Consts.NOF;
    }

    private boolean ifr(final List<Integer> rollValues) {
        return rollValues.length() == 1 && 
            rollValues[0] != Consts.PPF;
    }

    private boolean sf(final List<Integer> rollValues) {
        return rollValues.length() == 1 && 
            rollValues[0] == Consts.PPF;
    }

    private boolean sf2(final List<Integer> rollValues) {
        return rollValues[0] + rollValues[1] == Consts.PPF;
    }
}

public interface Subscriber {
    public void run(final int r);
}

public interface Frame {
    public int score();
}

public class Strike implements Subscriber, Frame {

    private int score = Consts.PPF;
    private int count = 0;

    @Override
    public int score() {
        return this.score;
    }

    @Override
    public void run(final int r) {
        if (this.count < 2) {
            this.score += r;
            this.count += 1;
        }
    }
}


public class Spare implements Subscriber, Frame {

    private int score = Consts.PPF;
    private boolean off = false;

    @Override
    public int score() {
        return this.score;
    }

    @Override
    public void run(final int roll) {
        if (!this.off) {
            this.score += roll;
            this.addedBonus = true;
        }
    }
}


public class Incomplete implements Frame {

    private final int fr;

    public Incomplete(final int fr) {
        this.fr = fr;
    }

    @Override
    public int score() {
        return this.fr;
    }
}

public class NonTen implements Frame {

    private final int fr;
    private final int sr;

    public NonTen(final int fr, final int sr) {
        this.fr = fr;
        this.sr = sr;
    }

    @Override
    public int score() {
        return this.fr + this.sr;
    }
}


public class Final implements Frame {

    private final int score;

    public Final(final List<Integer> frame) {
        this.score = frame.stream().reduce(0, Integer::sum);
    }

    @Override
    public int score() {
        return this.score;
    }
}

The Iterative Improvements

I will work through the classes from the top down of the above code, starting with the

Consts

class. For each, I will identify what it does, the problems with its names, and then present an improved version.

The Consts Class

There is a storied argument about using constants versus configuration for values such as the ones stored in this class, but my mission here isn't to discuss the design but rather the naming. The class looks like this:

public static class Consts {
    public static int PPF = 10;
    public static int NOF = 10;
}

The name of the class,

Consts

, is a short-form. While it seems pretty clear that this class is meant to hold constants, the short-form doesn't save us much (3 letters) so why bother risking ambiguity? This name violates my rule of completeness, and so I will change it to

Constants

Also, the authors are using acronyms for the constant in this class. These acronyms violate both my rules of completeness and expressiveness. Without digging into the rest of the code, how are we to know what these variables mean? I will rename

PPF

PINS_PER_FRAME

and

NOF

NUMBER_OF_FRAMES

to correct this.

public static class Constants {
    public static int PINS_PER_FRAME = 10;
    public static int NUMBER_OF_FRAMES = 10;
}

The BowlingScore Class

This is the main entry point for using the example. A user would use one of these by writing

BowlingScore.build().calcScore(...)

, passing the input into the calcScore function.

The class currently looks like:

public class BowlingScore {

    private final FrameBuilder frameBuilder;
    private final RollPropagation rollPropagation;

    public BowlingScore(
            final FrameBuilder frameBuilder, 
            final RollPropagation rollPropagation) {
        this.frameBuilder = frameBuilder;
        this.rollPropagation = rollPropagation;
    }

    public static BowlingScore build() {
        return new BowlingScore(
            new FrameBuilder(), 
            new RollPropagation())
    }

    public int calcScore(final List<List<Integer>> rollPairList) {
        final List<Frame> values = new ArrayList<>();
        for (final List<Integer> rollPair : rollPairList) {
            this.rollPropagation.add(rollPair);
            values.add(
                this.frameBuilder.func(
                    rollPair, 
                    this.rollPropagation));
        }
        return values.stream().reduce(0, (s, f) -> s + f.score());

    }
}

The name of the class

BowlingScore

does not communicate that this class is an actor. Rather, this class sounds like a data-structure which contains the score for a bowling game. It is surprising to find that this class has responsibilities (like

calcScore

) and reading the usage at the call-site is pretty awkward. From the outside, does the

build

method build the score itself?

What is the separation of concerns between

build

and

calcScore

? To fix this I will rename this class to an actor called

BowlingScorer

. It's a tiny fix, but the call-site should be much clearer now, showing that the

build

method builds a

BowlingScorer

and the

calcScore

then calculates a games score.

The next name that I should address is the method

build

. This name gives us no context on why I should use it over calling the bare constructor. We have to actually read the definition to see that it sets a default

FrameBuilder

and

RollPropagation

object. I can improve the expressiveness of this name by calling it

fromDefaultBuilderAndPropagation

I also need to address the function name

calcScore

which is a short-form for

calculateScore

. This violates my rule of completeness, so I will rename it appropriately.

I should also change the parameter name

rollPairList

. It is likely this name derived from a type that changed. The author probably started by writing this code so that every frame took exactly two elements, allowing them to use a tuple type, which they called a pair. However, once they discovered that the last frame can contain three rolls, they changed the type to a list of lists. Not surprisingly, they forgot to update the name.

This kind of story happens regularly and leads to violations of my be honest rule. The usage of the word

List

in the name is at least honest, but it's a valueless word, and opens us up to the same risk as

Pair

. Instead, I will rename this parameter to be called

rawFrames

. The plurality of the

Frames

indicates that there are multiple of these, and the word

raw

indicates that I haven't done anything with them yet.

The second part of that argument is based on reading ahead and seeing there are data-structures called

Frame

s and not wanting to confuse readers. I will also update derived names, like

rollPair

, to match the new

rawFrames

name.

Another issue I find with this code is the variable named

values

. This name has no meaning, it violates expressiveness, and so I will rename it to

frames

, since that is what is put into this collection.

Finally I should address the single letter variables in the lambda function given as a parameter to

reduce

. Because the scope of usage for these values is so short, single letter variables shouldn't be considered all that bad. However, a single letter variable always creates the potential risk of a later author increasing the scope without renaming things.

Also, if a reader is not comfortable with the reduce method here, the name is not providing them with any help. Let's rename these variables to

currentSum

and

frame

to provide a bit more expressiveness.

The updated code now looks like:

public class BowlingScorer {

    private final FrameBuilder frameBuilder;
    private final RollPropagation rollPropagation;

    public BowlingScorer(
            final FrameBuilder frameBuilder, 
            final RollPropagation rollPropagation
    ) {
        this.frameBuilder = frameBuilder;
        this.rollPropagation = rollPropagation;
    }

    public static BowlingScorer fromDefaultBuilderAndPropagation() {
        return new BowlingScorer(
            new FrameBuilder(), 
            new RollPropagation());
    }

    public int calculateScore(final List<List<Integer>> rawFrames) {
        final List<Frame> frames = new ArrayList<>();
        for (final List<Integer> rawFrame : rawFrames) {
            this.rollPropagation.add(rawFrame);
            frames.add(
                this.frameBuilder.func(
                    rawFrame, 
                    this.rollPropagation));
        }
        return frames.stream().reduce(
            0, 
            (currentSum, frame) -> currentSum + frame.score());
    }
}

There are still problems with naming in this class, like the

func

that is called on the

frameBuilder

but I will address that when we get to that class. You can assume I will propagate such changes back to these objects (and you will see the effects of that in the final form).

The RollPropagation Class

The

RollPropagation

class allows the passing of values from new rolls back to frames that have already occurred. This is valuable for strikes and spares where the score for the frame is based not only on the rolls of those frames but also of the following frames. It currently looks like this:

public class RollPropagation {

    private List<Subscriber> strikes = new ArrayList<>();

    public void add(final Subscriber subscriber) {
        this.strikes.add(subscriber);
    }

    public void callbacks(final List<Integer> rollValues) {
        for (final int r : rollValues) {
            for (final Subscriber strike : this.strikes) {
                strike.run(r)
            }
        }
    }
}

The very first problem with this is the class name itself is not an actor, but reads as if this is some kind of data-structure. To fix this I will rename the class to match what it does, which is broadcast rolls to the frames which are subscribed to it. I could call this

RollBroadcaster

which communicates the functionality, but I realize this is following a known pattern called the Observer Pattern.

If a reader if familiar with this pattern, having a name which indicates its use, will be valuable to them. In the Observer Pattern there are two parts, the Subject and the Observers. In this case, this class is the Subject (the thing which the observers will observe; the rolls).

I will rename this class

RollSubjectBroadcaster

. It is important to note, as authors, we may not be aware of every pattern, or their proper naming conventions. We cannot be expected to know what we do not know. If I had not caught that this was the Observer Pattern, naming this class

RollBroadcaster

would still be an improvement. However, it would have cost an unfamiliar reader a few extra seconds to catch on to what I was doing.

The next problem is with the

strikes

variable. I know this name is a lie because strikes are not the only type of frame that needs to know about rolls after the fact (What about spares?). It is my guess that this class was originally written with two lists, one for strikes and the other for spares.

The author then rightly made the change to have a common interface between those two, reducing the list to its current, singular form.

The problem is they never adjusted the name to match their changes.

To correct this, I will rename this variable to

rollObservers

which accurately describes what the list is intended to hold.

The function name

add

is commonly used. For example, on Java's Collections interface they use the name to indicate a way to add new members to the collection.

The problem with

add

is that it can also mean "to sum together" which is certainly not being done here. Usually, better names would be

append

push

insert

, etc., as they are more expressive. In this case though, since the authors are not building a generic collection, they could have (and therefore should have) been even more expressive. I will rename this function to

subscribe

, as call site users should not care how this class stores them.

The final function,

callbacks

is named after things, not actions, so this is a bad name. Even if I called it

callback

, which can be an action, it still would not be very expressive. Instead I will call this

broadcastRolls

which clearly states exactly what this function will do.

The parameter to

callbacks

is called

rollValues

. The word

Values

here is meaningless, so I will remove it (to follow my rule of being concise). I will rename this to

rawFrame

which indicates that this is the rolls from a single frame.

The short-form variable

is safe because it is being used in a very small scope. However, it's minimal work to add expressiveness so I will rename it to

roll

. Also, scopes have a risk of growing in the future. We can future-proof our names by avoiding the small scope argument as an excuse to be lazy.

With these changes, the new class looks like:

public class RollSubjectBroadcaster {

    private List<Subscriber> rollObservers = new ArrayList<>();

    public void subscribe(final Subscriber subscriber) {
        this.rollObservers.add(subscriber);
    }

    public void broadcastRolls(final List<Integer> rawFrame) {
        for (final int roll : rawFrame) {
            for (final Subscriber rollObservers : 
                    this.rollObservers) {
                rollObservers.run(roll)
            }
        }
    }
}

The FrameBuilder Class

The

FrameBuilder

class takes the rolls from a single raw frame and builds a new frame object of the correct type. The

FrameBuilder

class also subscribes the frame to future rolls if its required by its type for scoring. Its current untouched form looks like:

public class FrameBuilder {

    private int count = 0;

    public Frame func(
            final List<Integer> rollValues, 
            final RollPropagation rollPropagation
    ) {
        this.count += 1;

        if (this.lf()) {
            return new Final(rollValues);
        }

        if (this.ifr(rollValues)) {
            return new Incomplete(rollValues[0])
        }

        if (this.sf(rollValues)) {
            final Strike strike = new Strike();
            rollPropagation.subscribe(strike);
            return strike;
        }

        if (this.sf2(rollValues)) {
            final Spare spare = new Spare();
            rollPropagation.subscribe(spare);
            return spare;
        }

        return new NonTen(rollValues[0], rollValues[1])
    }

    private boolean lf() {
        return this.count == Constants.NUMBER_OF_FRAMES;
    }

    private boolean ifr(final List<Integer> rollValues) {
        return rollValues.length() == 1 && 
            rollValues[0] != Constants.PINS_PER_FRAME;
    }

    private boolean sf(final List<Integer> rollValues) {
        return rollValues.length() == 1 && 
            rollValues[0] == Constants.PINS_PER_FRAME;
    }

    private boolean sf2(final List<Integer> rollValues) {
        return rollValues[0] + rollValues[1] == 
            Constatns.PINS_PER_FRAME;
    }
}

The first problem with this class is that its name is a lie. In software engineering we use the word pattern to describe certain solution designs to common problems. It is possible the author of this class did not know that

Builder

is a type of pattern, but it does exist. We cannot expect to know what we don't know, but this leaves readers feeling deceived.

I recommend that everyone learn the various patterns at some point in their career. At the very least, start by learning the names of the patterns out there. Then, if you are not using the pattern (or aren't sure), avoid using the patterns name. Patterns are designed as communication tools. If someone names something

FrameBuilder

, as a reader, I have a pretty good idea about how that object can be used and what it will do. In this case the author is actually using a pattern called the Factory pattern, and so I will rename this class to

FrameFactory

The next issue is with the private member

count

. The word

count

is often used in programming but is rarely expressive of what is being counted. As this is counting the number of frames the factory has built thus far, I will rename this member

builtFrames

The function

func

is terribly named. We saw its usage when we were fixing the

BowlingScorer

and it caused us issues there. Unfortunately, I have seen this many times in production code bases around the industry, and to the best of my ability can only attribute it to pure laziness. I will rename this method to

construct

For the parameter

rollValues

passed to

func

the word

Values

is meaningless, and therefore a violation of my conciseness rule. Instead, I will call this

rawFrame

to indicate that I am using a single frame worth of rolls to construct the new

Frame

object.

The

FrameBuilder

class contains a series of boolean functions that encapsulate the logic of the various conditionals used to build the different frame types. These functions were lazily named for brevities sake as

lf

ifr

sf

, and

sf2

. These names can neither be considered complete nor expressive.

I assume the author wrote the first function,

lf

, to represent the logic to identify the last frame, and in doing so, set the precedent for the rest of these functions. I will rename

lf

isLastFrame

. Using the prefix

is

is a common way to denote a method which returns a boolean. This also allows my name to be an action instead of a thing (as

lastFrame

would be). There is an interesting conversation about Command actions versus Query actions (

isLastFrame

is the latter) but I will cover that in a later article.

The next conditional function,

ifr

, checks if a frame is incomplete. The author probably wanted to call this function

if

but realized this would conflict with the reserved word of the same name, so they added the

postfix. Another issue with this naming scheme is the similarity between letters

and

. The first two functions could easily be mistyped by a programmer, causing a major bug that would be hard to see. I will rename this method to

isIncompleteFrame

The third function

sf

is used for a strike frame. Again this is lazy short-forming, and I will rename it to

isStrikeFrame

The fourth function shows another issue with this style of naming. The author wants a function to check if the frame is a spare frame, but the name

sf

is already taken by the strike frame function. Short names are vulnerable to these types of collisions. The author's solution is to postfix this name with the number 2. It's saddening how clearly ridiculous this seems, and yet how often I find this exact thing in peoples code. To fix this I will rename this method

isSpareFrame

The improved code now looks like:

public class FrameFactory {

    private int builtFrames = 0;

    public Frame construct(
            final List<Integer> rawFrame, 
            final RollBroadcaster rollBroadcaster
    ) {
        this.builtFrames += 1;

        if (this.isLastFrame()) {
            return new Final(rawFrame);
        }

        if (this.isIncompleteFrame(rawFrame)) {
            return new Incomplete(rawFrame[0])
        }

        if (this.isStrikeFrame(rawFrame)) {
            final Strike strike = new Strike();
            rollBroadcaster.subscribe(strike);
            return strike;
        }

        if (this.isSpareFrame(rawFrame)) {
            final Spare spare = new Spare();
            rollBroadcaster.subscribe(spare);
            return spare;
        }

        return new NonTen(rawFrame[0], rawFrame[1])
    }

    private boolean isLastFrame() {
        return this.builtFrames == Constants.NUMBER_OF_FRAMES;
    }

    private boolean isIncompleteFrame(final List<Integer> rawFrame) {
        return rawFrame.length() == 1 && 
            rawFrame[0] != Constants.PINS_PER_FRAME;
    }

    private boolean isStrikeFrame(final List<Integer> rawFrame) {
        return rawFrame.length() == 1 && 
            rawFrame[0] == Constants.PINS_PER_FRAME;
    }

    private boolean isSpareFrame(final List<Integer> rawFrame) {
        return rawFrame[0] + rawFrame[1] == Constants.PINS_PER_FRAME;
    }
}

The Subscriber Interface

The next piece of code for analysis is the

Subscriber

interface. Interfaces allow us to create contracts on a classes usage. The

Subscriber

interface declares a method called

run

which takes an integer and returns nothing (void). Any class which wishes to be a

Subscriber

must implement this

run

method for itself. You can see the advantage of this in the

RollSubjectBroadcaster

class above; strikes and spares can be grouped together as objects that can have rolls broadcasted back to them. The current interface looks like:

public interface Subscriber {
    public void run(final int r);
}

The name of this interface isn't terrible, except it sounds too generic to be used for such a specialized purpose. I can easily increase the expressiveness of this name by calling it

RollSubscriberFrame

. However, as discussed above, this is part of the Observer Pattern and so I would like to indicate that. Therefore, I will use the word

Observer

instead of the word

Subscriber.

Note that my new name,

RollObserverFrame

, isn't that of an actor. Investigating the usage of this interface finds that it is actually being used for data-structure objects instead of actors (as

Subscriber

indicates) making this name change appropriate.

The function

run

is commonly used but generally terrible. Within the context of this interface, it's easy to understand that this is the only thing a

RollObserverFrame

has to do, but when used in actual implementation it wont make much sense. I will rename this function to be

rollCallback

to denote that this is the action to be taken when a roll is to be handled by the implementation of this function.

Often, authors will use single letter parameters in their interfaces because the name only actually matters in the implementation (and they don't have to match). However, doing so provides no help to a reader of the interface. A little extra effort here goes a long ways. To be expressive, I will rename the parameter

roll

The improved code looks like:

public interface RollObserverFrame {
    public void rollCallback(final int roll);
}

The Frame Interface

The

Frame

interface defines a contract that anything wishing to be used as a

Frame

should implement a

score

method returning an integer. It is being implemented in the

BowlingScorer

, Which calculates the games total score. It does this by summing all the individual scores of the frames.

public interface Frame {
    public int score();
}

The only naming problem here is that the method

score

is not an action but could denote one. Instead, I will rename this to

getScore

which clearly defines the word score as a value and not an action.

The improved code looks like:

public interface Frame {
    public int getScore();
}

The Strike Class

The

Strike

class implements both interfaces. It implements the

RollObserverFrame

because it needs to update the score with the next two rolls from following frames. It also implements the

Frame

interface, so that a score can be extracted from it.

The current version looks like:

public class Strike implements RollObserverFrame, Frame {

    private int score = Constants.PINS_PER_FRAME;
    private int count = 0;

    @Override
    public int getScore() {
        return this.score;
    }

    @Override
    public void rollCallback(final int r) {
        if (this.count < 2) {
            this.score += r;
            this.count += 1;
        }
    }
}

My first comment is on the private member

count

. What is this value counting? A little investigation shows it increases with each call to

rollCallback

until it reaches 2. I am going to rename this to

addedBonusCount

to be more expressive.

I also notice that the parameter

is being passed into the

rollCallback

function. This is probably named as a single letter because the interface had it named that way too. This is a simple example of how lazy naming can be infectious. I will rename this to

roll

for clarification.

The improved

Strike

class looks like:

public class Strike implements RollObserverFrame, Frame {

    private int score = Constants.PINS_PER_FRAME;
    private int addedBonusCount = 0;

    @Override
    public int getScore() {
        return this.score;
    }

    @Override
    public void rollCallback(final int roll) {
        if (this.addedBonusCount < 2) {
            this.score += roll;
            this.addedBonusCount += 1;
        }
    }
}

The Spare Class

Similar to the

Strike

class, the

Spare

class implements both the

RollObserverFrame

and

Frame

interfaces. In this class only the very next rolls value should be added to the score.

Before improving the naming it looks like:

public class Spare implements RollObserverFrame, Frame {

    private int score = Constants.PINS_PER_FRAME;
    private boolean off = false;

    @Override
    public int getScore() {
        return this.score;
    }

    @Override
    public void rollCallback(final int roll) {
        if (!this.off) {
            this.score += roll;
            this.addedBonus = true;
        }
    }
}

My complaint here is to do with the private member

off

which is not expressive. It could mean "not on", or perhaps, it is a short-form that I don't understand. If it does mean "not on" what is it that isn't on? I am renaming this to

addedBonus

to indicate whether I have yet added a bonus.

The improved version looks like:

public class Spare implements RollObserverFrame, Frame {

    private int score = Constants.PINS_PER_FRAME;
    private boolean addedBonus = false;

    @Override
    public int getScore() {
        return this.score;
    }

    @Override
    public void rollCallback(final int roll) {
        if (!this.addedBonus) {
            this.score += roll;
            this.addedBonus = true;
        }
    }
}

The Incomplete Class

The

Incomplete

class represents a frame where only one roll was made so far. It implements the

Frame

interface like the other frame objects. However, it does not care about extra rolls, so it does not implement the

RollObserverFrame

interface.

public class Incomplete implements Frame {

    private final int fr;

    public Incomplete(final int fr) {
        this.fr = fr;
    }

    @Override
    public int getScore() {
        return this.fr;
    }
}

When the author named the

Spare

and

Strike

frames, I didn't mind the names because those are fairly specific bowling terms.

Incomplete

, however, is a difficult name because it doesn't specifically have to be about a bowling frame. For this reason, I will rename this class to

IncompleteFrame

. This creates an inconsistency with the other frame names, so I will similarly postfix them with the word

Frame

(

Strike

becomes

StrikeFrame

, etc.).

Another problem in this class is the private member

fr

. This is a short-form (violating the rule on completeness) for

firstRoll

so I will rename the value appropriately.

The updated

Incomplete

class is now:

public class IncompleteFrame implements Frame {

    private final int firstRoll;

    public IncompleteFrame(final int firstRoll) {
        this.firstRoll = firstRoll;
    }

    @Override
    public int getScore() {
        return this.firstRoll;
    }
}

The NonTenFrame Class

The

NonTenFrame

class represents a frame which is complete, but neither a strike nor a spare.

public class NonTenFrame implements Frame {

    private final int fr;
    private final int sr;

    public NonTen(final int fr, final int sr) {
        this.fr = fr;
        this.sr = sr;
    }

    @Override
    public int getScore() {
        return this.fr + this.sr;
    }
}

The bad naming here is similar to the

IncompleteFrame

class, so I apply the same solution; renaming

fr

firstRoll

and

sr

secondRoll

The improved version of the

NonTenFrame

looks like:

public class NonTenFrame implements Frame {

    private final int firstRoll;
    private final int secondRoll;

    public NonTen(final int firstRoll, final int secondRoll) {
        this.firstRoll = firstRoll;
        this.secondRoll = secondRoll;
    }

    @Override
    public int getScore() {
        return this.firstRoll + this.secondRoll;
    }
}

The Final Class

The last class is the

FinalFrame

which represents the tenth frame of a bowling game.

public class FinalFrame implements Frame {

    private final int score;

    public Final(final List<Integer> frame) {
        this.score = frame.stream().reduce(0, Integer::sum);
    }

    @Override
    public int getScore() {
        return this.score;
    }
}

The names in this class are already acceptable, but that is only true because I renamed it to be postfixed with

Frame

. Before, this class would have been (and was) named

Final

. The word

final

is a reserved word in Java, with very specific meaning. Creating a

Final

class muddies the waters with the reserved meaning. Renaming this to

FinalFrame

is a big improvement.

The Improved Code

I have now gone through and cleaned up the naming of the bowling scorer. I didn't touch the design or any other aspect, but I believe I have made large strides in improving the quality of this code. Below is the improved version in its entirety. Compare it to the original code in this chapter to see the improvement these naming rules can make.

public static class Constants {
    public static int PINS_PER_FRAME = 10;
    public static int NUMBER_OF_FRAMES = 10;
}

public class BowlingScorer {

    private final FrameFactory frameFactory;
    private final RollSubjectBroadcaster rollBroadcaster;

    public BowlingScorer(
        final FrameFactory frameFactory, 
        final RollSubjectBroadcaster rollBroadcaster
    ) {
        this.frameFactory = frameFactory;
        this.rollBroadcaster = rollBroadcaster;
    }

    public static BowlingScorer fromDefaultFactoryAndBroadcaster() {
        return new BowlingScorer(
            new FrameFactory(), 
            new RollSubjectBroadcaster());
    }

    public int calculateScore(final List<List<Integer>> rawFrames) {
        final List<Frame> frames = new ArrayList<>();
        for (final List<Integer> rawFrame : rawFrames) {
            this.rollBroadcaster.broadcastRolls(rawFrame);
            frames.add(
                this.frameFactory.construct(
                    rawFrame, 
                    this.rollBroadcaster));
        }
        return frames.stream().reduce(
            0, 
            (currentSum, frame) -> currentSum + frame.getScore());
    }
}

public class RollSubjectBroadcaster {

    private List<RollObserverFrame> rollObservers = 
        new ArrayList<>();

    public void subscribe(final RollObserverFrame observer) {
        this.rollObservers.add(observer);
    }

    public void broadcastRolls(final List<Integer> rawFrame) {
        for (final int roll : rawFrame) {
            for (final RollObserverFrame observer : 
                    this.rollObservers) {
                observer.rollCallback(roll)
            }
        }
    }
}

public class FrameFactory {

    private int builtFrames = 0;

    public Frame construct(
            final List<Integer> rawFrame, 
            final RollSubjectBroadcaster rollBroadcaster
    ) {
        this.builtFrames += 1;

        if (this.isLastFrame()) {
            return new FinalFrame(rawFrame);
        }

        if (this.isIncompleteFrame(rawFrame)) {
            return new IncompleteFrame(rawFrame[0])
        }

        if (this.isStrikeFrame(rawFrame)) {
            final StrikeFrame strikeFrame = new StrikeFrame();
            rollBroadcaster.subscribe(strikeFrame);
            return strikeFrame;
        }

        if (this.isSpareFrame(rawFrame)) {
            final SpareFrame spareFrame = new SpareFrame();
            rollBroadcaster.subscribe(spareFrame);
            return spareFrame;
        }

        return new NonTenFrame(rawFrame[0], rawFrame[1])
    }

    private boolean isLastFrame() {
        return this.builtFrames == Constants.NUMBER_OF_FRAMES;
    }

    private boolean isIncompleteFrame(final List<Integer> rawFrame) {
        return rawFrame.length() == 1 && 
            rawFrame[0] != Constants.PINS_PER_FRAME;
    }

    private boolean isStrikeFrame(final List<Integer> rawFrame) {
        return rawFrame.length() == 1 && 
            rawFrame[0] == Constants.PINS_PER_FRAME;
    }

    private boolean isSpareFrame(final List<Integer> rawFrame) {
        return rawFrame[0] + rawFrame[1] == Constants.PINS_PER_FRAME;
    }
}

public interface RollObserverFrame {
    public void rollCallback(final int roll);
}

public interface Frame {
    public int getScore();
}

public class StrikeFrame implements RollObserverFrame, Frame {

    private int score = Constants.PINS_PER_FRAME;
    private int addedBonusCount = 0;

    @Override
    public int getScore() {
        return this.score;
    }

    @Override
    public void rollCallback(final int roll) {
        if (this.addedBonusCount < 2) {
            this.score += roll;
            this.addedBonusCount += 1;
        }
    }
}

public class SpareFrame implements RollObserverFrame, Frame {

    private int score = Constants.PINS_PER_FRAME;
    private boolean addedBonus = false;

    @Override
    public int getScore() {
        return this.score;
    }

    @Override
    public void rollCallback(final int roll) {
        if (!this.addedBonus) {
            this.score += roll;
            this.addedBonus = true;
        }
    }
}

public class IncompleteFrame implements Frame {

    private final int firstRoll;

    public IncompleteFrame(final int firstRoll) {
        this.firstRoll = firstRoll;
    }

    @Override
    public int getScore() {
        return this.firstRoll;
    }
}

public class NonTenFrame implements Frame {

    private final int firstRoll;
    private final int secondRoll;

    public NonTenFrame(final int firstRoll, final int secondRoll) {
        this.firstRoll = firstRoll;
        this.secondRoll = secondRoll;
    }

    @Override
    public int getScore() {
        return this.firstRoll + this.secondRoll;
    }
}

public class FinalFrame implements Frame {

    private final int score;

    public FinalFrame(final List<Integer> frame) {
        this.score = frame.stream().reduce(0, Integer::sum);
    }

    @Override
    public int getScore() {
        return this.score;
    }
}

I hope you agree this new version is an improvement. Writing good names can be difficult, but as we have seen it can make a big difference. By following the principles I outlined in this chapter, naming should also be a bit easier.

The Conclusion

When writing names in code, it is best to make them Concise, Honest, Expressive, and Complete. Even a reasonably well designed solution can be difficult to understand if good naming practices are not followed. This practice is challenging but empowers readers to understand our code with ease.