none
Best Practice... Return Values RRS feed

  • Question

  • Hi

    In your opinion, what is the best way to go about returning a value? This probably seems like a stupid question (and it probably is a stupid question) but what do you others do when you want to return a value from a method?

    Consider the following...

    Public Function OpenFile(ByVal fileName As StringAs FileStream  
        If Not File.Exists(fileName) Then 
            Throw New ArgumentException("Specified file does not exist")  
        End If 
     
        Return File.Open(fileName, FileMode.Open)  
    End Function 

    And the equivalent...

    Public Function TryOpenFile(ByVal fileName As StringByRef fileStream As FileStream) As Boolean 
        If Not File.Exists(fileName) Then 
            Return False 
        End If 
     
        fileStream = File.Open(fileName, FileMode.Open)  
     
        If fileStream Is Nothing Then 
            Return False 
        End If 
     
        Return True 
    End Function 

    Granted, the code inside the methods is used as an example only but which way would be preferable? Generate an Exception as per the first example or use a ByRef parameter to pass out a value and use a return value to determine if the parameter contains valid data?

    Comments please

    Kev
    Thursday, February 5, 2009 2:42 PM

Answers

  • You shouldn't really throw an exception unless you have to. You first example throws an exception unnnecessarily: you have a boolean value indicating if the file exists, so why not use that?

    I'd probably do the latter and return the FileStream. If the file cannot be opened then return Nothing. However, there is a problem.

    The problem is this: if the consumer finds that the filestream object returned is nothing, what is the problem? There are dozens of reasons - the consumer has no knowledge of the contents of the routine, so it could be a bug, sloppy coding, or even no code, as well as a legitimate error (of which the consumer will never know). In which case, throwing a meaningful exception may be appropriate.

    There are pros and cons: true, delegating error handling responsibility to a client can be a legitimate, if lazy, argument. The issue is that the error may not be handled. In which case, the program will crash. Your routine caused the error, thus causing the crash.

    Your situation is rather contrived, however: I would assume that a routine does more than just return a filestream. For example, a user would want to call a routine to get the contents of a file in some meaningtful format. The user could care less about file existence, IO errors, security errors, and so on. It's a boolean response: either they have the information they want, or they don't. If they don't, they want a meaningful reason why. It's not the consumers job to deal with the reason This is where you can have a 'get last error' property if a method fails to return an appropriate object; this has fallen out of favor and sneered upon by OOP programmers but it works and is a functional coding pattern.

    Bottom line, only throw an error up to the caller if you have exhausted all possibilities of handling the error.




    Stephen J Whiteley
    • Marked as answer by Yichun Feng Tuesday, February 10, 2009 2:31 AM
    Thursday, February 5, 2009 5:12 PM
    Moderator
  • Two cents, since the thread requests them...  =)

    On one hand it is true that you should intentionally throw exceptions as little as possible - use them only as a last resort in normal coding.

    However, when creating methods on an object, there is nothing wrong with throwing an exception when the case warrants - that's why they are there.  Assuming that the OpenFile function in the example would do more work than File.Open(), throwing an exception on an error condition within this function is perfectly acceptable.  Particularly as in the example where a specific type of error is thrown with a descriptive message.

    Then on the other hand, a consumer of this method may find either functionality desirable depending on the situation.  So I would agree that in the ideal world, you would provide both methods when you know your code has a potential for failure.  We see this model in the Parse() and TryParse() methods of the numeric data types and there it makes sense - the string may or may not be known to be a number prior to parsing.


    Reed Kimble - "When you do things right, people won't be sure you've done anything at all"
    • Marked as answer by Yichun Feng Tuesday, February 10, 2009 2:31 AM
    Friday, February 6, 2009 6:07 AM
    Moderator
  • Corey Furman said:

    I-DotNET said:

    'air code
    Private Sub DoSomething()
        Try
        .....
        Catch ex As Exception
            Throw
        End Try
    End Sub

    or even worse:

    'air code
    Private Sub DoSomething()
        Try
        .....
        Catch ex As Exception
            Throw ex
        End Try
    End Sub


    Why is the second worse than the first?


    The first one is bad for two reasons. 

    First, you should not catch general exceptions.  Instead, you're supposed to catch specific exceptions such as FileNotFoundException or DivideByZeroException.  This is documented here:

    http://msdn.microsoft.com/en-us/library/ms182137.aspx

    The second reason (and I've never seen this documentented anywhere) is that it doesn't really do anything.  If all you're going to do with your exception handling is simply catch an exception and then throw it back up the callstack, you haven't doing anything useful.  The exception would get thrown up the callstack anyway.  IOW, you've written four 4 lines of code that don't do anything practical.

    I really wish Microsoft would document this somewhere.  It's briefly mentioned here, "Do not overuse catch. Exceptions should often be allowed to propagate up the call stack." but they need to provide more detail:

    http://msdn.microsoft.com/en-us/library/ms229005.aspx 

    The second one is bad for the above two reasons plus a third.  When an exception is first thrown, it keeps a record of the stack trace. The way the code is written in the second example, the original stack trace is lost.  So you should throw it without specificying the exception.  This is documented here:

    http://msdn.microsoft.com/en-us/library/ms182363.aspx

    Putting it all together, this (hopefully) is a good example of correct exception handling:

        'air code  
        Private Sub DoSomething()  
            Try 
                'do something that might cause a file not found exception  
            Catch ex As FileNotFoundException  
                MessageBox.Show("The file you specified was not found.  Please try again.", Application.ProductName, MessageBoxButtons.OK, MessageBoxIcon.Error)  
            End Try 
        End Sub 
     

    EDIT:  After I posted this above code, I realized that it didn't follow best practices for strings or for globalization.  I don't want to hijack Kevin's thread, so I created a new one here: 

    http://social.msdn.microsoft.com/Forums/en-US/vbgeneral/thread/d1e66274-5092-4734-8854-a5c47530046c
    • Marked as answer by Yichun Feng Tuesday, February 10, 2009 2:31 AM
    Friday, February 6, 2009 2:38 PM
  • I-DotNET said:

    ...

    You can also have a Try...Finally block without a Catch:

    ....

    True enough; I've felt this does go against the Microsoft 'principle' of not catching general exceptions, but always catch specific exceptions...this is just saying 'chuck all the exceptions away' :)

    Having said that, I've found the 'always catch specific exceptions' to be very much a guideline than a rule - that is, I rarely catch specific exceptions - right or wrong, but then I also rarely throw exceptions out of a class unless absolutely necessary as part of the design or is unavoidable.

    I do think error handling is actually one of the harder concepts to get to grips with. Summarily, error handling: 5 minutes to learn, a lifetime to master.

    Stephen J Whiteley
    • Marked as answer by Yichun Feng Tuesday, February 10, 2009 2:31 AM
    Friday, February 6, 2009 7:25 PM
    Moderator

All replies

  • If you can silently handle all possible exceptions from your "try" operation in your own code, and throwing errors would serve no useful purpose to the calling code then go for the second option.

    If, on the other hand, is is essential that the calling code receive any raised exception as a message describing why the error occurred, then the first option is the way to go.

    This leads me to the conclusion that the actual "best practice" is to provide both methods, allow the end-user to choose which type of function they want and document the functions accordingly, time-consuming though that may be.
    Thursday, February 5, 2009 2:51 PM
  • I try to have the least amount of exception in my logic because exception are slow. Exception should be throwned when something exceptional is hapenning, like the file exists but the user can't open it (security).

    Puzzles, brain teases, riddles, enigmas: http://www.toysforthebrain.com
    Thursday, February 5, 2009 3:49 PM
  • First, I believe the TryXXXX pattern is used for operations that are likely to fail.  So the question to ask yourself is "Do I think this operation is likely to fail?"  If not, there's no point in using the TryXXX pattern.

    Second, depending on the situation, I don't think there's anything wrong with delegating to the client (i.e. the code that's calling your code) the responsibility of data validation.  Perhaps the client knows for a fact that the file exists or maybe it already performed a File.Exists().  I write a lot of general purpose libraries and I typically leave data validation up to the client.  But you need to make this delegation of responsibility clear to both parties.

    Third, I realize that the code you posted is just an example, but I don't think there's anything wrong with letting File.Open() throw the exception.  IOW, I don't think that there's anything wrong with doing this:

    Public Function OpenFile(ByVal fileName As StringAs FileStream     
        Return File.Open(fileName, FileMode.Open)     
    End Function    
     

    Does it really matter whether File.Open() that throws the exception or OpenFile() that throws the exception?  If it doesn't, I'd let File.Open throw the exception because it's going to do it anyway.

    Fourth, I found this article from Microsoft which covers this topic:

    http://msdn.microsoft.com/en-us/library/ms229009.aspx

    However, I'm not sure I like their Tester-Doer pattern.  It performs the data validation twice.  In the example that they give, the data validation is a simple test for Nothing.  But if the data validation is expensive, do you really want to do it twice?
    Thursday, February 5, 2009 4:29 PM
  • You shouldn't really throw an exception unless you have to. You first example throws an exception unnnecessarily: you have a boolean value indicating if the file exists, so why not use that?

    I'd probably do the latter and return the FileStream. If the file cannot be opened then return Nothing. However, there is a problem.

    The problem is this: if the consumer finds that the filestream object returned is nothing, what is the problem? There are dozens of reasons - the consumer has no knowledge of the contents of the routine, so it could be a bug, sloppy coding, or even no code, as well as a legitimate error (of which the consumer will never know). In which case, throwing a meaningful exception may be appropriate.

    There are pros and cons: true, delegating error handling responsibility to a client can be a legitimate, if lazy, argument. The issue is that the error may not be handled. In which case, the program will crash. Your routine caused the error, thus causing the crash.

    Your situation is rather contrived, however: I would assume that a routine does more than just return a filestream. For example, a user would want to call a routine to get the contents of a file in some meaningtful format. The user could care less about file existence, IO errors, security errors, and so on. It's a boolean response: either they have the information they want, or they don't. If they don't, they want a meaningful reason why. It's not the consumers job to deal with the reason This is where you can have a 'get last error' property if a method fails to return an appropriate object; this has fallen out of favor and sneered upon by OOP programmers but it works and is a functional coding pattern.

    Bottom line, only throw an error up to the caller if you have exhausted all possibilities of handling the error.




    Stephen J Whiteley
    • Marked as answer by Yichun Feng Tuesday, February 10, 2009 2:31 AM
    Thursday, February 5, 2009 5:12 PM
    Moderator
  • > There are pros and cons: true, delegating error handling responsibility to a client can be
    > a legitimate, if lazy, argument. The issue is that the error may not be handled. In which
    > case, the program will crash. Your routine caused the error, thus causing the crash.

    Hey, did you just call me lazy?!   I'm frequently the client of my class libraries and I have excellent communication with myself. ;)

    In all seriousness, I also have general purpose libraries for data validation and exception handling.  I also created some really nice validator controls, much better than what .NET provides.  So I strongly believe in having good data validation and exception handling, it's just a question of where it goes.  

    > Bottom line, only throw an error up to the caller if you have exhausted all possibilities of handling the error.

    I've been meaning to write an article on this, but unless you know how to handle the error, it's usually best to let the exception propagate up the callstack.  Then create a global exception handler and let it catch the exception.

    It drives me nuts when I see developers do stupid ____ like this:

    'air code
    Private Sub DoSomething()
        Try
        .....
        Catch ex As Exception
            Throw
        End Try
    End Sub

    or even worse:

    'air code
    Private Sub DoSomething()
        Try
        .....
        Catch ex As Exception
            Throw ex
        End Try
    End Sub
    Thursday, February 5, 2009 5:32 PM
  • I-DotNET said:

    ...

    'air code
    Private Sub DoSomething()
        Try
        .....
        Catch ex As Exception
            Throw
        End Try
    End Sub

    or even worse:

    'air code
    Private Sub DoSomething()
        Try
        .....
        Catch ex As Exception
            Throw ex
        End Try
    End Sub

    Unfortunately, that's what some MS documentation shows as a demonstration of using Throw. MS documentation tends to show you the mechanics of something and, as you show above, is 'air code' even though it's valid syntax.

    It is valid, of course, but makes little sense. The reason you'd actually use a Throw in the Catch section is so that you can 'precondition' the error, modify the state of the object prior to throwing the error, or you have a finally block (with something in it, that you need to finalize before trowing the error).


    Stephen J Whiteley
    Thursday, February 5, 2009 10:05 PM
    Moderator
  • I-DotNET said:

    'air code
    Private Sub DoSomething()
        Try
        .....
        Catch ex As Exception
            Throw
        End Try
    End Sub

    or even worse:

    'air code
    Private Sub DoSomething()
        Try
        .....
        Catch ex As Exception
            Throw ex
        End Try
    End Sub


    Why is the second worse than the first?


    Thanks,
    Corey Furman
    @ Facebook
    ______________________________________________________
    Please mark posts as answer or helpful when they are.
    Friday, February 6, 2009 2:10 AM
  • Two cents, since the thread requests them...  =)

    On one hand it is true that you should intentionally throw exceptions as little as possible - use them only as a last resort in normal coding.

    However, when creating methods on an object, there is nothing wrong with throwing an exception when the case warrants - that's why they are there.  Assuming that the OpenFile function in the example would do more work than File.Open(), throwing an exception on an error condition within this function is perfectly acceptable.  Particularly as in the example where a specific type of error is thrown with a descriptive message.

    Then on the other hand, a consumer of this method may find either functionality desirable depending on the situation.  So I would agree that in the ideal world, you would provide both methods when you know your code has a potential for failure.  We see this model in the Parse() and TryParse() methods of the numeric data types and there it makes sense - the string may or may not be known to be a number prior to parsing.


    Reed Kimble - "When you do things right, people won't be sure you've done anything at all"
    • Marked as answer by Yichun Feng Tuesday, February 10, 2009 2:31 AM
    Friday, February 6, 2009 6:07 AM
    Moderator
  • Corey Furman said:

    I-DotNET said:

    'air code
    Private Sub DoSomething()
        Try
        .....
        Catch ex As Exception
            Throw
        End Try
    End Sub

    or even worse:

    'air code
    Private Sub DoSomething()
        Try
        .....
        Catch ex As Exception
            Throw ex
        End Try
    End Sub


    Why is the second worse than the first?


    The first one is bad for two reasons. 

    First, you should not catch general exceptions.  Instead, you're supposed to catch specific exceptions such as FileNotFoundException or DivideByZeroException.  This is documented here:

    http://msdn.microsoft.com/en-us/library/ms182137.aspx

    The second reason (and I've never seen this documentented anywhere) is that it doesn't really do anything.  If all you're going to do with your exception handling is simply catch an exception and then throw it back up the callstack, you haven't doing anything useful.  The exception would get thrown up the callstack anyway.  IOW, you've written four 4 lines of code that don't do anything practical.

    I really wish Microsoft would document this somewhere.  It's briefly mentioned here, "Do not overuse catch. Exceptions should often be allowed to propagate up the call stack." but they need to provide more detail:

    http://msdn.microsoft.com/en-us/library/ms229005.aspx 

    The second one is bad for the above two reasons plus a third.  When an exception is first thrown, it keeps a record of the stack trace. The way the code is written in the second example, the original stack trace is lost.  So you should throw it without specificying the exception.  This is documented here:

    http://msdn.microsoft.com/en-us/library/ms182363.aspx

    Putting it all together, this (hopefully) is a good example of correct exception handling:

        'air code  
        Private Sub DoSomething()  
            Try 
                'do something that might cause a file not found exception  
            Catch ex As FileNotFoundException  
                MessageBox.Show("The file you specified was not found.  Please try again.", Application.ProductName, MessageBoxButtons.OK, MessageBoxIcon.Error)  
            End Try 
        End Sub 
     

    EDIT:  After I posted this above code, I realized that it didn't follow best practices for strings or for globalization.  I don't want to hijack Kevin's thread, so I created a new one here: 

    http://social.msdn.microsoft.com/Forums/en-US/vbgeneral/thread/d1e66274-5092-4734-8854-a5c47530046c
    • Marked as answer by Yichun Feng Tuesday, February 10, 2009 2:31 AM
    Friday, February 6, 2009 2:38 PM
  • > Unfortunately, that's what some MS documentation shows as a demonstration of using Throw.
    > MS documentation tends to show you the mechanics of something and,

    You're absolutely correct.  But it's not just Microsoft.  A lot of books, articles, etc. focus on how to do something rather than why.  I might know how to use a hammer and screwdriver correctly, but if I don't know enough to use the hammer on the nails and the screw drivers on the screws, I've got a serious problem.
     
    > as you show above, is 'air code' even though it's valid syntax.

    By 'air code' I mean that I didn't run it through the compiler so some minor syntactical or spelling errors might be present but the technique itself that I'm demonstrating can be discerned.

    > It is valid, of course, but makes little sense. The reason you'd actually use a Throw in the Catch
    > section is so that you can 'precondition' the error, modify the state of the object prior to throwing
    > the error, or you have a finally block (with something in it, that you need to finalize before trowing the error).

    You can also have a Try...Finally block without a Catch:

    'air code
    Private Sub DoSomething()
        Try
        .....
        Finally
            MyObject.Dispose()
        End Try
    End Sub

    Friday, February 6, 2009 5:50 PM
  • I-DotNET said:

    ...

    You can also have a Try...Finally block without a Catch:

    ....

    True enough; I've felt this does go against the Microsoft 'principle' of not catching general exceptions, but always catch specific exceptions...this is just saying 'chuck all the exceptions away' :)

    Having said that, I've found the 'always catch specific exceptions' to be very much a guideline than a rule - that is, I rarely catch specific exceptions - right or wrong, but then I also rarely throw exceptions out of a class unless absolutely necessary as part of the design or is unavoidable.

    I do think error handling is actually one of the harder concepts to get to grips with. Summarily, error handling: 5 minutes to learn, a lifetime to master.

    Stephen J Whiteley
    • Marked as answer by Yichun Feng Tuesday, February 10, 2009 2:31 AM
    Friday, February 6, 2009 7:25 PM
    Moderator
  • > I've felt this does go against the Microsoft 'principle' of not catching general exceptions, but always catch specific exceptions

    Actually, this doesn't.  You're not catching anything.  The error will propagate up the call stack as normal.  All it does is allow you to free resources.  It's basically the same thing as using a Using.

    In fact, before Using was added to VB, this was the only way to free resourses.

    Friday, February 6, 2009 9:57 PM