2

I have methods to read various sensors and process data. Within these methods I have other methods that send commands to a circuit via serial port (to get the sensor values). A communication error may occur and I was wondering if it is ever OK to "return" an exception? For example:

public double AverageSensorValues()
{
  try
  {
   ...
   double sensor1Value = SensorValue(1);
   double sensor2Value = SensorValue(2);
   ...
   }
   catch (Exception ex)
   {
      MessageBox.Show("Error in AverageSensorValues()");
   }
}

public double SensorValue(int sensorNum)
{
   try {

   // Send command to circuit to return value.
   string response = SendCommand(commandStringToGetValue);

   // Check to see if response is an error
   bool isError = ErrorReturned(response);

   if(isError)
      ProcessError(response);  // Throws exception.

   ... // Other things that could cause exceptions to be thrown.

   }
   catch (Exception ex)
   {
      throw new Exception("Error in SensorValue()", ex);
   }
}

public void ProcessError(string errorResponse)
{
   // Split string and get error parameters (#, input command, etc.)

   throw new Exception(String.Format("Error-{0}: See ...", errorNumber));  // Is this OK? More readable than "ER,84,DM,3L" for example.
}

Is this ever OK or is it considered "bad practice"?

Thanks!

EDIT

I read over the various responses and it looks like I am doing this completely wrong. I attempted to use the above as a quick example but it looks like I should have just posted the full details from the get-go. So here is a more detailed example of my situation:

public double[] GetHeightAtCoords(CoordClass[] coords)  // Get height measurement at various positions. Called after button click, results are displayed on UI.
{
   try  // Error could occur within one of these methods. If it does, not Program critical but it should notify user and not return any result.
   {
   for(int coordIndex = 0; coordIndex < coords.Length; coordIndex++)  // Cycle through each desired position.
   {
      ...
      currentCoords = GetCurrentCoords();   // Get current actuator position.
      ... //Update UI.
      MoveToCoords(coords[coordIndex]);   // Move actuator to position.
      currentCoords = GetCurrentCoords(); // Verify position.
      EngageBrake();   // Lock actuator in place.
      double height = GetHeight(); // Read sensor data.
      ReleaseBrake();   // Release brake.
      ...
   }
  }
  catch (Exception ex)
  {
     // Display in statusbar.
     statusBar1.Text = String.Format("Error in GetHeightAtCoords(): {0}", ex.Message);
  }

   ...
   return heights;  // Return position heights array.
}

public CoordClass GetCurrentCoords()   // Method to read positional encoder values.
{
   ...
   try
   {
   double xPosition = GetXEncoderValue();   // Return x-coord value.
   double yPosition = GetYEncoderValue();   // Return y-coord value.
   }
   catch (Exception ex)
   {
      throw new Exception("Error in GetCurrentCoords(): {0}", ex.Message);
   }
   ...
   return new CoordClass(xPosition, yPosition); // Return current coords.
}

public void MoveToCoords(CoordClass coord)   // Method to move actuators to desired positions.
{
   try
   {
   ... 
   currentCoords = GetCurrentCoords(); // See where actuators are now.
   ... // Setup movement parameters.
   MoveToX(coord.X);   // Move x-axis actuator to position.
   MoveToY(coord.Y);   // Move y-axis actuator to position.
   }
   catch (Exception ex)
   {
      throw new Exception("Error in MoveToCoords(): {0}", ex.Message);
   }
   ...
}

public double GetXEncoderValue()   // Method to return x-coord value.
{
   string getXCoordCommand = "SR,ML,01,1";   // Serial command to get x-coord.
   ...
   string controllerResponse = SendReceive(getXCoordCommand);  // Send command, get response command.

   if(!ResponseOK(controllerResponse))   // If the response doesn't match the "command OK" response (i.e. SR,ML,01,1,OK)...
   {
      if(IsErrorResponse(controllerResponse))  // See if response is an error response (e.g. command error, status error, parameter count error, etc.)
         // Some known error type occurred, cannot continue. Format error string (e.g. ER,SRML,61) to something more meaningful and report to user (e.g. Read X Value Error: Status error.).
         throw new Exception("Read X Value Error-{0}: {1}", errorNumber, (ErrorEnum)errorNumber);
      else
         // Something else went wrong, cannot continue. Report generic error (Read X Value Error.).
         throw new Exception("Read X Value Error.");
   }
   ...
}

// GetYEncoderValue(), MoveToX(), MoveToY(), GetHeight(), EngageBrake() and ReleaseBrake() follow the same format as EngageBrake().

Here was my logic, if...

Call order: GetHeightAtCoords() -> MoveToCoords() -> GetCurrentCoords() -> GetXEncoderValue(), error with controller response.

Throw new Exception within GetXEncoder(), catch in GetCurrentCoords() and re-throw new Exception, catch in MoveToCoords() and re-throw new Exception, catch in GetHeightAtCoords() and display message in status bar (message = "Error in GetHeightAtCoords() : Error in MoveToCoords() : Error in GetCurrentCoords() : Read X Value Error-6: Status Error").

Because GetXEncoder() can be called from multiple places within a method, I figured that if I let the original exception bubble all the way up it would be of little help to the user (e.g. "Error in GetHeightAtCoords() : Read X Value Error-6: Status Error", which time?). Take this example, which Read X Value failed? GetHeightAtCoords() -> MoveToCoords() -> GetCurrentCoords() -> GetXEncoderValue() OR GetHeightAtCoords() -> GetCurrentCoords() -> GetXEncoderValue() ?

Hopefully that is more clear :/

Is something like this ever done? How would you recommend I proceed? Thanks again Everyone for your input!

Jonathan Hall
  • 75,165
  • 16
  • 143
  • 189
john
  • 4,043
  • 7
  • 27
  • 39

6 Answers6

11

Making a method that always throws an exception is a bit bad smelling. It looks like that processes the error and then continues on. I would rather do it like this:

if(isError)
      throw MakeMeAnException(response);  
...
}

public Exception MakeMeAnException(string errorResponse)
{
   // Split string and get error parameters (#, input command, etc.)
   return new MyException(String.Format("Error-{0}: See ...", errorNumber)); 
}

That makes it very clear that the if (isError) consequence always throws; in your original version it is hard to see that it does that.

Also, the stack trace of an exception is set at the point where it is thrown, so this sets the stack trace to the point where the error is detected, not the point where the exception is constructed, which seems better.

There are plenty more bad practices in your code.

  • Do not throw a new Exception; define your own exception class and throw it. That way the caller can catch your exception specifically.

  • Do not catch every exception and then wrap it in a new exception and throw that. What the heck is the point of that? What if every method did that? Soon you'd have an exception wrapped two dozen levels deep and no ability to work out what the exception really means.

  • Do not catch every exception and then show a message box. First off, that is combining error handling mechanism code with user interface code; keep those two separate. Second, you are potentially reporting all kinds of exceptions here -- thread aborts and out of memory and whatever. Catch a specific exception and handle it; if you don't know how to recover from it, don't eat it.

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • note that the suggested better practice here is not to "return" an exception (where the quotation marks mean "pseudo-return") by throwing it inside a helper method, but actually to return it (honest-to-goodness, without the quotation marks) from the helper method! – phoog Oct 11 '11 at 02:36
  • Of course, if you're going to define your own exception class, you can probably avoid both "pseudo-return" and "return" in favor of just calling its constructor. Give it a constructor that takes "error parameters (#, input command, etc.)" and avoid parsing. – Brian Oct 11 '11 at 13:21
  • Thanks for the input Eric, I updated my post to better explain (I hope) what I am trying to accomplish. – john Oct 11 '11 at 16:30
  • I can see some uses for having routines that always throw, particularly as something to assign to a delegate. Though it probably wouldn't be useful to have a delegate that always throws, it would certainly seem reasonable to assign to a delegate an always-throw method in situations where the delegate shouldn't be invoked. – supercat Oct 11 '11 at 19:54
  • @supercat: The irritating thing about routines that always throw is that there is no "never" return type in the CLR, only "void". A "never" method would be known at compile time to always throw or go into an infinite loop, and the compiler could therefore be smart about doing flow analysis. – Eric Lippert Oct 11 '11 at 20:04
  • With regard to only catching exceptions one can recover from, that's nice in theory, but it seems there are a lot of cases where unpredictable exogenous exceptions may occur, most of which will have predictable consequences. For example, if a routine to read an IEnumerable into a data structure is given an IEnumerable that reads data from a network, and a TimeOutException occurs within that IEnumerable's MoveNext method, should the application unavoidably die? If not, by what means should the application determine whether it can continue? – supercat Oct 11 '11 at 20:05
  • @supercat: I agree with you; this situation is vexing. I do not want Java-style checked exceptions but it would be nice if the set of exceptions that are "typically" thrown to describe unexpected exogenous situations were documented so that you could have some confidence that you had actually handled them. It is very irritating to have to catch IO exceptions and security exceptions and this and that and the other thing, and never be sure if you got them all. I would rather library providers realize that *the user not having access to the file is unexceptional*, so don't throw. – Eric Lippert Oct 11 '11 at 20:08
  • @Eric Lippert: I'm not suggesting that always-throw routines should be called statically. Come to think of it, though, there are quite a few always-throw routines in the CLR which are called virtually, not even through delegates. A lot of IEnumerator.Reset() implementations, for example, will always throw NotSupportedException, will they not? – supercat Oct 11 '11 at 20:09
  • I'm not sure how much room there is in the framework to improve exception handling without breaking compatibility, but one thing I'd like to see would be an abstract ExceptionBase class, which would be throwable, with children Exception and EvilException. One could explicltly catch EvilException or types derived from it, but such exceptions would not be caught by "catch Exception ex". How badly would such a change break things? – supercat Oct 11 '11 at 20:27
  • @supercat: We have made a similar change but not represented it in the type system as you suggest. In CLR v4 there are certain exceptions like "thread abort" that you can catch, but you can't eat. If you try to, they just get re-thrown for you. The original exception hierarchy divided exceptions up into system exceptions and user exceptions, which turned out to be a bad idea; there's no situation in which you want to catch all system exceptions or all user exceptions. It would have been smarter to capture other aspects in the type system, as you describe. – Eric Lippert Oct 11 '11 at 21:39
  • @Eric Lippert: Given the exception system that exists, what should something like a IEnumerable do if a TimeoutException occurs? Returning an empty enumeration would falsely tell the caller that the remotely-stored enumeration was empty. Returning an IEnumerable that would throw InvalidOperationException at the first MoveNext would be a slightly smaller lie but still seems icky. Interfaces have to be able to throw exogenous exceptions, so the question is what to do about them. – supercat Oct 13 '11 at 17:06
  • @Eric Lippert: Also, would there be any possibility of allowing user extension of the 'hard to catch' exception concept? My thought would be to define an IExceptionNeedingResolution interface with boolean property IsResolved. Throwing an exception that implements that interface would add it to a thread-static list. At the end of any catch block, all exceptions on the list would have IsResolved called; ones that return true would be deleted. If any exceptions survive and no other exception is pending, throw an UnresolvedExceptionException. – supercat Oct 13 '11 at 17:20
  • @Eric Lippert: I would think the run-time cost of such an approach should be fairly modest, at least in code which doesn't use exceptions excessively. If code catches an exception which implements IExceptionNeedingResolution and its IsResolved reports true, the code could figure the problem has been dealt with (e.g. the object known to be corrupt has been Disposed), without having to worry about breaking the rest of the system. Does that sound like a good mechanism? – supercat Oct 13 '11 at 17:25
1

This is bad because you are hiding the type of the exception:

catch (Exception ex)
{
    throw new Exception("Error in SensorValue()", ex);
}

Better would be to simply omit the try { ... } catch { ... }. Only catch exceptions that you can do something useful with.

This is bad because you are catching everything, but not showing what the actual error is:

catch (Exception ex)
{
    MessageBox.Show("Error in AverageSensorValues()");
}

You may have caught an OutOfMemoryException, a NullReferenceException or something else entirely. Your users won't know. At least show the exception message.

throw new Exception(String.Format("Error-{0}: See ...", errorNumber));

That's a reasonable use of exceptions - changing an error code to a exception. But you should probably use a more specific type than Exception - perhaps even a custom class deriving from Exception.

Mark Byers
  • 811,555
  • 193
  • 1,581
  • 1,452
  • For the `MessageBox.Show` case I actually update the status bar with "Error in AverageSensorValues(): 'inner exception details'". I was just lazy. :( I added an Edit to the post to further explain what I am trying to do. Thanks for your input! – john Oct 10 '11 at 22:29
  • Ignore above, I'll try to re-edit my post tomorrow morning to make it more coherent. I just attempted it but noticed my caffeine levels are critically low. – john Oct 10 '11 at 22:53
  • 1
    Oftentimes one should change the type of an exception, particularly when crossing between application layers. If a TimeOutException is thrown in the execution of an inner-layer method, an outer layer would have no way of knowing whether that represents a recoverable or unrecoverable situation unless the inner layer tells it; the cleanest way to do that is to have the inner layer catch the TimeoutException and rethrow as a type defined in its contract with the outer layer as being either recoverable or unrecoverable. – supercat Oct 11 '11 at 19:45
1

There is no point in using an exception like that, exceptions should be for when something unexpected happened. They should NOT be part of the general control flow.

Porco
  • 4,163
  • 3
  • 22
  • 25
0

If the only cause is hardware malfunction - yes, its OK to throw an exception saying 'hardware malfunction.'.

If its users fault, you should rather say 'give me good input, try again.'.

Mark Segal
  • 5,427
  • 4
  • 31
  • 69
0

C# Ever OK to Return an Exception?

Yes, but only a few reasons to throw an exceptions. For example if the execution meets a condition that should never happen.

Look at here which discuss when to throw an exception.

Because they're things that will happen normally. Exceptions are not control flow mechanisms. Users often get passwords wrong, it's not an exceptional case. Exceptions should be a truly rare thing, UserHasDiedAtKeyboard type situations

Get back to your code.

Displaying the actual message of the exception makes more sense.

 catch (Exception ex)
   {
      MessageBox.Show(string.Format("Error in AverageSensorValues() - {0}", ex.Message));
   }

Below code does not look right. Return your own error (or result) object rather than throwing exceptions unnecessarily. 

public void ProcessError(string errorResponse)
{
   // Split string and get error parameters (#, input command, etc.)

   throw new Exception(String.Format("Error-{0}: See ...", errorNumber));  // Is this OK? More readable than "ER,84,DM,3L" for example.
}
Community
  • 1
  • 1
CharithJ
  • 46,289
  • 20
  • 116
  • 131
0

IMHO, exceptions should be used in circumstances that cause your program to no longer function or that the program can not handle.

If it is something you can recover from, then no.

however, you should catch any exceptions and log them appropriately so that you can debug any problems.

NotMe
  • 87,343
  • 27
  • 171
  • 245