# Creating a new item or updating an existing item

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?

It's pretty simple, when the page loads, it is checking to see if the user is trying to update an existing book or create a new book.

Here is the code:

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)
modID = DirectCast(cmd.ExecuteScalar(), Int32)
End Using
navmod = NavbookFactory.Getbook(CInt(modID))
populateNewBook()
End Sub

-

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

-