Am I understanding the basics of OOP design/structure correctly?

[Adam Thompson]

I know a little PHP (not from an object oriented perspective) and am learning Java now. Here is the project I'm learning with: https://github.com/adamjthompson86/lotrgame/tree/master/src

Do I seem to be understanding the basics of how to structure my code in an OOP manner? Am I using classes, methods, etc. appropriately? Any suggestions or feedback? Thanks!

[Carey Brown]

if (characterCurrentDrinks > 7 && characterCurrentDrinks <9) System.out.println("Hear, hear. It's the dwarves that go swimming with little hairy women!");
if (characterCurrentDrinks > 9 && characterCurrentDrinks <11) System.out.println("I feel something. A slight tingling in my fingers.");

What happens if characterCurrentDrinks is exactly 9?

[Junilu Lacar]

As far as OOP and assigning responsibilities to each class, it doesn't look too bad.

Overall, your code is very poorly formatted. If lines of code were teeth, yours needs some serious orthodontic work. Alignment and indentation is random and misleading at best. If you're using an IDE like Eclipse, autoformatting your code is as easy as Alt+Shift+F.

Some of your methods are way too big. Methods should be less than 15 lines, ideally 1-5 lines of executable code. 20 lines is pushing it but sometimes it can't be helped. More than that and you're probably jamming too much together into one place.

For a program like this, having input/output in each class doesn't really complicate things but when you are dealing with larger problems, it will. Classes like Character usually don't do input/output. Larger programs are separated by layers: input/output on the presentation layer, saving data in a persistence layer, "business logic" in the domain layer.

[Junilu Lacar]

... and Welcome to the Ranch!  

[Junilu Lacar]

In case you're wondering how you'd get methods that are only 1-5 lines long, here's how I might rewrite your main() method:

public static void main(String[] args) {
     Character player = createPlayer();
     drinkUntilClosing(player);
}

private static Character createPlayer() {
     ... // big block of code that used to be in main()
}

private static void drinkUntilClosing(Character player) {
     ... // second section of code that used to be in main()
}

That's just for starters though. Each of the private methods can be busted up into even smaller methods.

private static void drinkUntilClosing(Character player) {
     DrinkingGame game = new DrinkingGame(player);
     do {
          game.play();
     } while (playAgain());
}

private static boolean playAgain() {
     System.out.println("Do you want to play again? yes/no ");
     return "YES".equalsIgnoreCase(input.nextLine());
}

This is just one alternative way to do it, of course. There are countless other ways you can write the code to tell a better story. I could tell the same story with one line in main():

public static void main(String[] args) {
     new DrinkingGame(createPlayer()).play();
}

Each way suggests where a lot of that code you have now would be moved to a different place and possibly busted up into smaller chunks. That's really what OOP is about: how to break your problem down into smaller, more manageable pieces in a such a way that the code ends up looking like you're just creating a bunch of objects and telling them to do things.

[Adam Thompson]

Carey Brown wrote:

if (characterCurrentDrinks > 7 && characterCurrentDrinks <9) System.out.println("Hear, hear. It's the dwarves that go swimming with little hairy women!");
if (characterCurrentDrinks > 9 && characterCurrentDrinks <11) System.out.println("I feel something. A slight tingling in my fingers.");

What happens if characterCurrentDrinks is exactly 9?

The game continues, and when they get to 10 drinks they see the next quote.

I realized that's pretty silly code, though. Since I'm dealing in whole numbers, I don't need (characterCurrentDrinks > 7 && characterCurrentDrinks <9) when (characterCurrentDrinks == 8) does the same thing with half the work!

[Adam Thompson]

Thanks for the help. I don't fully understand all the provided advice yet, but what I do understand has been helpful. Working now on improving the structure. Thanks!

[Campbell Ritchie]

Consider if‑elses. Put them in order. Only use the < and > operators, which means you need to alter the order of terms depending on where the boundary values fall. If the boundary values fall in the larger category, use < and ascending order:-

if (mark < 0 || mark > 100)
{
   throw new IllegalArgumentException(...);
}
else if (mark < 20)
{
   grade = U; // member of enum
}
else if (mark < 30)
{
   grade = F;
}
else if (mark < 40)
{
   grade = E;
}
else if (mark < 50)
{
   grade = D;
}
else if (mark < 60)
{
   grade = C;
}
else if (mark < 70)
{
   grade = B;
}
else if (mark < 80)
{
   grade = A;
}
else
{
   grade = A_PLUS;
}

If you include the boundary value in the lower category use descending order and > like with weights of parcels:-

// ...
else if (weight > 3500) // grams
{
   postage = new BigDecimal("11.50")
}
else if (weight > 3000)
{
   postage = new BigDecimal("10.50")
}
// ... etc

That now allows you to cover all poshibilitiesh about how mush your pershon hash had to drink.

[Junilu Lacar]

It seems to me that Carey's point regarding the use of relational operators > and < was orthogonal to OP's actual intent.

The code that Carey commented on and which Campbell expounded on was written in a way that was misleading: it looked like it had a logic flaw that left some values unaccounted for, hence the leading question, "What happens when it's exactly 9?" OP has since modified his code to better reflect his intent: to check specifically for the values of 8 and 10.

[Campbell Ritchie]

Good point. You can slip an else if (drinksh == 42) ... at the appropriate place in the code. Otherwise, the way I wrote successive if‑elses covers every value possible for the arguments.

There isn't a keyword elshe to deal with that amount of drink, is there?

[German Martinez]

Use meaningful names.

For example, the names of the input parameters were changed:

public Character(String a, String b) {
    name = a;
    race = b;
    // ...
}

public Character(String name, String race) {
    this.name  = name;
    this.race  = race;

    coins      = startCoins();          // Give character 10-30 coins to start with
    danceSkill = startDanceSkill();     // Assign a dance skill number based on race  
    maxDrinks  = startMaxDrinks();      // Give character max drink number
}

[Junilu Lacar]

Since OP asked specifically about OOP design and structure, I'm going to back up and not focus on precise language details. Instead, I'll focus on semantics. Semantics, which refers to the meanings of words and symbols within a context, are a very important yet subtle and nuanced part of design. Because it's subtle and nuanced, the importance of semantics in a design tends to get overlooked and overshadowed by concerns about language syntax and the basic mechanics of programming.

If I go back to my handy-dandy martial arts analogies, it's the difference between technique and form. For beginners, the focus is still mostly on form and understandably so. It doesn't hurt to pay a little attention to technique though, even while you're still a beginner. As you get better and more comfortable with the form, which in programming is the language syntax and mechanics, you should start looking at the techniques from a higher perspective.

The names in the root level of your project play a very important role in establishing the semantics, and therefore the design, of the program as a whole. If you came in knowing nothing about a project then saw classes with names like Account, Ledger, Debtor, Creditor, Receivable, Payable, Capital, Asset, and Liability, you could still easily guess that this project had something to do with accounting. Likewise, if you saw the class names Team, Player, League, Schedule, Statistic, and Game, you could probably guess with a good deal of certainty that this project was related to sports.

This illustrates how good names are central to good semantics in a design and how they can be crucial in helping others understand the program, even with a very brief, high-level glance.

Now, let's look at the class names used in this particular design. Someone coming in knowing nothing about this project will see the classes Character, DrinkingGame, Game, and LOTRGame.

Do these names work together to create a clear high-level picture of what the program is about? I would say it does, but only a little bit. Mostly, however, the names create some confusion and doubt. Why are there three different "Game" classes? What is the difference between these classes? People who are familiar enough with pop culture and the books or movies can easily figure out that LOTR stands for "Lord of the Rings" but how are Game and DrinkingGame and LOTRGame related to each other? Right off the bat, the names chosen have already obscured the intent of this design. Digging into the details of each class doesn't do much to clarify one's understanding of how everything falls into place either.

So, if you're going to think about your design and whether it comes together and makes sense, you might want to start with the top level names that everyone sees right away when they look at your project for the first time. The names you choose at this level will determine whether strangers to your design will feel comfortable and intuitively know how to find their way around or if they will feel lost, confused, and unable to find their bearings.

The high-level names also help define an outline that can guide you to making better decisions about where and how to assign responsibilities. Names will influence everything, from the kind of interactions make sense between classes and the kind of methods each class will have. Semantics are the thread and glue that holds your design together. The quality of the semantics determines whether your design stays together and holds its form or falls apart in a jumbled mess.

I'll stop here and let you think about these things for a little bit, maybe ask some questions. I hope it makes some sense to you though, even if you might not know what to do with it yet.

[Junilu Lacar]

I wrote:People who are familiar enough with pop culture and the books or movies can easily figure out that LOTR stands for "Lord of the Rings"

I take that back. You can't really assume that other people will see the name LOTRGame and create the same kind of associations you, as the author of the code, had in your head. I actually didn't make the connection between LOTR and "Lord of the Rings" until I saw a reference to "hobbit" buried in the code of that class.

[Marius Semeon]

I just wanted to say, reading through this this post has been tremendously helpful and intriguing - I'm a newbie too and I've just begun to start appreciating the attention to semantic detail and project organisation that I think Junilu Lacar is talking about, but having it described in such a fashion is very motivating. I am like the OP - I don't think I fully understand these explanation yet but I'm working towards doing so. I would love to learn the specifics in workflow/style towards writing more compact, eloquent and maintainable code- but it's difficult to learn it quickly, compared to nuts and bolts stuff like syntax (/ the appropriate usage of it) or expanding your knowledge of the API.
Regarding specific points in the OPs LOTR game and Campbell Ritchies response: When there are loads of if/else conditionals, isn't it generally preferable to use case/switch instead to make it easier to read?
Also, in the OPs Character class, two separate methods instantiate new Random objects each time they're called. Wouldn't it be simpler to declare and instantiate a random object as an instance variable of the Character class and just use this same object in each of the methods?

[Junilu Lacar]

Marius Semeon wrote:in the OPs Character class, two separate methods instantiate new Random objects each time they're called. Wouldn't it be simpler to declare and instantiate a random object as an instance variable of the Character class and just use this same object in each of the methods?

Yes, that would certainly be simpler. You could also declare a static field and instantiate a single Random object to be reused by all instances.

Since you seem interested in semantics, when it comes to random number generation, using ThreadLocalRandom.current() can also mean that the author might have had concerns related to concurrency. Consider what you would think if you saw some of the different ways you can generate a random number:

public class WhatWereYouThinking {
     
     private static final Random rand = new Random();

     // or

     private static Random rand = new Random();

     // or

     private Random rand = new Random();
     
     // or

     private int nextRoll() {
          // ???
          // Random rand = new Random();

          return rand.nextInt(6);

          // or

          return ThreadLocalRandom.current().nextInt(6);
     }
  
     // --------

     public void play() {
          // ???
          // Random rand = new Random();

          while (...) {
               int roll = rand.nextInt(6);

               // or

               int roll = nextRoll();
          }
     }
}

These are just a few of the alternatives you can consider choosing as you write the code. Various combinations of public/private/protected/(default), static vs. non-static, final vs. non-final, and random number generation method used have subtle differences in what they imply and what they might reveal about the author's intent when he/she wrote the code. If you read code like you're a detective looking for clues left behind at a crime scene, you can get some pretty good insights about what happened and what past author(s) of the code may or may not have been thinking.

[Junilu Lacar]

Marius Semeon wrote:I would love to learn the specifics in workflow/style towards writing more compact, eloquent and maintainable code- but it's difficult to learn it quickly, compared to nuts and bolts stuff like syntax (/ the appropriate usage of it) or expanding your knowledge of the API.

You have to start somewhere, so don't get too down on yourself about having to start at the nuts and bolts level and familiarizing yourself with the Java API. At the same time, study principles like SOLID, DRY, KISS, and GRASP. At first, they will seem way too advanced for you. Read about them anyway. Just being aware of them is an important first step to learning to master them. Study how the good programmers do their work and how they think. I'd recommend a couple of books:

Think Like a Programmer by V. Anton Spraul
Understanding the Four Rules of Simple Design by Corey Haines

To learn how to be a good programmer, you need to learn how to think like a good programmer. These books will help you do that.

[Campbell Ritchie]

Another thing we have all missed: there is a class called Character. That is the same as a class in the java.lang.package, and you will have problems when you try to use the original Character class.

[Junilu Lacar]

Here's an example of how the DRY principle (Don't Repeat Yourself) is applied to improve your code. BTW, the opposite of DRY is WET (Write Everything Twice).

public class WhatWereYouThinking {
     
      private static final Random rand = new Random();

      public void play() {
           while (...) {
                 int roll1 = rand.nextInt(6);
                 int roll2 = rand.nextInt(6);
                 int roll3 = rand.nextInt(6);
           }
      }

      // vs. 

      private int nextRoll() {
           return ThreadLocalRandom.current().nextInt(6);
    
           // or
           // return rand.nextInt(6);
      }

      public void play() {
           while (...) {
                 int roll1 = nextRoll();
                 int roll2 = nextRoll();
                 int roll3 = nextRoll();
           }
      }

      ...
}

An astute programmer would recognize a DRY violation on lines 7-9. In contrast, lines 24-26 might seem like they also violate DRY but they don't suffer from the same problem that lines 7-9 have.

What's the problem? Well, if you look at the formula carefully, nextInt(6) returns a number from 0 - 5. The intent is to get a number from 1 - 6. To fix this, you'd have to change all three of lines 7-9. Ok, no problem here because it's only three lines. But what if there had been hundreds of lines of code that used this same formula directly? You'd have to fix the bug in all those places!

How many lines of code do you have to change if you had the formula safely hidden away inside a method that revealed its intent instead, like the private nextRoll() method? Even if there were thousands of different places where you could find calls to nextRoll(), you'd still only need to change line 16 to fix the bug.

This is what happens when you adhere to or violate the DRY principle. Mastering this needs quite a bit of study and practice but now that you're aware of this, you can make your first step down the road toward mastery. Without that awareness, you might continue writing WET code for some time before you start recognizing the kind of problems you're creating for yourself and others.

When you first learn about DRY, you might get the impression that "duplication" refers to character-for-character or mechanical duplication of code. Yes, mechanical duplication can be a good indicator of DRY violations in most (but not all) cases. However, DRY is really about having the same expression of an idea in multiple places in your code. When an idea is duplicated as a formula—which is what nextInt(6) + 1 is—then you've violated DRY.  When the idea is duplicated as an abstraction—which is what a method call to nextRoll() is— then there's no violation of DRY.

In other words, DRY has to do with expressing abstraction without revealing implementation. This is a recurring theme in programming. The same theme runs through the SOLID design principles as well.

[Junilu Lacar]

Here's some good information about DRY and it's fraternal twin, SPOT (Single Point of Truth): http://www.catb.org/~esr/writings/taoup/html/ch04s02.html#spot_rule

This is a good article about DRY vs WET code

[Junilu Lacar]

Here's some code that has a strong WET smell for me:

public class DrinkingGame {

   String opponent;
   int opponentMaxDrinks;
   int characterMaxDrinks;
   int opponentCurrentDrinks;
   int characterCurrentDrinks;

Why does this smell WET? Because the ideas of "max drinks" and "current drinks" are inside the class DrinkingGame. Why does this seem WET? Because this seems like it would be DRY-er:

public class DrinkingGame {
    Character mainPlayer;
    Character opponent;  
    ...

You might ask, "Where did the ideas of 'max' and 'current' drinks go?" The answer is that they are hidden inside the abstraction of a Character and appropriately named methods:

    mainPlayer.getMaxDrinks();
    mainPlayer.getCurrentDrinks();

    opponent.getMaxDrinks();
    opponent.getCurrentDrinks();

Here, the violation of DRY helps us recognize flaws in the design and leads us to a better way of organizing responsibilities and modifying the API so that the code has better object-oriented semantics.

[Junilu Lacar]

A useful co-indicator of DRY is consistency. Consider that WET code again:

public class DrinkingGame {
 
   String opponent;
   int opponentMaxDrinks;
   int characterMaxDrinks;
   int opponentCurrentDrinks;
   int characterCurrentDrinks;

The names of the four int fields clearly have some duplication: "MaxDrinks" and "CurrentDrinks". They also hint at a larger grouping of ideas: that of "opponent" and "character".  However, when you see only an opponent declared above and no matching character, that hints at something that's out of balance in these declarations. This sense of imbalance gives rise to questions about the appropriateness of the design.

Questions like: why is opponent declared as a String? Who is this "opponent" opposing? Why isn't whoever that is more explicitly called out? Why aren't the ideas of "max drinks" and "current drinks" more strongly associated with the idea of a "participant" of the drinking game instead?

The DRY principle is an important guideline but it also works together with other measures of quality, like symmetry, semantics, and balance, to indicate problems in the design. Symmetry and balance combine to give consistency and semantics (how we associate meaning to things in a context) help us sniff out code that lacks consistency in both usage and meaning.

[Junilu Lacar]

Here's some more code that has poor semantics:
   DrinkingGame currentDrinkingGame = new DrinkingGame(currentCharacter.maxDrinks);    if (currentDrinkingGame.drink()) {        currentCharacter.addCoin(5);    }
When you're writing OOP code, do it as though you're trying to tell a story. Objects are the characters in the story and the methods you call on these objects tell what the objects are capable of doing. Interactions between objects also help tell a story. As you read the above code, ask yourself "What story was the author of this code trying to tell?" What does it mean to tell a DrinkingGame object to drink? What does it mean when drink() returns true. What does it mean when drink() returns false? Wouldn't it make more sense to tell a Character object to drink? What does it mean to add a coin to a Character object when the DrinkingGame.drink() method returns true? What idea is this trying to convey?

What if the code were written differently:

DrinkingGame drinkTilYouOrc = new DrinkingGame();

drinkTilYouOrc.addContestant(new Character("Gimli", Race.DWARF));
drinkTilYouOrc.addContestant(new Character("Frodo", Race.HOBBIT));
drinkTilYouOrc.addContestant(new Character("Legolas", Race.ELF));
drinkTilYouOrc.addContestant(new Character("Aragorn", Race.HUMAN));

do {
   drinkTilYouOrc.playOneRound();
   drinkTilYouOrc.showWhosStillStanding();
} while (!drinkTilYouOrc.isOver());

System.out.printf("Only %s remains standing. All the rest have Orc'ed!%n", drinkTilYouOrc.winner());

Can you get a better sense of a story from this code?

[Junilu Lacar]

Adam Thompson wrote:Do I seem to be understanding the basics of how to structure my code in an OOP manner? Am I using classes, methods, etc. appropriately?

The more I read through the code, the less I understand your motivation for structuring it the way you did and choosing the names and relationships of the classes the way you did. The LOTRGame class creates a Game object which in turn creates a Character object. Then it creates a new DrinkingGame object each time the user says they want to keep playing. The whole thing seems like an overly complicated Rube Goldberg machine that uses way too much code to achieve its goal.

It seems to me that having three different "game" classes is unnecessary. Try to cut it down to one class that drives the flow of the game. Then try to make the methods smaller and focused on smaller tasks. Try to see what each task logically belongs to: either a Character object or the Game object.

Then think of the high-level code you write as a script, and each object plays its role, interacting with the other object as needed. Kind of like "Game object asks user which character he wants to play. Game object creates a random opponent character. Game object tells the main character to drink. Game object tells the opponent to drink. Each character drinks and updates its status based on its race and inherent ability to hold up and not pass out. Game object checks if opponent is still standing. Game object checks if main character is still standing. Game is over when one or both are too drunk to stay standing."

When you have a script like this, it's easier to see where fields should be assigned and initialized, what methods need to be assigned to each class, what the return types should be, etc.

[Junilu Lacar]

There are more violations of DRY.

In the constructor of the DrinkingGame class, you have this:
     // Choose opponent      opponent = "Gimli";      //set winning number of drinks          Random rand = new Random();      int maxDrinks = rand.nextInt(10) + 10;      opponentMaxDrinks = maxDrinks;
In the Character class, you have this constructor:
public Character(String a, String b) {      name = a;      race = b;      // Give character 10-30 coins to start with      coins = startCoins();      //Assign a dance skill number based on race          danceSkill = startDanceSkill();      // Give character max drink number      maxDrinks = startMaxDrinks(); }
and these methods:
  private int startCoins() {      Random rand = new Random();      int startCoins = rand.nextInt(10) + 20;      return startCoins;   }   private int startMaxDrinks() {      Random rand = new Random();      int startMaxDrinks = rand.nextInt(10) + 10;      return startMaxDrinks;   }
There are a number of separate DRY violations here:
1. The idea of initializing a character is duplicated in the constructors of the DrinkingGame and Character
2. The idea of initializing "max drinks" to a number from 10-20 is replicated in DrinkingGame and Character.
3. The idea of producing a number from N-M is replicated in a few places

Here are possible fixes:

1. In the DrinkingGame constructor, just have this:
Character opponent = randomOpponent();
Then, implement randomOpponent() as something like this:

private static final Character[] opponents;

static {
    Map<String, Character> lineup = new HashMap<>();

    lineup.put(new Character("Gimli", DWARF));
    lineup.put(new Character("Legolas", ELF));
    lineup.put(new Character("Aragorn", HUMAN));
    lineup.put(new Character("Gandalf", WIZARD));
    ... // add as many as you like

    opponents = lineup.entrySet().toArray(new Character[]);
}

private Character randomOpponent() {
   return opponents[ThreadLocalRandom.current().nextInt(opponents.length)];
}

The values DWARF, ELF, HUMAN, WIZARD would be defined by the enum Character.Race and statically imported in the DrinkingGame class.

Of course, you'd have to change your Character class like so:

public class Character {

   public enum Race { DWARF, ELF, WIZARD, HUMAN, ORC }

   private final String name;
   private final Race race;
   
   public Character(String name, Race race) {
       this.name = name;
       this.race = race;

       ...
   }

   ...
}

(continued...)

[Junilu Lacar]

(... continuing)

There is a bug in the way you're generating a random number that falls within a given range, like [10-20].

Your intent is stated as a comment in the Character constructor:
// Give character 10-30 coins to start with coins = startCoins();
But the implementation only generates numbers from 10 to 29 because nextInt(N) produces a random integer from 0 to N-1. Edit: (See how unreliable comments are? At least I got it right in the code comment) That should be: But the implementation actually generates numbers from [20-29] because nextInt(10) will return a random integer from [0-9].

private int startCoins() {      Random rand = new Random();      int startCoins = rand.nextInt(10) + 20; // BUG: [0-9] + 20 => [20-29]      return startCoins; }
The comment is attached to a line of code that is abstract and free of implementation detail yet the comment reveals an implementation detail (10-30). It's not good to specify an implementation detail in a comment that is far away from the actual implementation code. The reader will either trust the comment to be true (and thus be misled because of the bug) or he will have to jump to the implementation code in startCoins() to try to find the bug. The theme of separating abstractions from implementation should be applied to comments as well.

There's also another level of duplication here. You also have this method:
private int startMaxDrinks() {      Random rand = new Random();      int startMaxDrinks = rand.nextInt(10) + 10;      return startMaxDrinks; }
Do you recognize the replicated idea here? It's the idea of generating a random number that falls in a specific range. Do you see how the bug is also replicated? If you wanted max drinks to be a random number from [10-20] then this implementation is wrong because it will only produce numbers in the range [10-19]. It has the same kind of mistake the other code does. Also—and this has already been pointed out—you don't need to use a local Random instance, you only need one Random object to be instantiated when you load your class, so it's best to declare a Random static field instead.

Or you could just use ThreadLocalRandom.current(). (NOTE: You might have noticed by now that I like to use ThreadLocalRandom.current() to get a random number generator. Honestly, I just learned about this class recently from a post by the author Simon Roberts and I can see that aside from the thread-safety, it also frees me from having to manage an instance of Random. Yes, it's a little more wordy than rand.nextInt() and I may tire of it after a while but right now I'm still in the "shiny new toy" phase)
     
Getting back to refactoring to remove duplication, here's a possible fix:

public Character(...) {
      ...
      coins = startCoins();
      maxDrinks = startMaxDrinks(); 
      ...
}
     
private int startCoins() {
      return randomInRange(10, 30);
}

private int startMaxDrinks() {
      return randomInRange(10, 20);
}

private int randomInRange(int fromInclusive, int toInclusive) {
      return ThreadLocalRandom.current().nextInt(toInclusive - fromInclusive) + fromInclusive + 1;
}

Do you see how this eliminates duplication and makes the code much DRY-er?

Note that you should remove the comment in the Character constructor that reveals implementation detail about the range of numbers being generated. Your method name is expressive enough to give the reader an idea of what's happening when you call startCoins() anyway. If the reader wants to know more details about the implementation, they know where to look. Let the code tell the story. If you're going to write comments, put them near the implementation code and explain why you're doing something in a certain way. Otherwise, the code should be clear enough to make what it's doing obvious to the reader.

[Junilu Lacar]

Now, when you realize that there's still a bug in the randomRange() implementation I showed above, how many lines of code do you need to change? If you hadn't refactored and eliminated duplication, you'd have to go find and change all the places where the bug was duplicated.

[Junilu Lacar]

You may be wondering about this:

I wrote:Then, implement randomOpponent() as something like this:

private static final Character[] opponents;

static {
    Map<String, Character> lineup = new HashMap<>();

    lineup.put(new Character("Gimli", DWARF));
    lineup.put(new Character("Legolas", ELF));
    lineup.put(new Character("Aragorn", HUMAN));
    lineup.put(new Character("Gandalf", WIZARD));
    ... // add as many as you like

    opponents = lineup.entrySet().toArray(new Character[]);
}

private Character randomOpponent() {
   return opponents[ThreadLocalRandom.current().nextInt(opponents.length)];
}

The construct that starts on line 3 is called a static initialization block. You can find more information about them if you follow that link.

That's just one way to code it though (that was my first cut). The alternative below shows how first-cut code can often be way more complicated than it needs to be.

// A simpler, more straightforward way to do it:

private static final Character[] opponents = {
    new Character("Gimli", DWARF),
    new Character("Legolas", ELF),
    new Character("Gandalf", WIZARD),
    new Character("Aragorn", HUMAN),
    new Character("Thorin", DWARF),
    new Character("Tauriel", ELF),
};

You might have noticed that line 9 looks like it might produce a compiler error since there's nothing between the comma at the end of it and the closing brace on line 10. I'm not sure if this has always been allowed but newer versions of the Java compiler allow it. This is a nice feature that languages like Go have and it makes it easier to add more items later on. If you don't have that extra comma then decided later on to add more characters, you will get a compiler error if you don't first add a comma at the end of line 9.

My point here is that you should take comfort in the fact that even experienced developers are not immune to writing overcomplicated code.

[Adam Thompson]

Thanks all. I am digesting the input and will be studying over the next week or so.

Junilu Lacar wrote:

Adam Thompson wrote:Do I seem to be understanding the basics of how to structure my code in an OOP manner? Am I using classes, methods, etc. appropriately?

The more I read through the code, the less I understand your motivation for structuring it the way you did and choosing the names and relationships of the classes the way you did. The LOTRGame class creates a Game object which in turn creates a Character object. Then it creates a new DrinkingGame object each time the user says they want to keep playing. The whole thing seems like an overly complicated Rube Goldberg machine that uses way too much code to achieve its goal.

It seems to me that having three different "game" classes is unnecessary. Try to cut it down to one class that drives the flow of the game. Then try to make the methods smaller and focused on smaller tasks. Try to see what each task logically belongs to: either a Character object or the Game object.

Then think of the high-level code you write as a script, and each object plays its role, interacting with the other object as needed. Kind of like "Game object asks user which character he wants to play. Game object creates a random opponent character. Game object tells the main character to drink. Game object tells the opponent to drink. Each character drinks and updates its status based on its race and inherent ability to hold up and not pass out. Game object checks if opponent is still standing. Game object checks if main character is still standing. Game is over when one or both are too drunk to stay standing."

When you have a script like this, it's easier to see where fields should be assigned and initialized, what methods need to be assigned to each class, what the return types should be, etc.

I think the Game class is not needed. That stuff should be in the LOTR class, I created the Game class as a feeble attempt to try to break up an overgrown main method. But I think I can just create new methods under the LOTRGame class and call those methods from within my main method.

My goal for the game is for there to be multiple games within the game. LOTRGame is the overall game you're playing, then your character can play multiple games at the pub - DrinkingGame, DancingGame, ArmWrestleGame, etc. Based on this, does it make sense for me to have a separate DrinkingGame class like I do?

[Junilu Lacar]

When you put it that way, it makes more sense. The thing to watch for, in that case, is having your driver class, LOTRGame, know too much detail about the specific game classes. Ideally, you wouldn't have to change this class if you added more games in the future. This means you'll probably need some kind of generic Game interface that the driver class is programmed to and that each specific game class will implement. You might also want to rethink your nomenclature. Calling your driver class a "game" is still confusing to other people. A qualifier like "Driver" or "Main" might be better.

This will keep you in line with the SOLID design principles.

[Junilu Lacar]

Here's what I mean by not knowing too much about specific Game classes:

// Violates SOLID
public class LOTRGame {

    public static void main(String[] args) {
         ...
         Game currentGame = chooseGame();

         if (currentGame instanceof DrinkingGame) {
             ((DrinkingGame) currentGame).drink();

         } else if (currentGame instanceof ArmWrestlingGame) {
             ((ArmWrestlingGame) currentGame).wrestle();

         } else if (currentGame instanceof ...) {
             ...
         }
    }
}

You should be like if you see this kind of code.

Instead, you'd want something like this:

public interface Game {
    void addPlayer(Character player);
    void play();
    Character winner();
}

and

public class LOTRMain {

   public static final LOTRMain INSTANCE = new LOTRMain();

   public static void main(String[] args) {
       INSTANCE.execute();
   }

   // prevent external instantiation - force use of INSTANCE instead
   private LOTRMain() {}

   private List<Game> games = new ArrayList<>();

   public void register(Game newGame) {
       games.add(newGame);
   }

   private void execute() {
       Character mainPlayer = createMainPlayer();
       Game currentGame = chooseGame();
       do {
            currentGame.addPlayer(mainPlayer);
            currentGame.play();
            showWinner(currentGame.winner());
       } while (playSomeMore());
   }

   ...
}

Then you'd have to figure out a way to call LOTRGame.INSTANCE.register() for each Game implementation you add to your project later on. I have a way to do that but I'll leave it to you for now as a challenge.

[Marius Semeon]

Junilu Lacar wrote:
Can you get a better sense of a story from this code?

Yes it seems a lot clearer written in this way. And I understand the idea of writing many short methods and adding the functionality to them (your roll example above) so that if you have to later change it you only have to change the called method as opposed to modifying the (possibly hundreds) of lines of code calling an equation directly in more WET code. Thank you for introducing me to all these concepts, I'd never heard of WET, DRY, SOlID, etc. I will have a look at those books too. Like OP said, there's a lot to take on board here and let sink in.  

[Junilu Lacar]

Marius Semeon wrote:Thank you for introducing me to all these concepts, I'd never heard of WET, DRY, SOlID, etc. I will have a look at those books too.

My pleasure. Yes, it's unfortunate that schools don't seem to emphasize these kinds of concepts more. Those principles are a very important part of learning how to program well. Go get those books, you won't regret having them. Even when you've had a few years of experience under your belt, you'll still find them useful. I know I did when I read them not that long ago.

[Junilu Lacar]

I wrote:

DrinkingGame drinkTilYouOrc = new DrinkingGame();

drinkTilYouOrc.addContestant(new Character("Gimli", Race.DWARF));
drinkTilYouOrc.addContestant(new Character("Frodo", Race.HOBBIT));
drinkTilYouOrc.addContestant(new Character("Legolas", Race.ELF));
drinkTilYouOrc.addContestant(new Character("Aragorn", Race.HUMAN));

do {
   drinkTilYouOrc.playOneRound();
   drinkTilYouOrc.showWhosStillStanding();
} while (!drinkTilYouOrc.isOver());

System.out.printf("Only %s remains standing. All the rest have Orc'ed!%n", drinkTilYouOrc.winner());

Can you get a better sense of a story from this code?

@Marius: I'm glad you found that it told a better story but at the same time did you smell something off in that code? Did you notice that almost every line starts with drinkTilYouOrc.? Code that looks like this is telling you "You should move all those lines to the DrinkingGame class" referring to lines 3-11.

[Campbell Ritchie]

drinkTilYouOrc? What a strange name for a variable.
As I told you earlier, calling a class Character is storing up trouble for the future, especially if you are in the unnamed package.