7

I recently came across a piece of code something like this (roughly C#):

public bool ValidateStuff(ref ArrayList listOfErrors, Stuff thingsToValidate)
{
    if (!thingsToValidate.isValid() )
    {
        errors.add("New error!");
    }
}

ArrayList errors = [];
bool valid = ValidateStuff(ref errors, stuffToValidate);

The key thing is the "ref" keyword - there is no need to use that keyword given the functionality here, and when I asked why it was there I was told that it had only been added as a kind of warning that the errors parameter is manipulated by the method, it's not purely used as input.

This immediately seemed wrong to me (ref has a purpose, and here it is not being used according to its purpose), but the idea of forcing (since you have to use ref when calling the method) a reminder of slightly counter-intuitive usage seemed reasonable, and I couldn't immediately think of a better way, apart from liberal use of comments. What do you think?

Extra credit: how could this method be better? Perhaps by simply returning a new List of errors which is added to the main List in the calling code? By providing two methods, one for the test and one for the errors? Or is it fine as it is?

2
  • 1
    This is wrong. The ref keyword has a specific purpose and this is not it. If you want to jolt callers our of their slumber, find another way. A 100 character function name springs to mind -- just as wrong.
    – david.pfx
    Sep 8 '14 at 13:03
  • Thank you for asking this question. I was told to do this in code review at my first job out of college by a veteran programmer, I am very receptive to code reviews and also to the reviewer, but I knew this was wrong. I was, however, required to comply. Dec 30 '16 at 14:02
8

Understand that the ref keyword signifies an intent to reassign the pointer.

If you enable Microsoft's code analysis, you will get a warning telling you that using ref param is likely a bad design.

Passing by ref is not just a 'visual' or syntactic sugar change. It actually allows the method to reassign the reference to a new object, which affects the calling code in ways that did not intend.

For example, without using ref, if the method tries to re-assign the paramter variable to a new object, it will NOT have any impact on the calling code's copy of the variable.

public bool ValidateStuff(ArrayList listOfErrors, Stuff thingsToValidate)
{
    if (!thingsToValidate.isValid() )
    {
        listOfErrors = new ArrayList(){"error"};
    }
}

ArrayList errors = ["existing error1", "existing error2"];
bool valid = ValidateStuff(errors, stuffToValidate);
Console.WriteLine(errors);  //output is still "existing error1", "existing error2"

But the same thing using ref, actually changes the reference outside of the method:

public bool ValidateStuff(ref ArrayList listOfErrors, Stuff thingsToValidate)
{
    if (!thingsToValidate.isValid() )
    {
        listOfErrors = new ArrayList(){"error"};
    }
}

ArrayList errors = ["existing error1", "existing error2"];
bool valid = ValidateStuff(ref errors, stuffToValidate);
Console.WriteLine(errors);  //output is now "error"

Now, in both of these pseudo code examples demonstrate a bug in the code. The important difference is that, the first version is likely to cause a predictable error. The second example could cause very unpredictable results in certain situations.

What to do instead

Now that you've (hopefully) accepted the fact that the language designers intended for ref to only be used when the method intends to reassign the pointer, you have only a few options:

  1. Redesign your method to return the error list instead of being a void.
  2. Add a new class containing everything that needs to be returned, and return a new instance of that class.
  3. Pass multiple reference-type parameters (without using the ref keyword) and let the method modify them as necessary.

Beyond that, there's nothing else you can do. The language was designed to be this way. Don't drive yourself crazy over the fact that C# doesn't allow multiple return values. I heard Go supports multiple return values if you can't accept it :D

4
  • 1
    what if he wanted his method to return 2 lists, how would you suggest changing the signature then? (I ask as its easy to say 'just do x' avoiding the intent of the question), BTW his method returns a bool, I assume that's an important return value in the real code, not the cut-down sample posted here.
    – gbjbaanb
    Sep 6 '14 at 13:14
  • "opens the door for programmers to make dangerous mistakes that can cause very nasty bugs that are difficult to track down." That is unavoidable no matter what you are doing. Sep 6 '14 at 17:41
  • @whatsisname Yea I might have been sensationalizing there. That is now removed from the answer.
    – Dan Ling
    Sep 12 '14 at 14:04
  • @gbjbaanb Thanks, I added more info to hopefully answer the question better
    – Dan Ling
    Sep 12 '14 at 14:06
4

Ugh. Always impressive when humanity shows how creatively terrible it can be...

Yes, the proper approach here is to return an IEnumerable of whatever your error type is (unless you're just using exceptions, in which case AggregateException exists). If you're doing anything remotely fancy with the errors, a special object that has a pass/fail property and is or exposes an IEnumerable of error type is the next step.

8
  • 2
    Rather pedantic answer, IMHO. It's not the point that the element is containing 'errors'. The point is that the collection is manipulated in the code. Sep 6 '14 at 12:07
  • @fransbouma - I certainly didn't mean the answer to be error specific. It is against best practice to modify the contents of a parameter in a function regardless of what it is. The fix depends on what it is, though this general approach should hold for most.
    – Telastyn
    Sep 6 '14 at 13:36
  • 1
    I wouldn't be so bold as to say "the proper approach" for such a vague hypothetical situation. Proper approach will depend on many details of the actual situation. Sep 6 '14 at 17:43
  • @whatsisname What kind of situation justifies using inputs as outputs?
    – Doval
    Sep 12 '14 at 14:52
  • @Doval: the most common is when dealing with interop. Sep 12 '14 at 16:27
3

In the early days of .NET and my endeavors into .NET land I used the same mechanism, i.e. to signal callers that the collection / container object they passed to the method was changed inside the method. But after a while I realized what is better worded in Dan Ling's answer above, namely that this is actually wrong: ref has a different meaning, and it actually illustrates something else, namely that the collection passed in can be a completely different object when the method returns.

It's better to document using XML document tags (that's what they're for!) what the behavior is of the method than signaling it through usage of 'ref'.

1
  • yes, use comments. Comments are good and shows that "self-documenting" code can be a bad thing when attempted like this.
    – gbjbaanb
    Sep 6 '14 at 13:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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