Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
        }
share|improve this question

2 Answers 2

up vote 6 down vote accepted

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>>());
}
share|improve this answer
    
Good god.. You're absolutely right, I feel so dumbfounded right now :D .. –  Overly Excessive May 2 at 15:43

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];
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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