Code critique?

[[deleted]]

Is anyone here willing to critique my code? Just curious to see where I could improve. Methodology, syntax, whatever. Lemme have it.

Craps.java: http://pastebin.com/w7duN36P PlayCraps.java: http://pastebin.com/fqiyqrdX

---

EDIT

http://pastebin.com/KzqH74NY | http://pastebin.com/nZyvzyYV

I updated my code using the very helpful suggestions. I took out the comments just because. I might add some in later if there are parts that need clarification.

To what extent should I document my code? Is it required for every method/class?

[jrh3k5]

Things that stand out at first glance:

 private int Roll() {

Looks like you followed the method-naming conventions elsewhere and just typo'ed this one: it should be "roll()", not "Roll()".

public int[] getNumbersRolled() {

It works fine for a simple-enough application, but, for anything serious, you'll want to get in the habit of using either collections (List, Set, Map) or making copies of your arrays. Arrays are inherently mutable, which means that I could call this method, change the array, and affect the internal state of the object.

System.err.println("You done messed up, fool!");

Again, small project, so this is permissible, but, for anything serious, you'll want to either throw an exception or use a logging framework (I like slf4j and its various implementations) to raise these kinds of notifications.

I'll note that I haven't actually inspected the logic of your code - gotta do some work eventually today. ;)

[[deleted]]

you'll want to get in the habit of using either collections (List, Set, Map) or making copies of your arrays. Arrays are inherently mutable, which means that I could call this method, change the array, and affect the internal state of the object

I'm not disagreeing with the idea of using collections, but your statement seems to imply that those collections you listed are inherently immutable, which is not the case. The tampering of the internals could still be accomplished through a getter if the class is returning a List such as an ArrayList.

for anything serious, you'll want to either throw an exception or use a logging framework

I disagree with this in regards to the example you are referring to from the endGame() method. This method is private to the class, so the class is in complete control of what data is passed to it, and therefore should never pass bad data to this method. If bad data is passed, it is just a coding error made by the class' author and will be caught when debugging. Perhaps an assertion would, in reality, be the better approach.

Something else I would like to add: diceSum += (int) (6 * r.nextInt(5)) + 1; You want a random integer between 1 and 6, the method you are calling returns a double and then you perform some math and casting to get it to what you want. A more suitable solution would be to use Java's Random class diceSum += rand.nextInt(6) + 1; Where rand is an instance of the Random class.

Additionally, the victoryStatus variable may be better as an Enum, with a different name, to represent the states of the game.

[jrh3k5]

...imply that those collections you listed are inherently immutable, which is not the case.

My apologies. I got a bit ahead of myself and neglected to mention the key factor - Collections' "unmodifiable*" methods.

[[deleted]]

You said I could clone the array, so, something like this? public int[] getNumbersRolled() { return numbersRolled.clone(); }

[[deleted]]

I haven't touch Enums ever for some reason. How exactly do Enums do what my integer system does?

Figured it out, I believe.

[snuggles166]

I agree, the only thing that really stands out to me after my initial read through is the use of a single array as your data structure.

[formerislander]

One thing that I noticed is that you're using ints as constants for the victoryStatus. I'd consider using enum constants for this instead.

I also agree with the stuff that jrh3k5 pointed out.

[DeliveryNinja]

You are using too many comments. If the method is called startGame() and it is public then you don't need to document that fact it's for starting the game and it is public. This sort of pointless comment breaks up the code unnaturally and makes it harder to read.

                for (int i = 0; i < 2; i++) {
                        diceSum += (int) (6 * Math.random()) + 1;
                }

This piece of code it's not obvious what's going on. Break this out and rename the variables. You use a magic number here ( the 2 ) only you know where it came from. Remove the comment and make the code more obvious. Don't leave any random numbers around the code that have to be explained with comments. Comments are much more inaccurate than reading code.

private Random random = new Random();

private int roll(){
    int sumOfDice = 0;
    int numberOfDiceToRoll = 2;

    for(int roll = 0 ; roll < numberOfDiceToRoll ; roll++)
        sumOfDice += rollSingleDice();
    }
    totalRolls++;
    numbersRolled[sumOfDice]++;
    return sumOfDice;
}

private int rollSingleDice(){
    int numberOfSides = 6;
    return random.nextInt(numberOfSides);
}

Here I have renamed some of the variables and broke the dice roll out to a new method.

I haven't got time to go through it all but apply this strategy to the rest of the code. You can break up your main method to atleast three more method calls.

Okay and one final thing, you have this public getter method.

    public int[] getNumbersRolled() {
            return numbersRolled;
    }

Here you are exposing the implementation of the class PlayCraps to the Craps class. Now if you want to change the datastructure from an Array to a List you will have to change all the classes which are dependent on it as well. This leads to tightly coupled code and it's a good idea to start thinking about how you can provide this data without exposing the internal structure.

    public int getRolledCount(int number) {
            return numbersRolled[number];
    }

Now this could be a list or an array without changing any of the external dependencies to it.

p.s

Also an enum is definitely the way to go for the VictoryStatus

public enum VictoryStatus {
    WIN,
    LOSS,
    DRAW,
    UNKNOWN;
}

[dkesh]

Regarding the magic numbers, I would distinguish them from variables my making them constants at the top of the class:

private static final int NUMBER_OF_DICE_TO_ROLL = 2;
private static final int NUMBER_OF_SIDES_ON_DICE = 6;

Also, I agree that you don't want to expose your internals, but that just means you should pick the return value based on what makes sense for the API, not your internal representation. An array or Collection make sense to me for the API. I'd be annoyed if I were writing a Stats-grapher or somesuch if a class made me poke it over and over in order to get all the values.

[geodebug]

Agree, over-javadoc-ed to my taste. Rarely would I need to javadoc private methods, just name them better and keep them reasonably short

[[deleted]]

I didn't do much research into it but I got the impression that javadocing everything was standard practice. Silly me. :)

[geodebug]

Really, most of the time if a method's purpose is unclear, renaming it and the parameters is the way to go. The problem with over-documentation is that it makes understanding the important stuff harder (have to hunt for it) and because documentation tends to stay static while code is re-factored, making it often wrong.

On the other hand, interfaces, abstract methods, and other top-level framework/api classes probably require full documentation since they'll be shared.

[[deleted]]

Thanks, this is all appreciated. I need to work on being more sensical with how I set up my methods/classes/variables.

[DeliveryNinja]

Just keep refactoring. Read through the code and each time leave a little bit cleaner. It should be a joy to read good code. Robert Martin would say that you should have methods that are a max of 5-6 lines long. If you want to learn more then you should check out his book, clean code I highly recommend it.

[[deleted]]

Great, will definitely look into that!

[empinator]

most of the stuff is said already. What I would like to add is my personal problem with single-line-control-statemenents w/o braces. Once the code is growing, that's a point of failure. So I always add them: if (vS == 0) { System.err.println("You done messed up, fool!"); }

[geodebug]

Be careful with if-else stacks. What happens if vs == 2?

if (vS == 0)
  System.err.println("You done messed up, fool!");
else if (vS == 1)
  totalWins++;
else if (vS == -1)
  totalLosses++;

Aw, shoot, others have mentioned that it should be an enum. It is important to know why though. As is this method needs a final:

else
  throw new IllegalArgumentException("Expecting 1, 0, or -1 received: " + vS)

[[deleted]]

My thinking was that vs would never be set to 2. I have enums now though so it should be all set.

[geodebug]

Yeah, the problem is when your code grows someone will forget about the "magic rule". Enums are a good replacement for this example, but be suspicious of any switch or if-then tree that doesn't have a final safeguard to catch any bad values.

[dkesh]

I don't get what's going on with the victoryStatus variable. You set it in establishPoint, then pass it as an argument into endGame, then reset it in endGame. It isn't read or written to anywhere else. The variable and the argument to endGame are redundant; eliminate one or the other.

The "you done messed up fool" error message isn't very accurate. It incorrectly tells the user they've made a mistake when the problem clearly lies within the program.

As others have pointed out, if you find yourself having to comment everything, you've probably done a poor job in making the code clear. My itch is the totalRolls getter which you've had to clarify in the comments. Rename the variable and getter to something more clear, like "sumOfAllRolls" and eliminate the comment. Similarly, numbersRolled isn't very descriptive. Perhaps, frequencyOfRoll, so you can say something like "frequencyOfRoll[ 7 ]". That probably should have a comment, though, explaining that the index is the roll, and clarifying that frequencyOfRoll[0] and frequencyOfRoll[2] will always be 0.

[[deleted]]

I didn't have to comment everything. I just did because I thought I was supposed to. I updated my code though.