Critique/Rate My Code

  • (2 Pages)
  • +
  • 1
  • 2

26 Replies - 17283 Views - Last Post: 06 November 2012 - 07:24 AM

#16 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2263
  • View blog
  • Posts: 9,467
  • Joined: 29-May 08

Re: Critique/Rate My Code

Posted 25 October 2012 - 08:46 AM

null null isn't always mean it's a bug or bad design. null can be useful to indicate the lack of a value.
Was This Post Helpful? 0
  • +
  • -

#17 cfoley  Icon User is online

  • Cabbage
  • member icon

Reputation: 1998
  • View blog
  • Posts: 4,150
  • Joined: 11-December 07

Re: Critique/Rate My Code

Posted 25 October 2012 - 08:57 AM

Edit: Yes, it's not always a bad design, but if it is getting into private methods then I would seriously question the design. /edit.

And if you allow it to propagate anywhere and everywhere then all your methods have to make special cases for nulls. That's a nasty way to program. You pollute all your methods with null checks and have to write test cases to cover null arguments. That's bad enough when you have one argument but if you have two, three or four you end up with a combinatorial problem!

There are other cleaner ways or representing no data in your program. Empty collections instead of nulls and special case objects are two that spring to mind. In this case, an empty string does the job nicely, although that won't be appropriate all the time. If you absolutely must account for null then extract it to another method:

Public Function RemoveDuplicates(ByVal values As String) As String
    Dim cleanValues as String = ValidateValues(values)
    Return MakeDistinct(cleanValues)
End Function

Public Function ValidateValues(ByVal values As String) As String
    If IsNothing(values) Then
        Return ""
    Else
        Return values
    End If
End Function


Public Function MakeDistinct(ByVal values As String) As String
    ' use one of the implementations above
End Function

This post has been edited by cfoley: 25 October 2012 - 08:59 AM

Was This Post Helpful? 0
  • +
  • -

#18 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2263
  • View blog
  • Posts: 9,467
  • Joined: 29-May 08

Re: Critique/Rate My Code

Posted 25 October 2012 - 09:08 AM

The Zero-Length String "" is subtly different to the empty string String.Empty.
Was This Post Helpful? 0
  • +
  • -

#19 cfoley  Icon User is online

  • Cabbage
  • member icon

Reputation: 1998
  • View blog
  • Posts: 4,150
  • Joined: 11-December 07

Re: Critique/Rate My Code

Posted 25 October 2012 - 11:56 PM

Are you sure? I did some testing to see the difference and came up with this. It reports that they are equal by the equals function and that they are, in fact, the same instance according to the ReferenceEquals function. If they are the same instance then what can the difference be?

    Sub Main()
        Dim a As String = ""
        Dim b As String = String.Empty

        If a.Equals(B)/> Then
            ' This gets printed
            Console.WriteLine("They are equal") 
        Else
            Console.WriteLine("Not equal")
        End If

        If ReferenceEquals(a, B)/> Then
            ' This gets printed
            Console.WriteLine("Same references")
        Else
            Console.WriteLine("Different references")
        End If

        Console.ReadKey()
    End Sub


This post has been edited by cfoley: 26 October 2012 - 12:04 AM

Was This Post Helpful? 1
  • +
  • -

#20 lucky3  Icon User is offline

  • Friend lucky3 As IHelpable
  • member icon

Reputation: 231
  • View blog
  • Posts: 765
  • Joined: 19-October 11

Re: Critique/Rate My Code

Posted 26 October 2012 - 12:30 AM

Same result with If a Is b Then.
Was This Post Helpful? 1
  • +
  • -

#21 cfoley  Icon User is online

  • Cabbage
  • member icon

Reputation: 1998
  • View blog
  • Posts: 4,150
  • Joined: 11-December 07

Re: Critique/Rate My Code

Posted 26 October 2012 - 03:21 AM

Did not know about the Is operator. While looking it up, I also found out about the IsNot patent application. :-)
Was This Post Helpful? 0
  • +
  • -

#22 depricated  Icon User is offline

  • DLN-000

Reputation: 717
  • View blog
  • Posts: 2,434
  • Joined: 13-September 08

Re: Critique/Rate My Code

Posted 05 November 2012 - 01:40 PM

Take 2! (Should this be a separate post? I don't want to clutter the board)

So one thing I built today was a login form. I want to authenticate against Active Directory. For that, I use a portion of this code:
http://vbnet.mvps.or...ritycontext.htm

I didn't pretty that up. What I'm posting here is strictly what was written by me. Once more, I'd love a critique on this portion as well. I've been reading the book Clean Code, as suggested, so I'm hoping some of that shows.

I'll explain after the code how it steps through. Hopefully, that'll be redundant by the time you get to the explanation, if I've done this right. :) That's my hope!

One note for clarification: the IDs stored in the database are only the numbers from our usernames.

Private Sub cmdLogin_Click()
   
   DoLogin

End Sub


Private Sub txtPassword_KeyPress(KeyAscii As Integer)

   If KeyAscii = vbKeyReturn Then
      KeyAscii = 0
      DoLogin
   End If
   
End Sub

Private Function DoLogin()
    If Not ValidateForm Then Exit Function
    If AuthenticateUser(txtDomain, txtUsername, txtPassword) Then
        DoSuccessfulLogin
    Else
        DoFailedLogin
    End If
End Function

Private Function ValidateForm()
    If IsNull(Me.txtUsername) Or Me.txtUsername = "" Then
      MsgBox "You must enter a User Name.", vbOKOnly, "Required Data"
        Me.txtUsername.SetFocus
        Exit Function
    End If


    If IsNull(Me.txtPassword) Or Me.txtPassword = "" Then
      MsgBox "You must enter a Password.", vbOKOnly, "Required Data"
        Me.txtPassword.SetFocus
        Exit Function
    End If
    
    ValidateForm = True
End Function

Private Function DoFailedLogin()
        MsgBox "Login Failed."
        txtPassword = ""
        txtPassword.SetFocus
End Function

Private Function DoSuccessfulLogin()
        lngMyEmpID = GetBCMByID
        SaveLoginLog
        DoCmd.Close acForm, "frmADLogon", acSaveNo
        DoCmd.OpenForm "frmSrvcMgrHome"
End Function

Private Function SaveLoginLog()
        DoCmd.SetWarnings False
        DoCmd.OpenQuery "qdelCurrentBCM", acViewNormal, acEdit
        DoCmd.OpenQuery "qappCurrentBCM", acViewNormal, acEdit
        DoCmd.OpenQuery "qappLoginLog", acViewNormal, acEdit
        DoCmd.SetWarnings True
End Function

Private Function GetBCMByID()
        GetBCMByID = DLookup("strEmpName", "tblBCM", "[lngEmpID]=" & GetStrippedID())
End Function

Private Function GetStrippedID()
   
    Dim strippedID As String
    Dim i As Integer
    
    strippedID = ""
    
    For i = 1 To Len(txtUsername)
        strippedID = strippedID + AddNumberFromString(i)
    Next
    
    GetStrippedID = strippedID
End Function

Private Function AddNumberFromString(position As Integer)
    If Mid(txtUsername, position, 1) >= "0" And Mid(txtUsername, position, 1) <= "9" Then
        AddNumberFromString = Mid(txtUsername, position, 1)
    End If
End Function



From either clicking the button "Login" or hitting enter from the Password box:
-Valdiate the Form. If it is not valid, abort.
-If user authenticated, do Login Stuff
--Set the database field "lngMyEmpID" to the BCM Name
---by looking up strEmpName from tblBCM where lngEmpID
----is only the numbers out of the ID by iterating through
-----and saving only the numbers.
-else if the user failed to authenticate
--tell them
--clear the password box
--put the focus there


Thoughts?

This post has been edited by depricated: 05 November 2012 - 01:43 PM

Was This Post Helpful? 0
  • +
  • -

#23 lucky3  Icon User is offline

  • Friend lucky3 As IHelpable
  • member icon

Reputation: 231
  • View blog
  • Posts: 765
  • Joined: 19-October 11

Re: Critique/Rate My Code

Posted 05 November 2012 - 01:56 PM

Functions are used to return something. Don't you get warnings?
Was This Post Helpful? 0
  • +
  • -

#24 depricated  Icon User is offline

  • DLN-000

Reputation: 717
  • View blog
  • Posts: 2,434
  • Joined: 13-September 08

Re: Critique/Rate My Code

Posted 05 November 2012 - 02:00 PM

Nope...

Am I missing something really basic? >.>

Maybe it's cause I learned on C++ and Java? I thought a Function in VB was like a void function in C++ if I left off the "as [whatever]". No need to return anything.
Was This Post Helpful? 0
  • +
  • -

#25 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2263
  • View blog
  • Posts: 9,467
  • Joined: 29-May 08

Re: Critique/Rate My Code

Posted 05 November 2012 - 02:16 PM

VB has Functions have an Implicit return variable, which is the same as the function's name. In VB.net it is only a Warning if you don't return a value for all of the code paths in the function.
Was This Post Helpful? 0
  • +
  • -

#26 lucky3  Icon User is offline

  • Friend lucky3 As IHelpable
  • member icon

Reputation: 231
  • View blog
  • Posts: 765
  • Joined: 19-October 11

Re: Critique/Rate My Code

Posted 05 November 2012 - 02:25 PM

MSDN: Functions in VB.NET
Was This Post Helpful? 0
  • +
  • -

#27 depricated  Icon User is offline

  • DLN-000

Reputation: 717
  • View blog
  • Posts: 2,434
  • Joined: 13-September 08

Re: Critique/Rate My Code

Posted 06 November 2012 - 07:24 AM

That's interesting. I have no formal training with VB.Net, really. We used it in one class in school, but barely.

I've been reading over the different types of procedure calls in vb - didn't even know about them! So thanks for that.

So reading over this I ought to use a Sub if I'm not returning a value? Is that correct?
Was This Post Helpful? 0
  • +
  • -

  • (2 Pages)
  • +
  • 1
  • 2