6
\$\begingroup\$

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

  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("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("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
            MessageBox.Show("Error occurred in LoginValidator.ValidateLoginInfo" & Environment.NewLine & Environment.NewLine & ex.Message, msgCaption)
            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
\$\endgroup\$
  • \$\begingroup\$ I've removed the fluff you added to the end of your answer. If you have something to ask, ask it in the comments of the answer. \$\endgroup\$ – Ethan Bierlein Oct 8 '15 at 14:58
  • \$\begingroup\$ 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'? \$\endgroup\$ – Rose Oct 8 '15 at 15:49
  • \$\begingroup\$ As long as your new question contains working, non-hypothetical code that you want reviewed, then ask away! :-) \$\endgroup\$ – Ethan Bierlein Oct 8 '15 at 15:50
  • \$\begingroup\$ 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? \$\endgroup\$ – Rose Oct 8 '15 at 15:59
  • \$\begingroup\$ Please just ask a new question. It's easier for everyone on the site, and it doesn't invalidate answers. \$\endgroup\$ – Ethan Bierlein Oct 8 '15 at 15:59
5
\$\begingroup\$
  • 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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – Rose Oct 8 '15 at 14:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.