Chutes & Ladders Board Generator (June 2016 Community Challenge)

This is an "entry" I made for the June 2016 Community Challenge to make a Chutes and Ladders generator. It generates a random number of chutes and ladders with a total delta of -50 and displays their start points, endpoints, and deltas.

Some notes:

• The number of chutes must be at least 2, and the number of ladders at least 1.

• The total delta from chutes must be (-)100 and the total delta from ladders must be 50. This is how I balanced the board to a total delta of -50 while ensuring that there were a decent amount of chutes and ladders.

• I have attempted to use the MVP pattern.

ChutesAndLaddersBoardGenerator

using System;
using System.Windows.Forms;

namespace ChutesAndLaddersBoardGenerator
{
    internal static class ChutesAndLaddersBoardGenerator
    {
        private const int BoardSize = 10;

        [STAThread]
        private static void Main()
        {
            Application.EnableVisualStyles();
            Application.SetCompatibleTextRenderingDefault(false);
            var model = new Generator(BoardSize);
            var view = new ChutesAndLaddersBoardGeneratorView();
            new ChutesAndLaddersBoardGeneratorPresenter(view, model);
            Application.Run(view);
        }
    }
}

ChutesAndLaddersBoardGeneratorView

```
using System;
using System.Collections.Generic;
using System.Windows.Forms;

namespace ChutesAndLaddersBoardGenerator
{
internal partial class ChutesAndLaddersBoardGeneratorView : Form,
IChutesAndLaddersBoardGeneratorView
{
public ChutesAndLaddersBoardGeneratorView()
{
InitializeComponent();
BindComponent();
}

private void BindComponent()
{
generateBoardButton.Click += OnGenerateBoardButtonClick;
}

public event Action GenerateBoard;

private void OnGenerateBoardButtonClick(object sender, EventArgs e)
{
GenerateBoard?.Invoke();
}

public void LoadChutesAndLa

class Generator

internal Generator(int size)
  {
      if (size % 2 != 0)
      {
          throw new Exception("Board size must be even!");
      }
      else if (size < 2)
      {
          throw new Exception("Board size must be positive!");
      }
      this.size = size;
      random = new Random();
  }

The correct exception would be an ArgumentOutOfRangeException instead of an Exception. You should always throw the most suitable exception.

In addition you won't need an else if just two if will do.

The List GenerateChutes is doing too much. You should extract the generation of the deltas to a separate method. It will be more readable and easier to maintain and will remove the code duplication happening in GenerateLadders() and GenerateChutes().

I don't like the manipulation of the looping variable either. Try to find a way to do it differently.

Both points can be done like so

private int[] GenerateDelta(int deltaUpperLimit, int secondDeltaValue, int maxRandomValue)
{
    int[] deltas = new int[deltaUpperLimit + 1];
    deltas[0] = 0;
    deltas[1] = secondDeltaValue;

    for (var i = 2; i < deltas.Length; i++)
    {
        int nextDelta = random.Next(1, maxRandomValue);
        while (deltas.Contains(nextDelta))
        {
            nextDelta = random.Next(1, maxRandomValue);
        }
        deltas[i] = nextDelta;
    }

    Array.Sort(deltas);

    for (var i = 0; i < deltaUpperLimit; i++)
    {
        deltas[i] = deltas[i + 1] - deltas[i];
    }
    return deltas;
}

Some more code duplication in the methods generating ladders and chutes

for (var i = 0; i < numberOfLadders; i++)
{
    var startSquareNumber =
        random.Next(2, (size * size) - (deltas[i] + 1));
    var startSquare = GetSquare(board, startSquareNumber);
    var endSquare =
        GetSquare(board, startSquareNumber + deltas[i]);
    if (!startSquare.HasChuteOrLadder &&
        !endSquare.HasChuteOrLadder)
    {
        ladders.Add(new ChuteOrLadder
        {
            StartSquareNumber = startSquareNumber,
            EndSquareNumber = startSquareNumber + deltas[i]
        });
        board[startSquare.Coordinates.Item1,
            startSquare.Coordinates.Item2].HasChuteOrLadder = true;
        board[endSquare.Coordinates.Item1,
            endSquare.Coordinates.Item2].HasChuteOrLadder = true;
    }
    else
    {
        i--;
    }
}

which can be removed by having an (badly named) AddItem() method like so

private bool AddItem(int startSquareNumber, int delta)
{
    var startSquare = GetSquare(board, startSquareNumber);
    if (startSquare.HasChuteOrLadder)
    { 
        return false;
    }

    var endSquare = GetSquare(board, startSquareNumber + delta);
    if (endSquare.HasChuteOrLadder)
    {
        return false;
    }

    ladders.Add(new ChuteOrLadder
    {
        StartSquareNumber = startSquareNumber,
        EndSquareNumber = startSquareNumber + delta
    });
    board[startSquare.Coordinates.Item1,
        startSquare.Coordinates.Item2].HasChuteOrLadder = true;
    board[endSquare.Coordinates.Item1,
        endSquare.Coordinates.Item2].HasChuteOrLadder = true;

    return true;
}

Implementing these changes will lead to

private List GenerateChutes(BoardSquare[,] board)
{
    var chutes = new List();
    var numberOfChutes = random.Next(2, board.GetLength(0));

    int[] deltas = GenerateDelta(numberOfChutes, 0 - TotalChutesDelta, (0 - TotalChutesDelta) - 1);

    int sizeSquared = size * size;
    for (var i = 0; i  GenerateLadders(BoardSquare[,] board)
{
    var ladders = new List();
    var numberOfLadders = random.Next(1, board.GetLength(0));

    int[] deltas = GenerateDelta(numberOfLadders, TotalLaddersDelta, TotalLaddersDelta);

    int sizeSquared = size * size;
    for (var i = 0; i < numberOfLadders; i++)
    {
        var randomMaxValue = sizeSquared - deltas[i] + 1;
        var startSquareNumber = random.Next(2, randomMaxValue);

        while (!AddItem(startSquareNumber, deltas[i]))
        {
            startSquareNumber =
            random.Next(2, randomMaxValue);
        }
    }
}

Why do you use a Tuple which is hard to read if accessing the values instead of having a simple small struct/class Position/Coordinate having only Row, Col or X,Y as variables/properties ? This would improve the readability of e.g

board[startSquare.Coordinates.Item1, startSquare.Coordinates.Item2].HasChuteOrLadder = true;
board[endSquare.Coordinates.Item1, endSquare.Coordinates.Item2].HasChuteOrLadder = true;

internal class ChuteOrLadder

Seeing an Or inside a class name usually indicates that that class has too many responsibilities which isn't the case here. I would suggest to rename this class to something else and then have

internal class Chute : RenamedChuteOrLadder
{

}
internal class Ladder : RenamedChuteOrLadder
{

}

to make it more clear with what you are dealing. This would involve some bigg

StackExchange Code Review Q#131437, answer score: 8