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

Whenever you name a variable, function, or class, ask if it is , , , and . The Rule: concise honest expressive 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: They should not be wasteful. This means we don't add on meaningless words or unnecessary context. Concise: They should not lie. This means a name should not hide, or misinform on any part of what it represents. Honest: They should express intent. This means a name should accurately and clearly describe what it represents. Expressive: The shouldn't use short forms or acronyms. This means we don't expect our readers to know the same encodings that we do. Complete: 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 variable name might look like: terrible List valueList = ArrayList<>(); final new Here the author used the word , which is meaningless (breaking Rule 1, be Concise) and the word 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: value List Set valueList = HashSet<>(); final new Because someone decided using a was better than a , 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. set list A better name would look like: List accountIdsToLookup = ArrayList<>(); final new This name indicates that there are multiple values through the plurality of but also does not tie us to an underlying data structure. I recommend this for all type objects. Furthermore, this name is expressive, as it indicates what the variable contains and what it should be used for. accountIds iterable For key-value data structures like maps (dictionaries, etc.) I recommend naming schemes like or . keysToValues valuesByKeys Map accountsMap = HashMap<>(); Map accountsById = HashMap<>(); final new // bad final new // 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 ( ) not a thing ( ). Functions execute change, naming them after things is dishonest. Furthermore, functions are the driving steps of the or your program, so when we read each step, we want it to read as if something is changing. Actions denote change. a Verb a Noun story A bad function name can be seen in this class: { String name; taxId; { .name = name; .taxId = taxId; } { StringBuilder() .append( ) .append( + .name) .append( ) .append( + .taxId) .append( ) .build(); } } public class Company private final private final int public Company ( String name, taxId) final final int this this String public json () return new "{" "'name': " this "," "'taxId': " this "}" The function 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 which indicates that the function is taking some action on the class of which it is a member. json to_json Company As a side note, the authors are using a here to convert this class to JSON (Javascript Object Notation). StringBuilder 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 or . FasterXML's Jackson Google's Gson Class Names There are two types of classes, and (I'll discuss this further in a later article about not building hybrids), and each should be named differently. data structures actors 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 ( Session session) final 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 it will read poorly like: validateSession Authentication authentication = Authentication(...); ... (authentication.validateSession(session)) { ... } final new if This would read significantly less awkward if I swapped out the class name to : Authenticator Authenticator authenticator = Authenticator(...); ... (authenticator.validateSession(session)) { ... } final new if By its name, it is clear that the is an actor, where having responsibilities makes sense. Authenticator 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 , , and in Java. HashMap TreeSet LinkedList Specific data structure objects should be named after the object they encapsulate, and not Actors. A badly named specific data structure might look like: { ... { .sessionKey; } } public class Authenticator String public getSessionKey () return this From the name, this class appears to be an actor, which means a call to makes us think this function is doing something (like accessing a database). Instead, a better class name would be so that the call-site reads nicely as: getSessionKey Authentication Authentication authentication = Authentication(...); ... String sessionKey = authentication.getSessionKey(); final new final Note how it's much more apparent this function is only accessing the property of the data structure. sessionKey authentication 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 { PPF = ; NOF = ; } { FrameBuilder frameBuilder; RollPropagation rollPropagation; { .frameBuilder = frameBuilder; .rollPropagation = rollPropagation; } { BowlingScore( FrameBuilder(), RollPropagation()); } { List values = ArrayList<>(); ( List rollPair : rollPairList) { .rollPropagation.add(rollPair); values.add( .frameBuilder.func( rollPair, .rollPropagation) ); } values.stream().reduce( , (s, f) -> s + f.score()); } } { List strikes = ArrayList<>(); { .strikes.add(subscriber); } { ( r : rollValues) { ( Subscriber strike : .strikes) { strike.run(r) } } } } { count = ; { .count += ; ( .lf()) { Final(rollValues); } ( .ifr(rollValues)) { Incomplete(rollValues[ ]) } ( .sf(rollValues)) { Strike strike = Strike(); rollPropagation.add(strike); strike; } ( .sf2(rollValues)) { Spare spare = Spare(); rollPropagation.add(spare); spare; } NonTen(rollValues[ ], rollValues[ ]) } { .count == Consts.NOF; } { rollValues.length() == && rollValues[ ] != Consts.PPF; } { rollValues.length() == && rollValues[ ] == Consts.PPF; } { rollValues[ ] + rollValues[ ] == Consts.PPF; } } { ; } { ; } { score = Consts.PPF; count = ; { .score; } { ( .count < ) { .score += r; .count += ; } } } { score = Consts.PPF; off = ; { .score; } { (! .off) { .score += roll; .addedBonus = ; } } } { fr; { .fr = fr; } { .fr; } } { fr; sr; { .fr = fr; .sr = sr; } { .fr + .sr; } } { score; { .score = frame.stream().reduce( , Integer::sum); } { .score; } } public static class Consts public static int 10 public static int 10 public class BowlingScore private final private final public BowlingScore ( FrameBuilder frameBuilder, RollPropagation rollPropagation ) final final this this BowlingScore public static build () return new new new public int calcScore ( List > rollPairList) final final new for final this this this return 0 public class RollPropagation private new public void add ( Subscriber subscriber) final this public void callbacks ( List rollValues) final for final int for final this public class FrameBuilder private int 0 Frame public func ( List rollValues, RollPropagation rollPropagation ) final final this 1 if this return new if this return new 0 if this final new return if this final new return return new 0 1 private boolean lf () return this private boolean ifr ( List rollValues) final return 1 0 private boolean sf ( List rollValues) final return 1 0 private boolean sf2 ( List rollValues) final return 0 1 public interface Subscriber public void run ( r) final int public interface Frame public int score () public , class Strike implements Subscriber Frame private int private int 0 @Override public int score () return this @Override public void run ( r) final int if this 2 this this 1 public , class Spare implements Subscriber Frame private int private boolean false @Override public int score () return this @Override public void run ( roll) final int if this this this true public class Incomplete implements Frame private final int public Incomplete ( fr) final int this @Override public int score () return this public class NonTen implements Frame private final int private final int public NonTen ( fr, sr) final int final int this this @Override public int score () return this this public class Final implements Frame private final int public Final ( List frame) final this 0 @Override public int score () return this The Iterative Improvements I will work through the classes from the top down of the above code, starting with the class. For each, I will identify what it does, the problems with its names, and then present an improved version. Consts 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: { PPF = ; NOF = ; } public static class Consts public static int 10 public static int 10 The name of the class, , 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 . Consts 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 to and to to correct this. PPF PINS_PER_FRAME NOF NUMBER_OF_FRAMES { PINS_PER_FRAME = ; NUMBER_OF_FRAMES = ; } public static class Constants public static int 10 public static int 10 The BowlingScore Class This is the main entry point for using the example. A user would use one of these by writing , passing the input into the calcScore function. BowlingScore.build().calcScore(...) The class currently looks like: { FrameBuilder frameBuilder; RollPropagation rollPropagation; { .frameBuilder = frameBuilder; .rollPropagation = rollPropagation; } { BowlingScore( FrameBuilder(), RollPropagation()) } { List values = ArrayList<>(); ( List rollPair : rollPairList) { .rollPropagation.add(rollPair); values.add( .frameBuilder.func( rollPair, .rollPropagation)); } values.stream().reduce( , (s, f) -> s + f.score()); } } public class BowlingScore private final private final public BowlingScore ( FrameBuilder frameBuilder, RollPropagation rollPropagation) final final this this BowlingScore public static build () return new new new public int calcScore ( List > rollPairList) final final new for final this this this return 0 The name of the class 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 ) and reading the usage at the call-site is pretty awkward. From the outside, does the method build the score itself? BowlingScore calcScore build What is the separation of concerns between and ? To fix this I will rename this class to an actor called . It's a tiny fix, but the call-site should be much clearer now, showing that the method builds a and the then calculates a games score. build calcScore BowlingScorer build BowlingScorer calcScore The next name that I should address is the method . 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 and object. I can improve the expressiveness of this name by calling it . build FrameBuilder RollPropagation fromDefaultBuilderAndPropagation I also need to address the function name which is a short-form for . This violates my rule of completeness, so I will rename it appropriately. calcScore calculateScore I should also change the parameter name . 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. rollPairList This kind of story happens regularly and leads to violations of my be honest rule. The usage of the word in the name is at least honest, but it's a valueless word, and opens us up to the same risk as . Instead, I will rename this parameter to be called . The plurality of the indicates that there are multiple of these, and the word indicates that I haven't done anything with them yet. List Pair rawFrames Frames raw The second part of that argument is based on reading ahead and seeing there are data-structures called s and not wanting to confuse readers. I will also update derived names, like , to match the new name. Frame rollPair rawFrames Another issue I find with this code is the variable named . This name has no meaning, it violates expressiveness, and so I will rename it to , since that is what is put into this collection. values frames Finally I should address the single letter variables in the lambda function given as a parameter to . 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. reduce 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 and to provide a bit more expressiveness. currentSum frame The updated code now looks like: { FrameBuilder frameBuilder; RollPropagation rollPropagation; { .frameBuilder = frameBuilder; .rollPropagation = rollPropagation; } { BowlingScorer( FrameBuilder(), RollPropagation()); } { List frames = ArrayList<>(); ( List rawFrame : rawFrames) { .rollPropagation.add(rawFrame); frames.add( .frameBuilder.func( rawFrame, .rollPropagation)); } frames.stream().reduce( , (currentSum, frame) -> currentSum + frame.score()); } } public class BowlingScorer private final private final public BowlingScorer ( FrameBuilder frameBuilder, RollPropagation rollPropagation ) final final this this BowlingScorer public static fromDefaultBuilderAndPropagation () return new new new public int calculateScore ( List > rawFrames) final final new for final this this this return 0 There are still problems with naming in this class, like the that is called on the 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). func frameBuilder The RollPropagation Class The 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: RollPropagation { List strikes = ArrayList<>(); { .strikes.add(subscriber); } { ( r : rollValues) { ( Subscriber strike : .strikes) { strike.run(r) } } } } public class RollPropagation private new public void add ( Subscriber subscriber) final this public void callbacks ( List rollValues) final for final int for final this 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 which communicates the functionality, but I realize this is following a known called the . RollBroadcaster pattern 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 and the . In this case, this class is the Subject (the thing which the observers will observe; the rolls). Subject Observers I will rename this class . 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 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. RollSubjectBroadcaster RollBroadcaster The next problem is with the 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. strikes 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 which accurately describes what the list is intended to hold. rollObservers The function name 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. add The problem with is that it can also mean "to sum together" which is certainly not being done here. Usually, better names would be , , , 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 , as call site users should not care how this class stores them. add append push insert subscribe The final function, is named after things, not actions, so this is a bad name. Even if I called it , which can be an action, it still would not be very expressive. Instead I will call this which clearly states exactly what this function will do. callbacks callback broadcastRolls The parameter to is called . The word here is meaningless, so I will remove it (to follow my rule of being concise). I will rename this to which indicates that this is the rolls from a single frame. callbacks rollValues Values rawFrame 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 . Also, scopes have a risk of growing in the future. r roll 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: { List rollObservers = ArrayList<>(); { .rollObservers.add(subscriber); } { ( roll : rawFrame) { ( Subscriber rollObservers : .rollObservers) { rollObservers.run(roll) } } } } public class RollSubjectBroadcaster private new public void subscribe ( Subscriber subscriber) final this public void broadcastRolls ( List rawFrame) final for final int for final this The FrameBuilder Class The class takes the rolls from a single raw frame and builds a new frame object of the correct type. The class also subscribes the frame to future rolls if its required by its type for scoring. Its current untouched form looks like: FrameBuilder FrameBuilder { count = ; { .count += ; ( .lf()) { Final(rollValues); } ( .ifr(rollValues)) { Incomplete(rollValues[ ]) } ( .sf(rollValues)) { Strike strike = Strike(); rollPropagation.subscribe(strike); strike; } ( .sf2(rollValues)) { Spare spare = Spare(); rollPropagation.subscribe(spare); spare; } NonTen(rollValues[ ], rollValues[ ]) } { .count == Constants.NUMBER_OF_FRAMES; } { rollValues.length() == && rollValues[ ] != Constants.PINS_PER_FRAME; } { rollValues.length() == && rollValues[ ] == Constants.PINS_PER_FRAME; } { rollValues[ ] + rollValues[ ] == Constatns.PINS_PER_FRAME; } } public class FrameBuilder private int 0 Frame public func ( List rollValues, RollPropagation rollPropagation ) final final this 1 if this return new if this return new 0 if this final new return if this final new return return new 0 1 private boolean lf () return this private boolean ifr ( List rollValues) final return 1 0 private boolean sf ( List rollValues) final return 1 0 private boolean sf2 ( List rollValues) final return 0 1 The first problem with this class is that its name is a lie. In software engineering we use the word to describe certain solution designs to common problems. It is possible the author of this class did not know that 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. pattern Builder 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 , 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 pattern, and so I will rename this class to . FrameBuilder Factory FrameFactory The next issue is with the private member . The word 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 . count count builtFrames The function is terribly named. We saw its usage when we were fixing the 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 . func BowlingScorer construct For the parameter passed to the word is meaningless, and therefore a violation of my conciseness rule. Instead, I will call this to indicate that I am using a single frame worth of rolls to construct the new object. rollValues func Values rawFrame Frame The 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 , , , and . These names can neither be considered complete nor expressive. FrameBuilder lf ifr sf sf2 I assume the author wrote the first function, , 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 to . Using the prefix 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 would be). There is an interesting conversation about actions versus actions ( is the latter) but I will cover that in a later article. lf lf isLastFrame is lastFrame Command Query isLastFrame The next conditional function, , checks if a frame is incomplete. The author probably wanted to call this function 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 . ifr if r i l isIncompleteFrame The third function is used for a strike frame. Again this is lazy short-forming, and I will rename it to . sf 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 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 . sf isSpareFrame The improved code now looks like: { builtFrames = ; { .builtFrames += ; ( .isLastFrame()) { Final(rawFrame); } ( .isIncompleteFrame(rawFrame)) { Incomplete(rawFrame[ ]) } ( .isStrikeFrame(rawFrame)) { Strike strike = Strike(); rollBroadcaster.subscribe(strike); strike; } ( .isSpareFrame(rawFrame)) { Spare spare = Spare(); rollBroadcaster.subscribe(spare); spare; } NonTen(rawFrame[ ], rawFrame[ ]) } { .builtFrames == Constants.NUMBER_OF_FRAMES; } { rawFrame.length() == && rawFrame[ ] != Constants.PINS_PER_FRAME; } { rawFrame.length() == && rawFrame[ ] == Constants.PINS_PER_FRAME; } { rawFrame[ ] + rawFrame[ ] == Constants.PINS_PER_FRAME; } } public class FrameFactory private int 0 Frame public construct ( List rawFrame, RollBroadcaster rollBroadcaster ) final final this 1 if this return new if this return new 0 if this final new return if this final new return return new 0 1 private boolean isLastFrame () return this private boolean isIncompleteFrame ( List rawFrame) final return 1 0 private boolean isStrikeFrame ( List rawFrame) final return 1 0 private boolean isSpareFrame ( List rawFrame) final return 0 1 The Subscriber Interface The next piece of code for analysis is the interface. Interfaces allow us to create contracts on a classes usage. The interface declares a method called which takes an integer and returns nothing (void). Any class which wishes to be a must implement this method for itself. You can see the advantage of this in the class above; strikes and spares can be grouped together as objects that can have rolls broadcasted back to them. The current interface looks like: Subscriber Subscriber run Subscriber run RollSubjectBroadcaster { ; } public interface Subscriber public void run ( r) final int 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 . However, as discussed above, this is part of the and so I would like to indicate that. Therefore, I will use the word instead of the word RollSubscriberFrame Observer Pattern Observer Subscriber. Note that my new name, , 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 indicates) making this name change appropriate. RollObserverFrame Subscriber The function is commonly used but generally terrible. Within the context of this interface, it's easy to understand that this is the only thing a has to do, but when used in actual implementation it wont make much sense. I will rename this function to be to denote that this is the action to be taken when a roll is to be handled by the implementation of this function. run RollObserverFrame rollCallback 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 to . r roll The improved code looks like: public interface RollObserverFrame { public rollCallback(final int roll); } void The Frame Interface The interface defines a contract that anything wishing to be used as a should implement a method returning an integer. It is being implemented in the , Which calculates the games total score. It does this by summing all the individual scores of the frames. Frame Frame score BowlingScorer { ; } public interface Frame public int score () The only naming problem here is that the method is not an action but could denote one. Instead, I will rename this to which clearly defines the word as a value and not an action. score getScore score The improved code looks like: { ; } public interface Frame public int getScore () The Strike Class The class implements both interfaces. It implements the because it needs to update the score with the next two rolls from following frames. It also implements the interface, so that a score can be extracted from it. Strike RollObserverFrame Frame The current version looks like: { score = Constants.PINS_PER_FRAME; count = ; { .score; } { ( .count < ) { .score += r; .count += ; } } } public , class Strike implements RollObserverFrame Frame private int private int 0 @Override public int getScore () return this @Override public void rollCallback ( r) final int if this 2 this this 1 My first comment is on the private member . What is this value counting? A little investigation shows it increases with each call to until it reaches 2. I am going to rename this to to be more expressive. count rollCallback addedBonusCount I also notice that the parameter is being passed into the 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 for clarification. r rollCallback roll The improved class looks like: Strike { score = Constants.PINS_PER_FRAME; addedBonusCount = ; { .score; } { ( .addedBonusCount < ) { .score += roll; .addedBonusCount += ; } } } public , class Strike implements RollObserverFrame Frame private int private int 0 @Override public int getScore () return this @Override public void rollCallback ( roll) final int if this 2 this this 1 The Spare Class Similar to the class, the class implements both the and interfaces. In this class only the very next rolls value should be added to the score. Strike Spare RollObserverFrame Frame Before improving the naming it looks like: { score = Constants.PINS_PER_FRAME; off = ; { .score; } { (! .off) { .score += roll; .addedBonus = ; } } } public , class Spare implements RollObserverFrame Frame private int private boolean false @Override public int getScore () return this @Override public void rollCallback ( roll) final int if this this this true My complaint here is to do with the private member 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 to indicate whether I have yet added a bonus. off addedBonus The improved version looks like: { score = Constants.PINS_PER_FRAME; addedBonus = ; { .score; } { (! .addedBonus) { .score += roll; .addedBonus = ; } } } public , class Spare implements RollObserverFrame Frame private int private boolean false @Override public int getScore () return this @Override public void rollCallback ( roll) final int if this this this true The Incomplete Class The class represents a frame where only one roll was made so far. It implements the interface like the other frame objects. However, it does not care about extra rolls, so it does not implement the interface. Incomplete Frame RollObserverFrame { fr; { .fr = fr; } { .fr; } } public class Incomplete implements Frame private final int public Incomplete ( fr) final int this @Override public int getScore () return this When the author named the and frames, I didn't mind the names because those are fairly specific bowling terms. , 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 . This creates an inconsistency with the other frame names, so I will similarly postfix them with the word ( becomes , etc.). Spare Strike Incomplete IncompleteFrame Frame Strike StrikeFrame Another problem in this class is the private member . This is a short-form (violating the rule on completeness) for so I will rename the value appropriately. fr firstRoll The updated class is now: Incomplete { firstRoll; { .firstRoll = firstRoll; } { .firstRoll; } } public class IncompleteFrame implements Frame private final int public IncompleteFrame ( firstRoll) final int this @Override public int getScore () return this The NonTenFrame Class The class represents a frame which is complete, but neither a strike nor a spare. NonTenFrame { fr; sr; { .fr = fr; .sr = sr; } { .fr + .sr; } } public class NonTenFrame implements Frame private final int private final int public NonTen ( fr, sr) final int final int this this @Override public int getScore () return this this The bad naming here is similar to the class, so I apply the same solution; renaming to and to . IncompleteFrame fr firstRoll sr secondRoll The improved version of the looks like: NonTenFrame { firstRoll; secondRoll; { .firstRoll = firstRoll; .secondRoll = secondRoll; } { .firstRoll + .secondRoll; } } public class NonTenFrame implements Frame private final int private final int public NonTen ( firstRoll, secondRoll) final int final int this this @Override public int getScore () return this this The Final Class The last class is the which represents the tenth frame of a bowling game. FinalFrame { score; { .score = frame.stream().reduce( , Integer::sum); } { .score; } } public class FinalFrame implements Frame private final int public Final ( List frame) final this 0 @Override public int getScore () return this The names in this class are already acceptable, but that is only true because I renamed it to be postfixed with . Before, this class would have been (and was) named . The word is a reserved word in Java, with very specific meaning. Creating a class muddies the waters with the reserved meaning. Renaming this to is a big improvement. Frame Final final Final FinalFrame 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. { PINS_PER_FRAME = ; NUMBER_OF_FRAMES = ; } { FrameFactory frameFactory; RollSubjectBroadcaster rollBroadcaster; { .frameFactory = frameFactory; .rollBroadcaster = rollBroadcaster; } { BowlingScorer( FrameFactory(), RollSubjectBroadcaster()); } { List frames = ArrayList<>(); ( List rawFrame : rawFrames) { .rollBroadcaster.broadcastRolls(rawFrame); frames.add( .frameFactory.construct( rawFrame, .rollBroadcaster)); } frames.stream().reduce( , (currentSum, frame) -> currentSum + frame.getScore()); } } { List rollObservers = ArrayList<>(); { .rollObservers.add(observer); } { ( roll : rawFrame) { ( RollObserverFrame observer : .rollObservers) { observer.rollCallback(roll) } } } } { builtFrames = ; { .builtFrames += ; ( .isLastFrame()) { FinalFrame(rawFrame); } ( .isIncompleteFrame(rawFrame)) { IncompleteFrame(rawFrame[ ]) } ( .isStrikeFrame(rawFrame)) { StrikeFrame strikeFrame = StrikeFrame(); rollBroadcaster.subscribe(strikeFrame); strikeFrame; } ( .isSpareFrame(rawFrame)) { SpareFrame spareFrame = SpareFrame(); rollBroadcaster.subscribe(spareFrame); spareFrame; } NonTenFrame(rawFrame[ ], rawFrame[ ]) } { .builtFrames == Constants.NUMBER_OF_FRAMES; } { rawFrame.length() == && rawFrame[ ] != Constants.PINS_PER_FRAME; } { rawFrame.length() == && rawFrame[ ] == Constants.PINS_PER_FRAME; } { rawFrame[ ] + rawFrame[ ] == Constants.PINS_PER_FRAME; } } { ; } { ; } { score = Constants.PINS_PER_FRAME; addedBonusCount = ; { .score; } { ( .addedBonusCount < ) { .score += roll; .addedBonusCount += ; } } } { score = Constants.PINS_PER_FRAME; addedBonus = ; { .score; } { (! .addedBonus) { .score += roll; .addedBonus = ; } } } { firstRoll; { .firstRoll = firstRoll; } { .firstRoll; } } { firstRoll; secondRoll; { .firstRoll = firstRoll; .secondRoll = secondRoll; } { .firstRoll + .secondRoll; } } { score; { .score = frame.stream().reduce( , Integer::sum); } { .score; } } public static class Constants public static int 10 public static int 10 public class BowlingScorer private final private final public BowlingScorer ( FrameFactory frameFactory, RollSubjectBroadcaster rollBroadcaster ) final final this this BowlingScorer public static fromDefaultFactoryAndBroadcaster () return new new new public int calculateScore ( List > rawFrames) final final new for final this this this return 0 public class RollSubjectBroadcaster private new public void subscribe ( RollObserverFrame observer) final this public void broadcastRolls ( List rawFrame) final for final int for final this public class FrameFactory private int 0 Frame public construct ( List rawFrame, RollSubjectBroadcaster rollBroadcaster ) final final this 1 if this return new if this return new 0 if this final new return if this final new return return new 0 1 private boolean isLastFrame () return this private boolean isIncompleteFrame ( List rawFrame) final return 1 0 private boolean isStrikeFrame ( List rawFrame) final return 1 0 private boolean isSpareFrame ( List rawFrame) final return 0 1 public interface RollObserverFrame public void rollCallback ( roll) final int public interface Frame public int getScore () public , class StrikeFrame implements RollObserverFrame Frame private int private int 0 @Override public int getScore () return this @Override public void rollCallback ( roll) final int if this 2 this this 1 public , class SpareFrame implements RollObserverFrame Frame private int private boolean false @Override public int getScore () return this @Override public void rollCallback ( roll) final int if this this this true public class IncompleteFrame implements Frame private final int public IncompleteFrame ( firstRoll) final int this @Override public int getScore () return this public class NonTenFrame implements Frame private final int private final int public NonTenFrame ( firstRoll, secondRoll) final int final int this this @Override public int getScore () return this this public class FinalFrame implements Frame private final int public FinalFrame ( List frame) final this 0 @Override public int getScore () return this 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.