Hi,

I wanted to know if there is a better way to write the code for this method that I am using in my project.

public Coordinates GetNodeCoordinates(int number)
        { 
            Coordinates coords = new Coordinates();
            foreach(Node node in nodes)
            {
                if (node.Number == number)
                {
                    coords = node.Coordinates;
                    return coords;
                }
            }
            return coords;   
        }

It basically goes through a 2 dimensional array and if the value in the array object matches the other value it returns a coordinate object. The way I have made my program is that it is impossible for the value being searched for to not be in the array. I do not want to have the second return statement after the foreach loop because I know it will never be called. But the compiler complains if I remove it. Can somebody tell me if there is any way to write this method so I don't have to write the second return statement.

I can't imagine why you care. If it's never executed, it's irrelavent.

But if you must: just replace the first return line with break;

Oh, and don't construct a wasted instance on this line:

Coordinates coords = new Coordinates();

Just set it to null:

Coordinates coords = null;

no you can't do that. Its a non-nullable value type. Also that instance construction must be there otherwise, the compiler will give an error on the second return statement complaining that I am using unassigned local variable. The compiler isn't sure whether my code will actually go inside the if statement or not.

The reason I care is because maybe somebody more experienced than I am might have a better way of going about this. Maybe I'll get to learn something.

well I could have it return a null value, in C# you can make even primitive values take on the null value, e.g

int? nullableInt = null;

and do something like this with my code

public Coordinates GetNodeCoordinates(int number)
        {
            Coordinates? coords = null;
            foreach(Node node in nodes)
            {
                if (node.Number == number)
                {
                    return node.Coordinates;
                }
            }
            return (Coordinates)coords;   
        }

But it looks just as ugly as my original method. Your improved way looks much better so I am going with that for now. Thanks Boogyman

I can't imagine why you care. If it's never executed, it's irrelavent.

This one is a classic. You have an area of your code that is unreachable. You think to yourself that "I'll just leave it". Then you get an obscure bug. After hours of debugging, you find that the code did indeed enter the area where it should be impossible to go. It happens. You can not always foresee all scenarios or bugs other places in code that may alter your logic.

I would:

1. Put a break, as HScottH said, or

2. At least put a log entry telling you that "I should never be here!" so you know when the impossible happens, you know where to look right away.

EDIT:

3. Consider if this is eligible for refactoring as this may indicate that there is a simpler way to change your code to avoid this situation altogether. It may not be, but it is worth looking into.

Linq might work for you if nodes is IEnumerable.

public Coordinates GetNodeCoordinates(int number)
{
     return nodes.FirstOrDefault(n=>n.Number==number)
}

If you want to use Nullable Coordinates

public Coordinates? GetNodeCoordinates(int number)
{
     return nodes.Cast<Coordinates?>().FirstOrDefault(n=>n.Number==number)
}

(I'd only use .Cast if it isn't being called often)

If a node with the correct number is more likely to be in nodes than not it might be quicker to do something like

public Coordinates? GetNodeCoordinates(int number)
{
  try{
    return nodes.First(n=>n.Number==number);
  } catch{
    return null;
  }
}

Linq might work for you if nodes is IEnumerable.

public Coordinates GetNodeCoordinates(int number)
{
     return nodes.FirstOrDefault(n=>n.Number==number)
}