Ref Abuse: Worth Cleaning Up?

Question

I have inherited some code that uses the ref keyword extensively and unnecessarily. The original developer apparently feared objects would be cloned like primitive types if ref was not used, and did not bother to research the issue before writing 50k+ lines of code.

This, combined with other bad coding practices, has created some situations that are absurdly dangerous on the surface. For example:

---

Customer person = NextInLine(); 
//person is Alice
person.DataBackend.ChangeAddress(ref person, newAddress);
//person could now be Bob, Eve, or null

Could you imagine walking into a store to change your address, and walking out as an entirely different person?

---

Scary, but in practice the use of ref in this application seems harmlessly superfluous. I am having trouble justifying the extensive amount of time it would take to clean it up. To help sell the idea, I pose the following question:

How else can unnecessary use of ref be destructive?

I am especially concerned with maintenance. Plausible answers with examples are preferred.

You are also welcome to argue clean-up is not necessary.

Answer 1

I would say the biggest danger is if the parameter were set to null inside the function for some reason:

public void MakeNull(ref Customer person)
{
    // random code
    person = null;
    return;
}

Now, you're not just a different person, you've been erased from existence altogether!

As long as whoever is developing this application understands that:

By default, object references are passed by value.

and:

With the ref keyword, object references are passed by reference.

If the code works as expected now and your developers understand the difference, it's probably not worth the effort it's going to take remove them all.

Answer 2

I'll just add the worst use of the ref keyword I've ever seen, the method looked something like this:

public bool DoAction(ref Exception exception) {...}

Yup, you had to declare and pass an Exception reference in order to call the method, then check the method's return value to see if an exception had been caught and returned via the ref exception.

Answer 3

Can you work out why the originator of the code thought that they needed to have the parameter as a ref? Was it because they did update it and then removed the functionality or was it simply because they didn't understand c# at the time?

If you think the clean up is worth it, then go ahead with it - particularly if you have the time now. You might not be in a position to do fix it properly when a real issue does arise, as it will most likely be an urgent bug fix and you won't have the time to do a proper job.

Answer 4

It is quite common in C# to modify the values of arguments in methods since they usually are by value, and not by ref. This applies to both reference and value types; setting a reference to null for instance would change the original reference. This could lead to very strange and painful bugs when other developers work "as usual". Creating recursive methods with ref arguments is a no-go.

Apart from this, you have restrictions on what you can pass by ref. For instance, you cannot pass constant values, readonly fields, properties etc., so that a lot of helper variables are required when calling methods with ref arguments.

Last but not least the performance if likely not nearly as well, since it requires more indirections (a ref is just a reference which needs to be resolved on every access) and may also keep objects alive longer, since the references are not going out of scope as quickly.

Answer 5

I would try to fix it. Just perform a solution wide string replacement with regex and check the unit tests after that. I am aware that this might break the code. But how often do you use ref? Almost never, right? And given that the developer did not know how it works, I consider the chance that it is used somewhere (the way it should) even smaller. And if the code break - well, rollback ...

Answer 6

To me, smells like a C++ developer making unwarranted assumptions.

I'd be wary of making wholesale changes to something that works. (I'm assuming it works because you don't comment about it being broken, just about it being dangerous).

The last thing you want to do is to break something subtle and have to spend a week tracking down the problem.

I suggest you clean up as you go - one method at a time.

1. Find a method that uses ref where you're sure it isn't required.
2. Change the method signature and fix up the calls.
3. Test.
4. Repeat.

While the specific problems you have may be more severe than most cases, your situation is pretty common - having a large code base that doesn't comply with our current understanding of the "right way" to do things.

Wholesale "upgrades" often run into difficulties. Refactoring as you go - bringing things up to spec as you work on them - is much safer.

There's precedent here in the building industry. For example, electrical wiring in older (say, 19th century) buildings doesn't need to be touched unless there's a problem. When there is a problem, though, the new work has to be completed to modern standards.