5
\$\begingroup\$

I've written a recursive method for fetching grid neighbours in a symmetrical 2-dimensional grid, the problem is that I've found myself duplicating the code more or less into an overload to prevent my list from re-initializing everytime the method calls on itself.

I'm guessing there are ways to get around this that doesn't involve code duplication. Any help is appreciated.

public List<Cell<int>> GetNeighbours(Cell<int> cell, Direction direction, int neighboursToGet)
        {
            var neighbours = new List<Cell<int>>();

            if (neighboursToGet > 0)
            {
                int x = 0;
                int y = 0;
                Cell<int> neighbour;

                switch (direction)
                {
                    case Direction.Left:
                        x = -1;
                        break;
                    case Direction.Right:
                        x = 1;
                        break;
                    case Direction.Up:
                        y = -1;
                        break;
                    case Direction.Down:
                        y = 1;
                        break;
                }

                neighbour = cells[cell.Position.Row + y, cell.Position.Column + x];
                neighbours.Add(neighbour);
                GetNeighbours(neighbour, direction, neighboursToGet - 1, neighbours);
            }             
            return neighbours;
        }

        public List<Cell<int>> GetNeighbours(Cell<int> cell, Direction direction, int neighboursToGet, List<Cell<int>> neighbours)
        {
            if (neighboursToGet > 0)
            {
                int x = 0;
                int y = 0;
                Cell<int> neighbour;

                switch (direction)
                {
                    case Direction.Left:
                        x = -1;
                        break;
                    case Direction.Right:
                        x = 1;
                        break;
                    case Direction.Up:
                        y = -1;
                        break;
                    case Direction.Down:
                        y = 1;
                        break;
                }

                neighbour = cells[cell.Position.Row + y, cell.Position.Column + x];
                neighbours.Add(neighbour);
                GetNeighbours(neighbour, direction, neighboursToGet - 1, neighbours);
            }
            return neighbours;
        }
\$\endgroup\$
6
\$\begingroup\$

It seems to me that most of the body of the first GetNeighbours is extraneous. Wouldn't this be enough ?

public List<Cell<int>> GetNeighbours(Cell<int> cell, Direction direction, int neighboursToGet)
{
    var neighbours = new List<Cell<int>>();

    GetNeighbours(cell, direction, neighboursToGet, neighbours);

    return neighbours;
}

Also, it is redundant to accept neighbours as a read/write parameter and to return it. However, if you are going to do it, you could as well write:

public List<Cell<int>> GetNeighbours(Cell<int> cell, Direction direction, int neighboursToGet)
{
    return GetNeighbours(cell, direction, neighboursToGet, new List<Cell<int>>());
}
\$\endgroup\$
  • \$\begingroup\$ Good god.. You're absolutely right, I feel so dumbfounded right now :D .. \$\endgroup\$ – Overly Excessive May 2 '14 at 15:43
5
\$\begingroup\$

As a first step, I'd change this implementation:

public List<Cell<int>> GetNeighbours(Cell<int> cell, 
                                     Direction direction, 
                                     int neighboursToGet)

To call into this one:

public List<Cell<int>> GetNeighbours(Cell<int> cell, 
                                     Direction direction, 
                                     int neighboursToGet, 
                                     List<Cell<int>> neighbours)

But @Zoyd was faster to spot it ;)

Bottom line is, when you find yourself hitting Ctrl+C, a big shiny red flag should be automatically raised in your mind before you even have time to hit Ctrl+V - if you're copying code, you're doing it wrong.


I like that you're using an enum for your Direction. However you could also tackle the switch like this:

var directionalOffsets = new Dictionary<Direction, Func<Point, Point>>
    {
        { Direction.Left, point => new Point(point.X - 1, point.Y) },
        { Direction.Right, point => new Point(point.X + 1, point.Y) },
        { Direction.Up, point => new Point(point.X, point.Y - 1) },
        { Direction.Down, point => new Point(point.X, point.Y + 1) }
    };

Which, assuming cell.Position can be refactored into a Point, means if (neighboursToget > 0)...

var position = directionalOffsets[direction](cell.Position);

Then neighbour could be retrieved like this:

var neighbour = cells[position.Y, position.X];

Now it looks like cells has its dimensions reversed. I'd be more instinctively expecting this:

var neighbour = cells[position.X, position.Y];
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.