Is it necessarily bad style to ignore the return value of a method

Question

Let's say I have a C# method

public void CheckXYZ(int xyz) {
  // do some operation with side effects
}

Elsewhere in the same class is another method

public int GetCheckedXYZ(int xyz) {
  int abc;
  // functionally equivalent operation to CheckXYZ, 
  // with additional side effect of assigning a value to abc
  return abc; // this value is calculated during the check above
}

Is it necessarily bad style to refactor this by removing the CheckXYZ method, and replacing all existing CheckXYZ() calls with GetCheckedXYZ(), ignoring the return value? The returned type isn't IDisposable in this case. Does it come down to discretion?

EDIT: After all the responses, I've expanded the example a little. (Yes, I realise it's now got out in it, it's especially for @Steven)

public void EnsureXYZ(int xyz) {
  if (!cache.ContainsKey(xyz))
    cache.Add(xyz, random.Next());
}

public int AlwaysGetXYZ(int xyz) {
  int abc;
  if (!cache.TryGetValue(xyz, out abc))
  {
    abc = random.Next();
    cache.Add(xyz, abc);
  }
  return abc;
}

Answer 1

It entirely depends upon what that return value is telling you and if that is important to know or not. If the data returned by the method is not relevant to the code that is calling it then ignoring it is entirely valid. But if it indicates some failure/counter/influential value then ignore it at your peril.

Answer 2

Usually it's bad style, yes. It's allowed and ok where methods return an instance of the class for chaining (foo.bar().baz().xyz().asdf() => asdf returns the instance foo but you don't need it anymore)

In your case the point of bad style wouldn't be the ignored return value but the methods with side effects. A CheckXyz() function should always return a boolean and have no further side effects.

In general, side effects are bad and if you call a method and can ignore the returned value it means that the method/object/library/program might be poorly designed.

Answer 3

IMHO it's generally best to have one (and only one) way of doing things to avoid duplicating your codebase. Generally though, if you sometimes use and sometimes dont use the return value, it's probably a sign that your code could be broken down in a better way. In your example, it would probably be fine if both of those functions called a third (common) function to avoid duplicating the core fnuctionality.

Error codes should always be checked for and handled but if the function's just returning information then what you do with that information is up to you.

[Edit] ...and as dbemerlin points out, side effects should be avoided wherever possible.

Answer 4

you could work with out parameters as well.

Answer 5

A common C convention is to write this:

(void)GetCheckedXYZ();

The cast to void has no effect, but by convention it shows that the developer knows that the return value is being ignored, i.e. it shows it's deliberate.

C# won't let you do that, but I've seen this instead (Also in Java):

/*(void)*/GetCheckedXYZ();

Which some may feel lacks aesthetics, but it does convey the intent of the developer without resorting to alternative versions of methods, which to my eye seems worse.

Answer 6

A lot of these answers have good points. I'd just add that if you choose to ignore a return value, then comment it along the lines of "don't care about the return value because...", so that the next person who comes into the code will see that you have not missed it by accident and that you have thought things through

EDIT: better still, put your comment inside an empty block

if (!something()) {
// Not worried if this fails because blah
}