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;

HScottH, sorry for down voting you, I meant to up vote you, but my fat fingers pressed in wrong placed.

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.

The second return statement is necessary because the compiler can't just assume the first return statement will execute. There is no guarantee that a node in the list matches the argument key.

Therefore, the return statement in question should return a value that one would expect if there was no entry matching the given key. If you can't return null, you could write it like this:

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

But you won't be able to tell if you actually found a valid set of Coordinates unless you save that state somewhere. The above code is pretty much the same as yourself, with the exception that it only creates a new Coordinates at the last minute and only when needed. Not a great improvement, but some start.

I don't know C# very well. I thought it would have a reference system more similar to Java, but in Java you can return a null value for just about anything that's not a primitive.

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 would write it how boogyman19946 does.

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.

Anything that you can foreach is IEnumerable, so it will definitely work.

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

This is similar to what I was going to write (above), but then I noticed that nodes in the collection contain the .Coordinates member, making First/FirstOrDefault not what you'd want (because you'd have to null check the node you get from that, then get .Coordinates from it, which ends up being one step more complex than doing the loop yourself). So I edited my post to agree with boogyman since that's exactly what I would write instead.