6

I wrote a FillSomeData(ParameterData param, Result res) method, which populates res with some data (just like it says on the label).

Result is a class, not a struct, so FillSomeData actually affects res.

Following a code review, my colleagues insist that I add a ref to the method signature (changing it to FillSomeData(ParameterData param, ref Result res)), merely in order to make it clear that the method changes res.

I believe this is not the intention of the ref keyword, and that this is wrong.

What is the common and recommended practice in such situations?

Improve this question
4
  • 5
    You're right, it's a dumb approach, because ref means changing the variable passed in and not mutating a reference type.
    – CodesInChaos
    Jun 4, 2017 at 11:30
  • 11
    Stop taking advice from those colleagues. That said, taking a parameter which is intended to be modified to indicate a result is a bad practice. Redesign your method so that it either (1) returns a value, or (2) produces a side effect, but NOT BOTH. Use exceptions in the latter case to indicate a failure.
    – Eric Lippert
    Jun 4, 2017 at 20:42
  • 1
    Keep in mind that your method isn't changing res at all. It is changing the contents of the object that res points to, so it keeps its previous value (a memory address to an specific object of type "Result") no matter what you do inside the method. It's a completely different matter. Your colleagues probably shouldn't be allowed near C# code until they grasp the difference.
    – T. Sar
    Jun 5, 2017 at 12:32
  • I don't think the coworkers in question need to be written off entirely. They are trying to clarify that the method causes a side effect that is best avoided, but just sort of part of the OOP world. I agree with Doc Brown that the best approach is the just try to make the method name as clear as possible though.
    – Graham
    Jun 5, 2017 at 13:30

1 Answer 1

Reset to default
25

No.

For anyone who has understood what the ref keyword means, this obfuscates what the method really does. The better alternative is to pick a more descriptive name for such a method like

 FillSomeDataIntoResult(ParameterData param, Result res)

You can also try to avoid the usage of ref at all, by changing your code in a way it will allow to write

 Result res=CreateResult(param)

or

 res.SomeData=CreateSomeData(param)

or 

 res.InitSomeData(CreateSomeData(param))

Of course, to make the latter a possible solution, one might need to restructure the fill code and/or the Result class, and sometimes that does not seem to be worth it.

Improve this answer
3
  • 1
    Correct. Also, I believe for the OPs use case, the correct keyword would actually be out in any case, which would be likely used as public bool FillSomeDataIntoResult(ParameterData data, out Result res){}, so that it's a 'true' return value if res was actually filled/modified. I don't think that's a great API, but it's what the out parameter keyword was intended for.
    – Paul
    Jun 4, 2017 at 19:45
  • 5
    @Paul: for the OPs use case, out would be even more wrong than ref, since his intention is to fill (or maybe partially fill) an existing object. With out that is simply not possible.
    – Doc Brown
    Jun 4, 2017 at 21:44
  • Ah! my mistake. I missed that it was updating an already initialized object. Good point.
    – Paul
    Jun 5, 2017 at 14:11

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.