4
\$\begingroup\$

We have a dynamically created map with two dimensional coordinates. There's an object in this map on a random position and I need to validate the four adjacent fields. However, I need to leave out the corners since they are not relevant for the validation!

This is an illustration of what needs to be validated (the green fields saying "v"):

  • Instance-Example: Object ("O") is on (2;2)
  • Line above the object: Y-Coordinate is 1 (so, 2 - 1 )
  • Line above the object: Needed x-coordinate is 2 (so, 2 + 0 )
  • Line of the object: Y-Coordinate is 2 (so, 2 + 0 )
  • Line of the object: Needed x-coordinate is 1 (so, 2 - 1 )
  • Line of the object: Needed x-coordinate is 3 (so, 2 + 1 )
  • Line beneath the object: Y-Coordinate is 3 (so, 2 + 1 )
  • Line beneath the object: Needed x-coordinate is 2 (so, 2 + 0 )

Here is my solution. The core magic lays within the inner for loop.

public bool validateEdibility ()
{

    for (int line = -1; line < 2; line ++)
    {

        for (int column = Math.Abs(line) - 1; column < 2; column += 2)
        {

                if (!(this.MyWorld.getObjectOnPosition(this.Y + line, this.X + column) is Wall))
                {

                    return true;

                }

        }

    }

    return false;

}

The first idea behind this code is to get an integer which represents the subtrahend for the specific coordinate of the object. For example we need to have "-1" for the y-coordinate if we want to iterate through the first line (the line above the object).

With this idea we need to get a sequence like this:

  • Line: -1
    • Column: 0
  • Line: 0
    • Column: -1
    • Column: 1
  • Line: 1
    • Column: 0

The second idea is the horizontal for loop (the inner loop). We take the absolute of the line and subtract it with 1. This provides us the right first and last sequence since we've terminated the only difference between them: the negative sign.

Than we add +2 to the index for each run since the only line surviving this is the one in the middle ("0"). Remember: we subtract 1, so the 0-line starts with "-1".

I think this is quite a brainfuck and that's why I wonder if there's a more elegant way to solve this. Is the code evolving from these ideas somehow improvable?

\$\endgroup\$
0
10
\$\begingroup\$
  • This code is extremely hard to read and much longer than it needs to be.
  • Use documented naming conventions for C#.
  • Make small methods that do one thing well.

public bool IsWall(int x, int y)
{
  return this.MyWorld.GetObjectOnPosition(x, y) is Wall;
}
public bool IsValidForEditing()
{
  int x = this.X;
  int y = this.Y;
  return 
    !IsWall(x-1, y) || !IsWall(x, y-1) || 
    !IsWall(x+1, y) || !IsWall(x, y+1);
}

Anyone who knows even basic C# -- or any C-like language -- can read this code and immediately understand what it means. Your original code, not so much.

Any time you write a loop, ask yourself "would this be more clear without the loop?" Odds are good, yes. In this case, definitely.

\$\endgroup\$
4
\$\begingroup\$

The method name validateEditable doesn't convey the right idea to me. It seems to suggest that it might throw an exception if the validation fails. If it's going to return a bool, then IsEditable seems to be a more appropriate name.

You seem to be using Math.Abs() as a cryptic substitute for if-else. If you're going to do that, then a ternary expression would be slightly less mysterious.

for (int column = (line == 0) ? -1 : 0; column <= 1; column += 2)

But really, I don't think you should use nested loops to visit the four neighbours, let alone one with cryptic arithmetic tricks. If you make a list of the four neighbours, then you can just step through them. Better yet, don't use a loop at all — use .All().

private static readonly Tuple<int, int>[] Neighbors =
{
                    new Tuple<int, int>(-1, 0),
    new Tuple<int, int>(0, -1),          new Tuple<int, int>(0, +1),
                    new Tuple<int, int>(+1, 0)
};

public bool IsEditable()
{
    return !Neighbors.All(neigh =>
        this.MyWorld.getObjectOnPosition(this.Y + neigh.Item1, this.X + neigh.Item2) is Wall
    );
}
\$\endgroup\$
1
\$\begingroup\$

Why not just calculating the indices:

private IEnumerable<Point> GetAdjustIndeces(int xObj, int yObj)
{
    // top
    if (yObj > 0) yield return new Point(xObj, yObj -1);
    // bottom
    if (yObj < heightMax) yield return new Point(xObj, yObj + 1);
    // right
    if (xObj < widthMax) yield return new Point(xObj + 1, yObj);
    // left
    if (xObj > 0) yield return new Point(xObj - 1, yObj);
}

with:

public bool ValidateEdibility ()
{
    return GetAdjustIndeces(x, y).Any(p => !(MyWorld.getObjectOnPosition(p.X, p.Y) is Wall));
}
\$\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.