14

For instance, along the lines of:

public bool Intersect (Ray ray, out float distance, out Vector3 normal)
{

}

vs

public IntersectResult Intersect (Ray ray)
{

}

public class IntersectResult
{
    public bool Intersects {get;set;}
    public float Distance {get;set;}
    public Vector3 Normal {get;set;}
}

Which is better both for clarity, ease of use, and most importantly performance.

Joan Venge
  • 315,713
  • 212
  • 479
  • 689

8 Answers8

16

I would use a combined type and I'll tell you why: because computation of a value should return the value, not mutate a bunch of variables. Mutating a bunch of variables doesn't scale once you need more than one of them mutated. Suppose you want a thousand of these things:

IEnumerable<Ray> rays = GetAThousandRays();
var intersections = from ray in rays 
                    where Intersect(ray, out distance, out normal)
                    orderby distance ...

Executing the query is now repeatedly mutating the same two variables. You're ordering based on a value that is being mutated. This is a mess. Don't make queries that mutate things; it is very confusing.

What you want is:

var intersections = from ray in rays 
                    let intersection = Intersect(ray)
                    where intersection.Intersects
                    orderby intersection.Distance ...

No mutation; manipulate a sequence of values as values not as variables.

I also would be inclined to maybe get rid of that Boolean flag, and make the value an immutable struct:

// returns null if there is no intersection
Intersection? Intersect(Ray ray) { ... }

struct Intersection 
{
    public double Distance { get; private set; }
    public Vector3 Normal { get; private set; }
    public Intersection(double distance, Vector3 normal) : this()
    {
        this.Normal = normal;
        this.Distance = distance;
    }
} 
Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • Thanks Eric, I appreciate your wisdom. I will get rid of the boolean and make it nullable like you showed, will make the code much clear I think. – Joan Venge Mar 10 '11 at 22:47
  • Eric, why do you feel a `struct` is preferable to a `class` here? My impression has been that best practice is something along the lines of: "Start with a `class`, because they're easier to reason about, and then change to a `struct` only if its necessary for performance reasons" – MgSam Feb 27 '13 at 04:07
10

I would use a combined type.

With an object you can attach behaviour, and return an arbitrarily complex object. You may wish to refactor your method in the future, and change the return values. By wrapping them in a return object and adding behaviour to that object, this refactoring can become largely transparent.

It's tempting to use tuples and the like. However the refactoring effort becomes a headache after a while (I'm speaking from experience here, having just made this mistake again)

Brian Agnew
  • 268,207
  • 37
  • 334
  • 440
  • Tuples are nice too if you don't want to create a unique type...dont abuse them of course ;-) – Amir Mar 10 '11 at 22:33
6

It's best to return a combined type. At the very least your method signature is cleaner and there's less work for the programmer to do when calling it. EG: you don't have to declare and initialize variables, which clutters the calling code.

Chris Wenham
  • 23,679
  • 13
  • 59
  • 69
5

Some people say it's a matter of preference, but I think that returning a complex object is better not only for clarity, but for maintenance as well.

If you ever needed to add another bit of data as output, you would have to change the method signature in addition to adding the code for the extra response; not so with a complex output type. In addition, it's harder to mock (for unit testing) output parameters. It also seems to break the 'simple' philosophy - you're having to go through the trouble of getting the output parameters set up when you only need one piece of input. The strength of an OOP language is making types - go with that route in my opinion.

The performance difference between the two is negligible.

Tejs
  • 40,736
  • 10
  • 68
  • 86
3

You could use a Tuple instead of creating the IntersectResult class as an alternative.

Reference:

http://msdn.microsoft.com/en-us/library/system.tuple.aspx

However, I would definitely lean toward returning a complex type over out parameters.

Brandon Moretz
  • 7,512
  • 3
  • 33
  • 43
3

Just for the record: It's hard to find a good example of the magic trio (clarity, ease of use, and performance).

Obviously the second example provides the first two, while the first one provides better performance. Whether this is negligible, I do not know. Maybe run a benchmark test and find out.

What I can tell you is that if you make your result type a small-sized struct, you can still save some performance points, since allocating on the heap is more expensive than on the stack. Again, copying the data from a value type can outweigh the heap-allocation penalty, but maybe not if you keep it small enough. Furthermore, you probably want to make that type immutable.

Yam Marcovic
  • 7,953
  • 1
  • 28
  • 38
  • Also if IntersectResult was a struct, would it make a difference? Also if it wouldn't intersect the Normal would be empty just like the variable you declare using the out parameter. So shouldn't they be identical? – Joan Venge Mar 10 '11 at 21:43
  • 2
    Structs are allocated on the stack and are copied elsewhere when necessary. Allocating on the heap can be surprisingly slower than on the stack + copying. Also, since it looks like your type contains only value types, making it itself a value type would cause it to be allocated faster than if you would allocate each of its members separately. – Yam Marcovic Mar 10 '11 at 21:52
2

Option 2 is probably the best solution if it best captures your domain. In which case your new type will get used in many other places as it represents a logical entity.

Adam Straughan
  • 2,766
  • 3
  • 20
  • 27
1

That depends entirely on how coherent the concept of an IntersectResult is.

Technically there is no reason to prefer one over the other.

H H
  • 263,252
  • 30
  • 330
  • 514
  • Thanks Henk. What do you mean by "how coherent the concept of an IntersectResult is", like how likely it's to change in the future? – Joan Venge Mar 10 '11 at 21:23
  • If you have to define `IntersectResult` just for hthis method it probably is the wrong approach. If `IntersectResult` is an idea/class that is used elsewhere, then the `out` version is ludicrous. – H H Mar 10 '11 at 21:26