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 have a C# function like below, and in the else block, the function must return something. I don't know whether returning null is a good practice or not.

public Nullable<Point> GetSprite(int x, int y)
{
    if (x <= xCount && y <= yCount)
    {
        return new Point(x * spriteHeight, y * spriteWidth);
    }
    else
    {
        return null;
    }
}
share|improve this question
6  
How is the GetSprite method used? And what is it used for? –  Simon André Forsberg Mar 8 at 17:59
2  
Isn't Point a value type? You'd have to return a Nullable<Point> for a null return value to work.. –  Mat's Mug Mar 8 at 18:37
1  
Also the name is confusing, GetSprite doesn't return a "Sprite", but we'd need more context to recommend better alternatives. –  Mat's Mug Mar 8 at 18:40
    
@Mat'sMug I edited it. Yes it does not return a Sprite but it returns Sprite's coords. –  Cem Mar 8 at 20:53

5 Answers 5

up vote 10 down vote accepted

Yes, you could just write:

public Point GetSprite(int x, int y)
{
    if (x <= xCount && y <= xCount)
    {
        return new Point(x * spriteHeight, y * spriteWidth);
    }

    return null;
}

And write in the documentation it could return null if if (x <= xCount && y <= xCount)

So who use your code could just write

Point point = GetSprite(x, y);
if (point == null) { /* something went wrong, check variables! */ }
else { /* ok! go ahead! */ }

I found this syntax very useful, and could not lead to bugs if the programmer is alerted about the null.

And, anyway you don't need the else block since the return call will stop the function (except if there are any finally blocks, but it's not the case.).

share|improve this answer
3  
“could not lead to bugs if the programmer is alerted about the null” How do you make sure they are alerted? Because people don't always read documentation. –  svick Mar 8 at 18:41
3  
@svick You could throw an error if it's exceptional behavior. if (...) { ... } else { thrown new InvalidArgumentException; } –  WernerCD Mar 8 at 19:24

It depends whether arguments that are out of range may occur in a regular way or if such method calls are considered being abusive.

  • If such arguments are regular, then return null (that's perfectly okay).

  • If such arguments are abusive, then throw an exception.

An exception should always catch an error and never be misused for a test.

If the purpose of the method is not only to return a sprite, but also to test whether a sprite is available for a set of parameters, then you could use the same pattern as with Dictionary<T>.TryGetValue:

public bool TryGetSprite(int x, int y, out Sprite sprite)
{
    ...
}

Use it like this:

Sprite sprite;
if (obj.TryGetSprite(x, y, out sprite) {
    // Sprite available
} else {
    // No sprite available
}

If the result to be returned can be a non-nullable type and a magic number like 0, -1 or Int32.MinValue is not obvious or not available then this a good option. Even null might be a regular result under certain circumstances; for instance, a dictionary is allowed to contain null values. If you need this distinction between contains null and not found, the TryGet... approach is perfect.

The advantage over returning null is that you don't need additional knowledge apart from the method's signature in order to know how it works. Otherwise you are not sure whether an exception will be thrown or whether null or even an empty Sprite will be returned without reading the documentation. The disadvantage of TryGetSomething is that is looks somewhat cumbersome.

share|improve this answer
1  
I'd say that values like MinValue or -1 are very rarely obvious. –  svick Mar 8 at 18:43
    
Yes they should be avoided, but even Microsoft does it with System.String.IndexOf(). If used, at least a constant should be declared for magical numbers: public const int NotFound = -1; –  Olivier Jacot-Descombes Mar 8 at 18:48

Null is bad. It's a flaw in the type system, and its inventor deeply regrets unleashing it upon the world. It adds an additional value to all reference types that you probably don't want.

If you don't want to throw an exception (because failing to return a Sprite is a common occurrence and will likely be handled immediately rather than several layers up), then the correct solution is to return a type that represents a value of type T that may or may not be there. For value types, Nullable fills that need reasonably well, because it forces you to handle the case where the value is missing. For reference types you'll have to roll your own since C# doesn't provide any.

share|improve this answer
    
You probably don't want that additional value, unless you do. That's why C# has Nullable for structs and why F# has option. –  svick Mar 9 at 3:58
    
@svick The difference is that Nullable and Option keep the "nothing" value out of the original type. A Nullable int is not an int and you can't blindly assign one to the other. null is not an integer and it would make no sense to be able to assign it to an integer variable. The same logic holds for every other type - null is not a List, or a Point, or anything at all. –  Doval Mar 9 at 16:31

Returning null is fine. You can check for null and decide what to do in the calling function.

Multiple return statement in a function is not a good practise.

So you can modify your function like:

public Point GetSprite(int x, int y)
{
    Point pObj = null;
    if (x <= xCount && y <= xCount)
    {
        pObj = new Point(x * spriteHeight, y * spriteWidth);
    }

    return pObj ;
}
share|improve this answer
    
Thank you for extra information. –  Cem Mar 8 at 17:40
6  
Multiple return statement in a function is not a good practise” – is this some official C# style guideline? It is universally accepted that multiple returns are a bad idea in C, because of resource cleanup on exit. However, C# is garbage-collected, so there is no technical reason to avoid multiple returns. In fact, I find multiple returns more elegant than using variables, especially for such a short function! –  amon Mar 8 at 17:46
    
I disagree with you @amon but I also agree. yes it is a short function, but this is clear about what the code is doing, it is returning a Point object. if you have a return null; then you wonder is it returning a null integer, a null pointer, a null coordinate. I know you classify the function as a Point Function but this code is clear as to what it is doing, Returning a Point object. –  Malachi Mar 8 at 18:43
3  
@Malachi I think that return null is very clear, and when you're reading a short enough function, it's hard to forget its return type. On the other hand, return pObj is less clear (even if the variable name was improved), because you have to look though the whole function to find out what it really means. (And I think finding all returns is easier than finding all usages of a variable.) –  svick Mar 8 at 18:48

I've read alot about returning null is not the best practice in the world, but like anything I think it will depend on it's usage and how often it might be called. So, in your case this might be ok. However, I'd just like to chuck out another way of doing things using the Null Object pattern.

Using this you would not return null at all but rather return a Null/invalid object. Your calling code dependant on it's usage will be checking an instance type and so you might not need to even worry about whether it's null and potentially remove all those == null checks.

public Point GetSprite(int x, int y)
{
    return x <= xCount && y <= xCount
        ? new Point(x * spriteHeight, y * spriteWidth);
        : Point.OffGrid();
}

Then in your Point class itself you might have something like

public class Point
{
    private static readonly Point _null = new Point(-9999,-9999);

    public static Point OffGrid
    {
        get
        {
            return _null;
        }
    }

    public bool IsOffGrid {
        get { return this.Equals(OffGrid); }
    }
}

This way you might never even need to check for null at times. However if you did your statements might look something like this now

var point = GetSprite(x, y);

if(point.IsOffGrid()) // equivalent to point == null
{
   // how do we handle it when it's off grid?
}

// do stuff
share|improve this answer

protected by rolfl Mar 9 at 20:11

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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