Link to home
Create AccountLog in
Avatar of hmcgeehan
hmcgeehan

asked on

Code check prior to code review

Hi

Prior to a code review can anyone suggest bad code or code that could be improved in the code below?

Thanks
H
Partial Public Class mediacentre_gallery
    Inherits System.Web.UI.Page
 
    Private _month As Integer = 0
    Protected Sub Page_Load(ByVal sender As Object, ByVal e As System.EventArgs) Handles Me.Load
 
        If Request.QueryString("m") <> "" Then
            _month = Convert.ToInt32(Request.QueryString("m"))
            BindGaleriesByMonth(_month)
        Else
            BindMonths()
        End If
 
 
    End Sub
 
    Private Sub BindMonths()
 
        Dim lstMonths As New List(Of Integer)
 
        For i As Integer = 1 To 12
            lstMonths.Add(i)
        Next
 
 
        If (Not lstMonths Is Nothing) AndAlso (lstMonths.Count > 0) Then
 
            rptMonths.DataSource = lstMonths
            rptMonths.DataBind()
 
            pnlMonthsList.Visible = True
 
 
        End If
    End Sub
 
    Private Sub BindGaleriesByMonth(ByVal iMonth As Integer)
 
        Dim MonthGalleriesList As List(Of Gallery) = Gallery.GetGalleriesByMonth(iMonth)
 
        If (Not MonthGalleriesList Is Nothing) AndAlso (MonthGalleriesList.Count > 0) Then
 
 
            rptGalleries.DataSource = MonthGalleriesList
            rptGalleries.DataBind()
 
            pnlGalleriesList.Visible = True
 
        Else
 
            pnlGalleriesList.Visible = False
            pnlNoContent.Visible = True
 
        End If
 
    End Sub
 
    Public Function GetMonthName(ByVal monthNumber As Integer) As String
        Dim df As DateTimeFormatInfo = New DateTimeFormatInfo()
        Return df.GetMonthName(monthNumber)
    End Function
 
End Class

Open in new window

Avatar of Dirk Haest
Dirk Haest
Flag of Belgium image

The only thing I can think of is that you don't you any kind of errorhandling !
Avatar of hmcgeehan
hmcgeehan

ASKER

Also should Function be private / public?
And should Sub be private / public?

Thanks
ASKER CERTIFIED SOLUTION
Avatar of Arthur_Wood
Arthur_Wood
Flag of United States of America image

Link to home
membership
Create an account to see this answer
Signing up is free. No credit card required.
Create Account
sorry, take out the final End If:


    Private Sub BindMonths()
 
        Dim lstMonths As New List(Of Integer)
 
        For i As Integer = 1 To 12
            lstMonths.Add(i)
        Next
 
        rptMonths.DataSource = lstMonths
        rptMonths.DataBind()
 
       pnlMonthsList.Visible = True
 
    End Sub

Open in new window

In this procedure, you do not need to test if (Not MonthGalleriesList Is Nothing), sice that list will ALWAYS be defined, and an instance has been created.

AW
    Private Sub BindGaleriesByMonth(ByVal iMonth As Integer)
 
        Dim MonthGalleriesList As List(Of Gallery) = Gallery.GetGalleriesByMonth(iMonth)
 
        If MonthGalleriesList.Count > 0 Then
 
             rptGalleries.DataSource = MonthGalleriesList
             rptGalleries.DataBind()
 
             pnlGalleriesList.Visible = True
 
        Else
 
            pnlGalleriesList.Visible = False
            pnlNoContent.Visible = True
 
        End If
 
    End Sub

Open in new window

Depends on your code review standards i guess. Comments for method headers and alongside implementation would be helpful i've always found
Thanks very much for the feedback guys.

Just a final part.
Should the functions / subs be private or public?

Thanks
It depends on whether they should be seen only INSIDE the class, or are they to be seen by code that USES the class?

In some classes, the external code will make use of the Methods exposed by the class - in this case the subs and functions that are to be exposed are Public.

In some classes, there are subs and functions that only serve to assist the classes otyher subs and/or functions in carrying out their job(s).  In this case, the helper procedures would be Private - only seen by the code with the class itself.
for example, in your code, you have a call to

Gallery.GetGalleriesByMonth(iMonth)

in this case, the GetGalleriesByMonth(iMonth) function in the Gallery Class would be declared as Public.

AW
glad to be of assistance.

AW
Hi Arthur

Sorry to ressurect this!

RE the line - If (Not MonthGalleriesList Is Nothing) AndAlso (MonthGalleriesList.Count > 0) Then

How can you be sure that MonthGalleriesList will never be Nothing?

This might be of help

Public Shared Function GetGalleriesByMonth(ByVal iMonth As Integer) As List(Of Gallery)
            Try

                Dim ds As DataSet = SqlHelper.ExecuteDataset(System.Configuration.ConfigurationManager.AppSettings("ConnectionString"), "usp_Gallery_GetAllByMonth", iMonth)
                Return CreateFromDataSet(ds)
            Catch ex As Exception
                Throw
            End Try


And likewise if I do

Dim lstMonths As New List(Of Integer)

Do I need to check

If (Not lstMonths Is Nothing) AndAlso (lstMonths.Count > 0) Then

How do I know it will never be nothing?

Thanks
        End Function
Dim lstMonths As New List(Of Integer)

Do I need to check

If (Not lstMonths Is Nothing) AndAlso (lstMonths.Count > 0) Then

How do I know it will never be nothing?

Because, by declaring the lstMonths as NEW List(of Integer), it will ALWAYS be instantiated (since the declaration and instantiation occur in the same line), so it can NEVER be Nothing.  In addition, you immediately fill the List with the Integers 1 to 12, so, again, the variable cannot be NOTHING.  There is no scenario in which lstMonths could ever end up being NOTHING.

If you declare the variable in one statement, and then later instantiate the variable, like this:

Dim lstMonths as List(of Integer)
.
.
.
lstMonths = New List(Of Integer)

then something could happen in the code between the Declaration (Dim...) and the Instatiation ( = New List(of Integer)) so that the variable was not instantaited.  The there could be a small possiblity that the variable was still Nothing when you tested it.

AW