I'm working in Visual Basic.NET

My questions are

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

2. 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?

3. 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?

4. 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("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("param7", OracleDbType.Varchar2, 10000, String.Empty, ParameterDirection.Output))
cmd.Parameters.Add(New OracleParameter("param8", OracleDbType.Varchar2, 10, String.Empty, ParameterDirection.Output))
cmd.Parameters.Add(New OracleParameter("param9", OracleDbType.Varchar2, 20, String.Empty, ParameterDirection.Output))
cmd.Parameters.Add(New OracleParameter("param10", OracleDbType.Varchar2, 10, String.Empty, ParameterDirection.Output))
cmd.Parameters.Add(New OracleParameter("param11", OracleDbType.Varchar2, 2, String.Empty, ParameterDirection.Output))
cmd.Parameters.Add(New OracleParameter("param12", OracleDbType.Varchar2, 50, String.Empty, ParameterDirection.Output))
cmd.Parameters.Add(New OracleParameter("param13", OracleDbType.Varchar2, 1500, String.Empty, ParameterDirection.Output))

cmd.ExecuteNonQuery()

SaveJobInfo(djnInfo, cmd)

For Each p As OracleParameter In cmd.Parameters
p.Dispose()
Next

Catch ex As Exception
Return False
End Try
Return True

End Function

Private Shared Sub SaveJobInfo(ByVal djnInfo As JobNumInfo, ByVal cmd As OracleCommand)

If cmd.Parameters(4).Size > 0 Then
djnInfo.UserAccessLevel = cmd.Parameters(4).Value.ToString
Else
djnInfo.UserAccessLevel = String.Empty
End If

If cmd.Parameters(5).Size > 0 Then
djnInfo.HasChildren = cmd.Parameters(5).Value.ToString.ToUpper = "YES"
Else
djnInfo.HasChildren = False
End If

If cmd.Parameters(6).Size > 0 Then
djnInfo.IsReleased = cmd.Parameters(6).Value.ToString.ToUpper = "YES"
Else
djnInfo.IsReleased = False
End If

If cmd.Parameters(7).Size > 0 Then
djnInfo.SpecInfo = cmd.Parameters(7).Value.ToString
Else
djnInfo.SpecInfo = String.Empty
End If

If cmd.Parameters(8).Size > 0 Then
djnInfo.OrgCode = cmd.Parameters(8).Value.ToString
Else
djnInfo.OrgCode = String.Empty
End If

If cmd.Parameters(9).Size > 0 Then
djnInfo.UOMFromOra = cmd.Parameters(9).Value.ToString
Else
djnInfo.UOMFromOra = String.Empty
End If

If cmd.Parameters(10).Size > 0 Then
djnInfo.DrawingNumber = cmd.Parameters(10).Value.ToString
Else
djnInfo.DrawingNumber = String.Empty
End If

If cmd.Parameters(11).Size > 0 Then
djnInfo.ProductRevisionCode = cmd.Parameters(11).Value.ToString
Else
djnInfo.ProductRevisionCode = String.Empty
End If

If cmd.Parameters(12).Size > 0 Then
djnInfo.FirstArticleFamCode = cmd.Parameters(12).Value.ToString
Else
djnInfo.FirstArticleFamCode = String.Empty
End If
' param 13 will be handled differently than the others
End Sub

End Class

• I had about 4 more 'fluffs' i was going to add to this original question so I could could use the formatting. Would it make more sense to make new questions for my other 'fluffs'? – Rose Oct 8 '15 at 15:49
• As long as your new question contains working, non-hypothetical code that you want reviewed, then ask away! :-) – Ethan Bierlein Oct 8 '15 at 15:50
• It would be nice if I could keep it all in one question. I'm not sure how to add to this question and have the option to 'format' my fluff additions? – Rose Oct 8 '15 at 15:59
• Please just ask a new question. It's easier for everyone on the site, and it doesn't invalidate answers. – Ethan Bierlein Oct 8 '15 at 15:59

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

• Thank you for the feedback I really appreciate it. In regards to an exception being thrown in the code calling to the database and letting the exception 'bubble up'. . . Please see edit to original question. 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? – Rose Oct 8 '15 at 14:54