CODEWITHSUNDEEP
c#
user3395953
user3395953

Reputation: 19

Trouble looping through array

I'm trying to loop through the array, and if all the elements are equal or less than zero it will display the winner screen. At the moment, it displays the winner screen when the play button is clicked.

for (int i = 0; i <= parent.mygame.getplayer(parent.mygame.getpturn()).getmonsterarray().Length; i++)
{
    if (parent.mygame.getplayer(parent.mygame.getpturn()).getmonsterarray()[i].getMonHealth() <= 0)
    {
        Winner winners = new Winner();
        winners.Show();
        this.Hide();
    }
}

Upvotes: 0

Views: 121

Answers (4)

Evil Dog Pie
Evil Dog Pie

Reputation: 2320

Your code will display the winners screen if any one of the win conditions is met, not all of them. Try to compile the conditions into a single boolean value during the loop and then check that at the end (a logial AND operation).

Boolean gameIsWon = true;
foreach(var monster in parent.mygame.getplayer(parent.mygame.getpturn()).getmonsterarray())
{
    gameIsWon &= monster.getMonHealth() <= 0;
}
if(gameIsWon)
{
    Winner winners = new Winner();
    winners.Show();
    this.Hide();
}

Upvotes: 0

Jon Skeet
Jon Skeet

Reputation: 1503200

There are a few issues here:

  • You're calling parent.mygame.getplayer(parent.mygame.getpturn()).getmonsterarray() on every iteration of the loop, which is at least a bit inefficient, and also makes the code harder to read.
  • You should consider using a foreach loop unless you really need the value of i
  • Your current array loop uses <= where it should use < - an array of length 5 has elements 0, 1, 2, 3 and 4, for example.
  • You're showing the "winner" screen inside the loop for any player, not if all elements are less than 0.
  • All these get methods are unconventionally named (e.g. getplayer should be GetPlayer), and some should probably be properties or indexers anyway
  • This level of indirection violates the Law of Demeter very significantly. While I'm not dogmatic about this generally, I suspect you could make this a lot cleaner with a bit of work.

Solving just the array part, you could use:

bool anyHealthyMonsters = false;
// I've assumed the type of Monster here. You could use var, but you should understand
// what that means thoroughly first.
foreach (Monster monster in parent.mygame.getplayer(parent.mygame.getpturn())
                                         .getmonsterarray())
{
    if (monster.getMonHealth() > 0)
    {
        anyHealthyMonsters = true;
        break;
    }
}

if (!anyHealthyMonsters)
{
    Winner winners = new Winner();
    winners.Show();
    this.Hide();
}

A cleaner solution would be to use LINQ though:

if (parent.mygame.getplayer(parent.mygame.getpturn()).getmonsterarray()
        .All(monster => monster.getMonHealth() <= 0)
{
    Winner winners = new Winner();
    winners.Show();
    this.Hide();
}

However, if you're relatively new to C#, I would suggest concentrating on improving the design before you worry too much about this. I'd expect the final result to look something like:

if (parent.CurrentGame.CurrentPlayer.HasDefeatedAllMonsters())
{
    ...
}

where HasDefeatedAllMonsters() would be a method in your Player class, which would be something like:

return monsters.All(monster => monster.Health <= 0);

(Even the parent.CurrentGame.CurrentPlayer part makes me somewhat nervous, but it's hard to mentally redesign a whole app without more context. I strongly suspect that you shouldn't need to ask your "parent" (whatever that is) for the game - this class should know about it directly.)

Upvotes: 4

DrKoch
DrKoch

Reputation: 9782

Use an additional boolean var:

bool showWinnerScreeen = true;

Then check if all MonHealth are <0

for (int i = 0; i <= parent.mygame.getplayer(parent.mygame.getpturn()).getmonsterarray().Length; i++)
{
    if (parent.mygame.getplayer(parent.mygame.getpturn()).getmonsterarray()[i].getMonHealth() > 0)
    {
       showWinnerScreen = false;
    }
}

finnaly show your screen

if(showWinnerScreen)
{
    Winner winners = new Winner();
        winners.Show();
        this.Hide();
}

You may use LINQ and .Any() to do the same shorter, more advanced...

Upvotes: 0

Lennart
Lennart

Reputation: 10343

You can do this a bit cleaner with LINQ:

if(parent.mygame.getplayer(parent.mygame.getpturn()).getmonsterarray().All(m => m.getMonHealth() <= 0))
{
    Winner winners = new Winner();
    winners.Show();
    this.Hide();
}

All() will check if the condition (m => m.getMonHealth() <= 0) is satisfied for all objects in the array.

However, you should make sure that all objects below parent are actually initialized. That code will fail, if any of the lists (e.g. getmonsterarray()) return null.

Upvotes: 1

Related Questions