Can someone look at this code for me and...

[TType85]

tell me if its good code or not? I'm just really starting to play with vb.net. This code does work. It pulls the data out of my database no problems, I just want to know if its a good way to code it.

I know I need to comment it better

[Carnifex]

A couple of suggestions as I browsed thru your code.

Use properties whenever you want to expose something as public so you can control if it is readonly/writeonly and any validation if someone trys to assign a value to a class member.

I'd recommend prefixing any class members with m_ (e.g strConnectionString would be m_ConnectionString) as this makes it easier to see if you are modifying something local to the sub/function you are working on, or a class member.

In some of your code you keep indexing the same thing repeatedly rather than store a temporary reference to the item. While .Net has really good caching you are still forcing it to index something repeatdly. (An example of this is part way thru your code you access the user table in the dataset several times when it would probably be better to create a reference to the datatable and use the reference, saves on the volume of indexing)

[TType85]

Originally posted by Carnifex
In some of your code you keep indexing the same thing repeatedly rather than store a temporary reference to the item. While .Net has really good caching you are still forcing it to index something repeatdly. (An example of this is part way thru your code you access the user table in the dataset several times when it would probably be better to create a reference to the datatable and use the reference, saves on the volume of indexing)

What exactly do you mean by reference instead?


i'm assuming the code you are talking about is this part.

With theUser
                .UserID = theDataSet.Tables("User").Rows(0).Item("User_ID")
                .LoginID = theDataSet.Tables("User").Rows(0).Item("Login_ID")
                .Password = theDataSet.Tables("User").Rows(0).Item("Password")
                .FullName = theDataSet.Tables("User").Rows(0).Item("Full_Name")
                .AccessLevel = theDataSet.Tables("User").Rows(0).Item("Access_Level")
                .AccountLocked = theDataSet.Tables("User").Rows(0).Item("Account_Locked")
            End With

Your other 2 suggestions are right on. Thanks for the suggestions.

Chris

[Carnifex]

Yes that is the piece of code I am talking about.
Instead of continually re-indexing the user table, and then re-indexing the row you are working on in the dataset do something like this.

Dim userRow As DataRow
 
userRow = theDataSet.Tables("User").Rows(0)
 
With theUser
        .UserID = userRow.Item("User_ID")
        .LoginID = userRow.Item("Login_ID")
 
        'etc...
End With

With this code you are saving yourself 10 index queries inside the dataset and datatable.

[TType85]

Thanks, i'm updating the code to reflect your suggestions.


One question thougg, prefixing the class members with m_ is a great idea, is there a list of preferred prefixes like that? i'm used to the str, int, etc... prefixes.

Somthing like:
c_ = class
f_ = function
s_ = Sub
e_ = event
etc...

Thanks

[Carnifex]

m_ (Member)
c_ (Constant)

Are really the only prefixes I use. I use camel case for variables so that the first word in a variable name is lowercase, and all the following words first letter start with capitals.

Functions/Subs names always start with capitals. I don't differentiate between the 2. I don't prefix events.

I used to prefix the datatype of a variable, however it makes the variable name look messy at times, and its very easy to find out the datatype of a variable by hovering your mouse over it in the VS .Net IDE so I stopped doing it for cleaner looking code.

Some people like to prefix classes with a capital C. I don't think its all the necessary myself, so don't bother.