Public Group

# Better way to write this code

This topic is 1768 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

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.

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

##### Share on other sites

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.

Edited by frazchaudhry

##### Share on other sites

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

Edited by frazchaudhry

##### Share on other sites
I would write it how boogyman19946 does.  Edited by Nypyren

##### Share on other sites

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.

Edited by aregee

##### Share on other sites

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;
}
}


##### Share on other sites

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. Edited by Nypyren

• ### What is your GameDev Story?

In 2019 we are celebrating 20 years of GameDev.net! Share your GameDev Story with us.

• 28
• 16
• 10
• 10
• 11
• ### Forum Statistics

• Total Topics
634111
• Total Posts
3015573
×