Login validator class

I'm trying to learn more about object oriented programming and I have a few questions about the class below.

I'm working in Visual Basic.NET

My questions are

• Is there a better place / way to show the MessageBox.Show?

• I'm being forced to use a single Oracle Connection, in my sample cnn is declared with Friend scope so the entire solution can see it. Is there a better way to handle this Connection Object, perhaps I should pass it all about my app as a parameter?

• The Sub SaveJobInfo I put this in a Sub so it wouldn't clutter up the ValidateLoginInfo code, was wondering if there is a better way to check for Nulls other than using the .Size property?

• Any other ideas that can improve this code?

```
Public Class LoginValidator

Friend Shared Function ValidateLoginInfo(jobNum As String, userID As String, ByRef djnInfo As JobNumInfo) As Boolean
Try

Dim cmd As New OracleCommand() With {.Connection = cnn, .CommandType = CommandType.StoredProcedure, .CommandText = "owner.packageName.procedureName"}

cmd.Parameters.Add(New OracleParameter("param1", OracleDbType.NVarchar2, jobNum, ParameterDirection.Input))
cmd.Parameters.Add(New OracleParameter("param2", OracleDbType.NVarchar2, userID, ParameterDirection.Input))
cmd.Parameters.Add(New OracleParameter("param3", OracleDbType.NVarchar2, My.Computer.Name.ToUpper, ParameterDirection.Input))
cmd.Parameters.Add(New OracleParameter("param3b", OracleDbType.NVarchar2, DJN_LOCK_ID, ParameterDirection.Input))

cmd.Parameters.Add(New OracleParameter("param4", OracleDbType.Varchar2, 20, String.Empty, ParameterDirection.Output))
cmd.Parameters.Add(New OracleParameter("param5", OracleDbType.Varchar2, 20, String.Empty, ParameterDirection.Output))
cmd.Parameters.Add(New OracleParameter("param6", OracleDbType.Varchar2, 20, String.Empty, ParameterDirection.Output))
cmd.Parameters.Add(New OracleParameter

• 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.

StackExchange Code Review Q#106852, answer score: 5