Return to Answer

• My first impression is that this class is doing too much. It is calling the database and manipulating the user interface. This needs to be separated.

• Exceptions are being swallowed and shown to the user, and a boolean value is returned.

  If an exception gets thrown in code calling to the database, let the exception bubble up. This is a catastrophic problem. If validating the login info is throwing an exception, write the database code so that it returns a flag if someone's login info isn't correct. Don't use exceptions for flow control.

• The ValidateLoginInfo method calls SaveJobInfo. After validating login info, you shouldn't be saving data to the database. That should be a separate operation.

• I'm also questioning why you need to explicitly dispose of the OracleParameter objects. The CLR should clean those up just fine on its own.

• The SaveJobInfo method is confusing. It's taking stored procedure parameter values and mapping them to a JobInfo object? This should be done after validating the login info. The stored procedure you are calling has a lot of output parameters. The validation of login info and loading data into the JobInfo object should be two separate calls. One does not have anything to do with the other.

@Rose commented:

If I changed the Exception handling and Caught any exceptions and simply Threw them back to whatever was calling ValidateLoginInfo, would this meet the 'bubbling up' definition?

If you are doing this:

Try
 // ...
Catch ex As Exception
 throw ex
End Try

Then there is no point in using a Try-Catch. Just execute the code and let the exceptions fly.

By "let the exception bubble up" I mean "don't use a Try-Catch if you can't recover from the exception." Unless your ValidateLoginInfo catches an exception and can recover from it, don't catch exceptions.

• My first impression is that this class is doing too much. It is calling the database and manipulating the user interface. This needs to be separated.

• Exceptions are being swallowed and shown to the user, and a boolean value is returned.

  If an exception gets thrown in code calling to the database, let the exception bubble up. This is a catastrophic problem. If validating the login info is throwing an exception, write the database code so that it returns a flag if someone's login info isn't correct. Don't use exceptions for flow control.

• The ValidateLoginInfo method calls SaveJobInfo. After validating login info, you shouldn't be saving data to the database. That should be a separate operation.

• I'm also questioning why you need to explicitly dispose of the OracleParameter objects. The CLR should clean those up just fine on its own.

• The SaveJobInfo method is confusing. It's taking stored procedure parameter values and mapping them to a JobInfo object? This should be done after validating the login info. The stored procedure you are calling has a lot of output parameters. The validation of login info and loading data into the JobInfo object should be two separate calls. One does not have anything to do with the other.

@Rose commented:

If I changed the Exception handling and Caught any exceptions and simply Threw them back to whatever was calling ValidateLoginInfo, would this meet the 'bubbling up' definition?

If you are doing this:

Try
 // ...
Catch ex As Exception
 throw ex
End Try

Then there is no point in using a Try-Catch. Just execute the code and let the exceptions fly.

By "let the exception bubble up" I mean "don't use a Try-Catch if you can't recover from the exception." Unless your ValidateLoginInfo catches an exception and can recover from it, don't catch exceptions.

• My first impression is that this class is doing too much. It is calling the database and manipulating the user interface. This needs to be separated.

• Exceptions are being swallowed and shown to the user, and a boolean value is returned.

  If an exception gets thrown in code calling to the database, let the exception bubble up. This is a catastrophic problem. If validating the login info is throwing an exception, write the database code so that it returns a flag if someone's login info isn't correct. Don't use exceptions for flow control.

• The ValidateLoginInfo method calls SaveJobInfo. After validating login info, you shouldn't be saving data to the database. That should be a separate operation.

• I'm also questioning why you need to explicitly dispose of the OracleParameter objects. The CLR should clean those up just fine on its own.

• The SaveJobInfo method is confusing. It's taking stored procedure parameter values and mapping them to a JobInfo object? This should be done after validating the login info. The stored procedure you are calling has a lot of output parameters. The validation of login info and loading data into the JobInfo object should be two separate calls. One does not have anything to do with the other.

• My first impression is that this class is doing too much. It is calling the database and manipulating the user interface. This needs to be separated.

• Exceptions are being swallowed and shown to the user, and a boolean value is returned.

  If an exception gets thrown in code calling to the database, let the exception bubble up. This is a catastrophic problem. If validating the login info is throwing an exception, write the database code so that it returns a flag if someone's login info isn't correct. Don't use exceptions for flow control.

• The ValidateLoginInfo method calls SaveJobInfo. After validating login info, you shouldn't be saving data to the database. That should be a separate operation.

• I'm also questioning why you need to explicitly dispose of the OracleParameter objects. The CLR should clean those up just fine on its own.

• The SaveJobInfo method is confusing. It's taking stored procedure parameter values and mapping them to a JobInfo object? This should be done after validating the login info. The stored procedure you are calling has a lot of output parameters. The validation of login info and loading data into the JobInfo object should be two separate calls. One does not have anything to do with the other.

@Rose commented:

If I changed the Exception handling and Caught any exceptions and simply Threw them back to whatever was calling ValidateLoginInfo, would this meet the 'bubbling up' definition?

If you are doing this:

Try
 // ...
Catch ex As Exception
 throw ex
End Try

Then there is no point in using a Try-Catch. Just execute the code and let the exceptions fly.

By "let the exception bubble up" I mean "don't use a Try-Catch if you can't recover from the exception." Unless your ValidateLoginInfo catches an exception and can recover from it, don't catch exceptions.