Creating a new item or updating an existing item on page load

Question

When the page loads, it checks to see if the user is trying to update an existing book or create a new book.

Something about my code just doesn't seem right. It feels cumbersome and not very logical. Could someone put me out of my misery and let me know that I wrote this bit ok?

Public Property modID As Int32

Protected Sub Page_Load(ByVal sender As Object, ByVal e As System.EventArgs) Handles Me.Load
Dim bookID = Request.QueryString("bookID")

If Not String.IsNullOrWhiteSpace(bookID) Then
        newBook = False
    Else
        newBook = True
        Create()   'create a new book
        bookID = modID.ToString
    End If

    If Not newBook Then
        commitData()   'saves data on form
    End If

    If Not IsPostBack And Not newBook Then
        Page.DataBind()
        populateControls()    'populate my dropdown lists and radio/checkbox controls
        populateDatasets()    'load datasets to be used later
    End If

End Sub

Public Sub Create()
    'create a new book
    newBook = False
    Using dbcon As New SqlConnection(DBUtilities.ConnectionStringDefault)
        dbcon.Open()
        Dim cmd As New SqlCommand(CreateSQL, dbcon)
        cmd.Parameters.Add(New SqlParameter("@authorID", currentUser))
        modID = DirectCast(cmd.ExecuteScalar(), Int32)
    End Using
    navmod = NavbookFactory.Getbook(CInt(modID))
    populateNewBook()
End Sub

Answer 1

The only real issue I can see (it's been a while since I worked in VB, so there may be other things) is using a global value to store the ID of the created record. Make your Create() sub a function and return the ID instead of storing it in the property; that will remove a potential source of errors.

Other than that, naming is always important. Create doesn't say much, maybe name it CreateNewBook. Also, what do navmod or modID mean? Generally be more explicit in your naming, it will help others (and you as well a bit later on) understand your code.

This may not exactly compile, as my VB days are a bit in the past as mentioned, but I think you'll get the idea:

Protected Sub Page_Load(ByVal sender As Object, ByVal e As System.EventArgs) Handles Me.Load
    Dim bookID = Request.QueryString("bookID")

    If Not String.IsNullOrWhiteSpace(bookID) Then
        newBook = False
    Else
        newBook = True
        bookID = CreateNewBook()   'create a new book
    End If

    If Not newBook Then
        commitData()   'saves data on form
    End If

    If Not IsPostBack And Not newBook Then
        Page.DataBind()
        populateControls()    'populate my dropdown lists and radio/checkbox controls
        populateDatasets()    'load datasets to be used later
    End If
End Sub

Public Function CreateNewBook() As String
    'create a new book
    Using dbcon As New SqlConnection(DBUtilities.ConnectionStringDefault)
        dbcon.Open()
        Dim cmd As New SqlCommand(CreateSQL, dbcon)
        cmd.Parameters.Add(New SqlParameter("@authorID", currentUser))
        modID = DirectCast(cmd.ExecuteScalar(), Int32)
    End Using
    navmod = NavbookFactory.Getbook(CInt(modID))
    populateNewBook()

    CreateNewBook = modID.ToString()
End Function

Now that I think of it, the modID property I just eradicated may have been used by other methods of yours. If at all possible, try to always pass those values around to any methods that need them. You want to have as little "global" state as possible, because that increases the risk of side effects - and such bugs are often pretty hard to track down and fix.

Also, if you need to work in VB (if you don't, my advice would be to consider switching to C#, as it's a much clearer and stricter language, you can often get much better help because many advanced programmers shun VB, and most example code nowadays will also be in C#), always call parameterless methods (such as ToString()) with parentheses anyway - method calls that don't look like method calls are always a pain, and it gets worse if that code should ever have to transition to another language.