Reputation: 510
I'm new to OOP so I need help to understand if this is the correct way to handle it. I have a 3-Layer web project (+ DTOs) and I'm trying to understand wich is the best way to return "errors" from the Business Objects to the Presentation Layer. In particular now I'm facing this creating a user. Let's say I want to create the the user in the db upon website registration and I need to tell the actual user if the username or email have already been taken (this is just an example).
The ASP.NET Membership.CreateUser() method for example pass a MembershipCreateStatus object byRef, so the method is used to return an Enum (wich resides in another namespace...) to pass the status of the attempt and this could be DuplicateEmail, DuplicateUserName, and so on.
I implemented another way based on exceptions but I'd like your opinion on it. In the BLL manager class that creates the user I created a nested Excpetion class and nested Enums for error type this way:
Public Class UserManager
Public Enum ErrorType
DatabaseError = 1
UserExists = 2
EmailExists = 3
End Enum
Public Class ManagerException
Inherits Exception
Public Property ErrorType As ErrorType
Public Property SuggestedUserName As String
Public Property SuggestedEmail As String
Public Sub New()
MyBase.New()
End Sub
Public Sub New(message As String)
MyBase.New(message)
End Sub
Public Sub New(message As String, inner As Exception)
MyBase.New(message, inner)
End Sub
End Class
Public Function CreateUserLogin(user As EvaVwUserLogin) As Guid
If user Is Nothing Then
Throw New ApplicationException("No user suppied")
End If
If String.IsNullOrWhiteSpace(user.Password) OrElse String.IsNullOrWhiteSpace(user.UserName) Then
Throw New ApplicationException("Password or username missing")
End If
If CheckDuplicateUserName() Then
Dim ex As New ManagerException("Username exists")
ex.ErrorType = ErrorType.UserExists
ex.SuggestedUserName = "this username is free"
Throw ex
End If
End Function
End Class
Then in the UI layer (aspx code behind) I call the manager and check the exception like this:
Dim userManager As New UserManager
Try
userManager.CreateUserLogin("test", "test")
Catch ex As UserManager.ManagerException
If ex.ErrorType = userManager.ErrorType.UserExists Then
Dim suggestedUsername = ex.SuggestedUserName
' Display error message and suggested user name
End If
End Try
Is this a correct approach, considering that both the exception and the enums are very specific to this manager, so that if I go down this route, each manager will have its nested ManagerException with related enums?
Thanks in advance for your opinions as always.
Following up the "custom code" scenario kindly suggested by Brian and Cyborg (I marked his answer just because it's more complete Brian maybe other users are interested in MS advice) do you think that nesting the custom return object and relative enums in the manager class would be a good idea? Since those enums will be strictly related to this manager class (and each managar class in the BLL would have his own) do you think I could still use them as the status code passed?
EDIT: I refactored this way... do you think it's ok?
Public Class UserManager
Public Enum ErrorType
DatabaseError = 1
UserExists = 2
EmailExists = 3
End Enum
Public Class ErrorData
Public Property ErrorType As ErrorType
Public Property SuggestedUserName As String
Public Property SuggestedEmail As String
End Class
Public Function CreateUserLogin(username As String, password As String, ByRef errorData As ErrorData) As Guid
Dim user As New EvaVwUserLogin
user.UserName = username
user.Password = password
Return CreateUserLogin(user, errorData)
End Function
Public Function CreateUserLogin(user As EvaVwUserLogin, ByRef errorData As ErrorData) As Guid
If user Is Nothing Then
Throw New ApplicationException("No user object")
End If
If String.IsNullOrWhiteSpace(user.Password) OrElse String.IsNullOrWhiteSpace(user.UserName) Then
Throw New ApplicationException("Missing password or username")
End If
Dim hashedPassword As String
hashedPassword = Crypto.HashPassword(user.Password)
If UserExists(user) Then
errorData.ErrorType = ErrorType.UserExists
errorData.SuggestedUserName = "this username is free"
Return Nothing
End If
.....
End Function
End Class
And in the Presentation Layer:
Dim userManager As New UserManager
Dim managerError As New UserManager.ErrorData
Dim userId As Guid
userId = userManager.CreateUserLogin("test", "test", managerError)
If userId = Nothing Then
If managerError.ErrorType = userManager.ErrorType.UserExists Then
Dim suggestedUserName = managerError.SuggestedUserName
' Do something with suggested user name
End If
End If
Upvotes: 1
Views: 2330
Reputation: 25810
(This answer is just to provide support for @BrianMains suggestion to use return codes rather than exceptions. The relevant documentation was too large to fit in a comment.)
From the MSDN documentation on exceptions:
A significant amount of system resources and execution time are used when you throw or handle an exception. Throw exceptions only to handle truly extraordinary conditions, not to handle predictable events or flow control. For example, your application can reasonably throw an exception if a method argument is invalid because you expect to call your method with valid parameters. An invalid method argument means something extraordinary has occurred. Conversely, do not throw an exception if user input is invalid because you can expect users to occasionally enter invalid data. In such a case, provide a retry mechanism so users can enter valid input.
Throw exceptions only for extraordinary conditions, then catch exceptions in a general purpose exception handler that applies to the majority of your application, not a handler that applies to a specific exception. The rationale for this approach is that most errors can be handled by validation and error handling code in proximity to the error; no exception needs to be thrown or caught. The general purpose exception handler catches truly unexpected exceptions thrown anywhere in the application.
In addition, do not throw an exception when a return code is sufficient; do not convert a return code to an exception; and do not routinely catch an exception, ignore it, then continue processing.
http://msdn.microsoft.com/en-us/library/system.exception.aspx
(Highlighting mine)
Upvotes: 1
Reputation: 50728
For that, I would follow what the Membership API does. Return an enum from your business component, which the view can use to check the response. So you return an enum with values like you mention to the ASP.NET page, and check the type there. You can do what you are doing, but from what I see I don't think it's necessary; plus, throwing exceptions are expensive.
To clarify what I mean by enum is return it from the caller of the business component, not via an exception.
Upvotes: 2