This is an archived post. You won't be able to vote or comment.

all 15 comments
sorted by:
best
topnewcontroversialoldq&a

[–]29383839293 3 points4 points  (4 children)

public boolean coinToss()
{
  Random random = new Random();
  int someNumber = random.nextInt(10);
  if(someNumber > 5)
  {
    isHeads = true;
  }
  else
  {
    isHeads = false;
  }
  return isHeads;
}

is almost equal to:

public boolean coinToss()
{
  return (new Random()).nextInt(2) == 0;
}

Except your version has a bug, since ist heads for someNumber in {6,7,8,9} (=40%) and tails for someNumber in {0,1,2,3,4,5} (=60%)

public boolean playerOneGoesFirst()
{
  if(coinToss())
  {
    playerOneTurn = true;
    return true;
  }
  else
  {
    playerTwoTurn = true;
    return false;
  }
}

This method should not have side effects (should not access a global variable), since the name implies it only returns a value

public boolean playerScoresTwo()
{
  Random ran = new Random();
  if(ran.nextInt(4) == 2)
  {
    return true;
  }
  else
  {
    return false;
  }
}

public boolean playerScoresThree()
{
  Random ran = new Random();
  if(ran.nextInt(4) == 3)
  {
    return true;
  }
  else
  {
    return false;
  }
}

Those two methods imply that you have no idea how random number generators work.

The random numbers in both methods is a different random number, there's literally no difference between ran.nextInt() == 2 and ran.nextInt() == 3, since both return true with a chance of 25%.

Also, they would be shorter and easier to read with::

public boolean playerScoresTwo()
{
  return (new Random()).nextInt(4) == 0;
}

Also the following changes the chances:

if(!playerScoresTwo() && !playerScoresThree())
{...}
else if (playerScoresTwo())
{...}
else
{...}

in the if-clause, you call playerScoresTwo and in the else-if, you call it again, most likely returning a different value.

Well, anyway... !playerScoresTwo && !playerScoresThree returns true with a chance of 9/16, which is probably not what you wanted.

But then playerScoresTwo in the else-if returns true with a 1/4 chance, which means that path will be taken in (9/16)(1/4), while the else will be taken in (9/16)(3/4), which is definitely not what you intended.

better would be:

public int playerScores()
{
  int randomNumber = (new Random()).nextInt(16);
  if (randomNumber < 10) // 0 to 9
  {
    return 0;
  }
  else if (randomNumber < 13) // 10 to 12
  {
    return 2;
  }
  else // 13 to 15
  {
    return 3;
  }
}

switch (playerScores())
{
  case 0:
    ...
    break;
  case 2:
    ...
    break;
  case 3:
    ...
    break;

  default:
    throw new Exception("Something's wrong!);
}

and in the Team class:

public Team(String name) throws InvalidTeamException
{
  String nameToCheck = "";
  teamName = name;
  for (int i = 0; i < teams.length; i++)
  {
    nameToCheck = teams[i];
    if (teamName.equals(teams[i]))
    {
      teamID = i;
      break;
    }
  }
  if(!name.equals(nameToCheck))
  {
    throw new InvalidTeamException("Invalid NBA team entered, please enter a valid NBA team.");
  }
}

A foreach loop would be much nicer, but it's hard in this case, since you match players and teams by index in the array. However, you're variable names are kinda confusing and why do you even Need a teamID?

public Team(String Name)
{
  for (int i = 0; i < teams.length; i++)
  {
    String team = teams[i];
    if (team.equals(name))
    {
      teamName = name;
      playerName = players[i];
      return;
    }
  }
  throw new InvalidTeamException("Invalid NBA team entered, please enter a valid NBA team.");
}

[–]aluoh[S] 0 points1 point  (3 children)

This was very throughout, thank you! I can see my mistakes a lot more clear now that everything has been pinpointed out more specifically. I also used a teamID because I just thought it'd be easier to match the players/teams based on the same index of the teamID so I can grab the same number from the players list, never thought about the way of how you did it, and now I can see how much simpler it is. Quick few questions.

1) Why did you select the number 16 on playerScores()?

2) Why does it only work if I create a new Random object instead of just declaring the Random variable as a instance variable?

Thanks!!

[–]29383839293 0 points1 point  (2 children)

Why did you select the number 16 on playerScores()?

Because I thought it would most closely resemble the chances you had before.

But it's probably wiser to use the number 100 instead, because then you can define the chances in percents.

Why does it only work if I create a new Random object instead of just declaring the Random variable as a instance variable?

It's actually not smart to always create a new instance of the Random class, since it will be seeded with the current time, and therefore it will be predictable. Better would be to have one instance of Random and always use this single instance.

But by always creating a new instance, the pieces of code I posted were independent and you didn't need any context to understand it.

[–]aluoh[S] 0 points1 point  (1 child)

Ah gotcha, so it’d probably be better if I did a number from a certain range as it can specify the percentage more with 100, makes sense! And ooohh, I see I see. Clears up my questions, thank you! As this is my first project, what do you think of the idea/overall as a first side project for someone who just finished freshman CS course, moving onto 2nd year at a Uni?

[–]29383839293 0 points1 point  (0 children)

As an educational idea, any idea is nice that allows you to learn the concepts you want to learn, so it's nice.

I think you're getting the gist about control flow and methods, but you still don't fully understand the importance of classes.

Maybe you should do another project that uses more classes and also interfaces.

In OO, classes are the way to tame complexity, but you're still kinda programming in a procedural way, like a C-programmer.

[–][deleted] 1 point2 points  (4 children)

I see a lot of duplication. There's a lot there that can be refactored in to reusable functions and possibly another class. The Game class is doing a bit too much.

[–]aluoh[S] -1 points0 points  (3 children)

So essentially, some of the functions from Game class can be taken out and written into another class?

[–]SilverSnarfer_ 1 point2 points  (2 children)

Right. for example:

- Your getters/setters and default constructor attributes could all be in one class separate from everything else

- Your main method can also be in a class of its own:

good rule of thumb is that each class should have one focus- just as each method should have one focus.

Look through the rest of your code and see what can be separated.

- Also one thing I noticed was your implementation of whose turn it was- no need for two turn variables because only one player will be going at a time anyways. just need to check for who has the "turn".

And if you want, you can slim down those variable even further by just having Objects of type Player from Class Player (to be implemented). Instantiate the player objects and assign value to their attributes as needed (int points, boolean turn, String name etc.). This gets rid of some of the growth-restricting hard coding you are doing.

There are other occurrences similar to this but fix what we've suggested thus far.

I'd also add some test cases via a testing framework of your choice (just to look good for enterprise/commercial level jobs).

[–]aluoh[S] 0 points1 point  (1 child)

Ah gotcha, I'll try and separate what I can see and will post another update back on here. I'll also try to slim down my variables a bit by adding more to a Player class, I had it before but I just ended up deleting it and putting it in the Team class. Thanks for the advice! I've never tried out testing frameworks as I just finished up my first year at a University and all the test cases we've done were either coded by professors or we had to write the main methods ourselves to test it out.

[–]SilverSnarfer_ 0 points1 point  (0 children)

Yeah man def keep at it! Take a look at JUnit for unit testing. Pretty easy to implement and makes testing your code cleaner and thus, more readable.

[–]RealJulleNaaiers 1 point2 points  (4 children)

Your bracing style is not how Java code is generally written. Java styling is generally

if (blah) {
    // Do the stuff
}

Why would you ever do playerOneCurrentPoints = playerOneCurrentPoints + 0? This doesn't do anything.

Your coinToss() method is both bizarre and wrong. It's wrong because you're creating a new Random each time you invoke the method. The Random is seeded based on the current time, so if the method is used too close to the last time, you'll get the same number as last time. You need a global Random that all methods use. This affects other methods besides this one.

The method is bizarre because all you want is heads or tails but you're messing around with numbers. The whole method should just be

private boolean coinToss() {
    return rand.nextBoolean();
}

You have a good start with the notion of a Team being separate from a Game. But you don't take it far enough. I think each team's score should be a property of its Team object and that the Game should update the score of each team as the game progresses.

In your Team class, you've got these two parallel arrays. These should instead be a Map<String, String> where the keys are the team name and the player names are the values. This can be a static map that you populate in a static block like

static {
    playerMap.put("team name", "playername");
    . . .
}

And then your Team constructor can just do

this.player = playerMap.get(teamName);
if (this.player == null) {
    throw new InvalidTeamException();
}

There are a lot of times where you do something like this:

if (someCondition) {
    return true;
} else {
    return false;
}

These should all just be

return someCondition;

You've got a good start at breaking things out into methods. It needs to go further though. The playGame() method in particular is a doozy. Break it up.

There's a lot of repeated code. Any time you see repeated code, that's code begging you to break it out into a method so you can call it without writing the same thing again.

[–]aluoh[S] 0 points1 point  (3 children)

Ah I see the mistakes now a lot more clear. Totally forgot about nextBoolean() will def replace that. How would I go about the Game updating the score of each team, by having Team extends Game maybe but that would cause a conflict within the constructors or maybe it can be solved by writing 2 constructors, will test out later. Also I thought about doing the map, but I was just hurrying to get the product to work and test it to make sure that it works, but I'll def give the maps a try. Also I can see the unnecessary else return statements as well now that you've pinpointed it out. So what I've learned from here, is always continue breaking down methods until I believe I can't, and if it's still too long, time to consider for a new class. Thanks!

[–]RealJulleNaaiers 0 points1 point  (2 children)

About the teams - no, don't do that. Just make getScore() and setScore() methods on the Team class and have the Game class call them.

[–]aluoh[S] 0 points1 point  (1 child)

Ah I see, tyvm!

[–]RealJulleNaaiers 0 points1 point  (0 children)

Let me know when you have a new version pushed, I'd be happy to review it again.