sub vs function passing in parameters and returning value (Peer Code Review)

[Glowworm27]

Hello all,

I am having to do a code review on one of our contractors and ran across an interesting piece of code.

The contractor created a datalayer and used sub routines instead of functions to return values. And my question is why would he do this and how does it differ from best practices

I have included a sample of his code below...

'''''''''''''''''''''''''''''''''''''''''''''''
Partial Class SomePage
 Inherits System.Web.UI.Page
  Sub LoadModelsTable(ByVal Model_id As String)
        Dim oDt As New Data.DataTable,
            sExec As String = "",
            dl As New DataLayer

        sExec = "exec DataSQL..spGetVehModel '" & Model_id & "'"

        dl.GetData(sExec, oDt)
        rgModels.DataSource = oDt
        rgModels.DataBind()
    End Sub
End Class
'''''''''''''''''''''''''''''''''''''''''''''''

'''''''''''''''''''''''''''''''''''''''''''''''
Public Class Datalayer
 Public Sub GetData(ByVal sExec As String, ByVal oDt As Data.DataTable)
        Dim oCn As New SqlConnection(System.Configuration.ConfigurationManager.AppSettings("oCn"))
        Dim oDa As New SqlDataAdapter(sExec, oCn)
        oDa.SelectCommand.CommandTimeout = 10000
        oCn.Open()
        oDa.Fill(oDt)
        oCn.Close()
    End Sub
End Class
'''''''''''''''''''''''''''''''''''''''''''''''

However what I am used to seeing and how I code is as follows

'''''''''''''''''''''''''''''''''''''''''''''''
Partial Class SomePage
 Inherits System.Web.UI.Page
  Sub LoadModelsTable(ByVal model_id As String)
        Dim oDt As New Data.DataTable
        
        oDt = Datalayer.GetVehicleModel(model_id)
        rgModels.DataSource = oDt
        rgModels.DataBind()
    End Sub
End Class
'''''''''''''''''''''''''''''''''''''''''''''''

'''''''''''''''''''''''''''''''''''''''''''''''
Public Class Datalayer
 Public Shared Function GetVehicleModel(ByVal sMake As String) As DataSet
        Dim oParams As New ParameterCollection

        Try
            oParams.Add(New Parameter("@VehMake", DbType.String, sMake))
            Return RunExecuteDataset("DataSQL", "spGetVehModel", oParams)
        Catch ex As Exception
            Throw
        Finally
            oParams = Nothing
        End Try
    End Function

 Private Shared Function RunExecuteDataset(ByVal sdatabaseName As String, ByVal sProcName As String, ByVal oParams As ParameterCollection) As DataSet
        Dim db As Database = DatabaseFactory.CreateDatabase(sdatabaseName)
        Dim mycommand As DbCommand = db.GetStoredProcCommand(sProcName)
        Dim myParm As Parameter

        Try
           For Each myParm In oParams
                If myParm.Direction = ParameterDirection.Input Then
                    db.AddInParameter(mycommand, myParm.Name, myParm.DbType, myParm.DefaultValue)
                End If
            Next

            Return db.ExecuteDataSet(mycommand)

        Catch ex As Exception
            Throw 
        Finally
            db = Nothing
            If Not mycommand Is Nothing Then
                mycommand.Dispose()
                mycommand = Nothing
            End If
            myParm = Nothing
        End Try
    End Function
End Class

George Oakes
CEO & President
COPS Software, Inc.

http://www.cops2005.com

Programmer & Developer
.Net, WSS 3.0, SQL DBA
Check out this awsome .Net Resource!

http://www.learnvisualstudio.net

 

[CajunCenturion]

==> used sub routines instead of functions to return values
Subroutines do not return values. In fact, that's the primary difference between a function and a subroutine. A function returns a value and a subroutine does not. What the contractor is doing is passing the odt object by reference thus allowing the subroutine to access the same instance of that object. That is slightly more efficient than calling a function which returns a value and then assigning that value to a local variable.

There is absolutely nothing wrong with passing parameters by reference instead of by value. There are cases where passing by reference is the better approach, perhaps even the only approach. There are cases where passing by value is the better approach, and there are cases, where functionally, it doesn't really matter. In this case, functionally, it doesn't matter.

There are two fundamental differences between your code and the contractor's code, but neither has anything to do with functions and/or subroutines. You are assigning SQL values with parameters, which from a security perspective is preferable, and of course, you have try catch processes in place.


--------------
Good Luck
To get the most from your Tek-Tips experience, please read
FAQ181-2886
Wise men speak because they have something to say, fools because they have to say something. - Plato

 

[Glowworm27]

What the contractor is doing is passing the odt object by reference thus allowing the subroutine to access the same instance of that object

I am looking at the subroutine, and he is actually passing in the Odt object ByVal, not ByRef, so that wouldn't even work then would it?

George Oakes
CEO & President
COPS Software, Inc.

http://www.cops2005.com

Programmer & Developer
.Net, WSS 3.0, SQL DBA
Check out this awsome .Net Resource!

http://www.learnvisualstudio.net

 

[CajunCenturion]

A object is a reference type entity so the difference between a ByVal indication and a ByRef indication is whether you pass the actual value of the object pointer (ByVal), or the address of the object pointer (ByRef). In either case, the subroutine uses the correct pointer to access the object's members which means that functionally, it's a pass by reference.

--------------
Good Luck
To get the most from your Tek-Tips experience, please read
FAQ181-2886
Wise men speak because they have something to say, fools because they have to say something. - Plato

 

[Glowworm27]

Thanks CajunCenturion,
I had feeling that because it was an object the compiler was treating it differently than a string or other data type variable.

George Oakes
CEO & President
COPS Software, Inc.

http://www.cops2005.com

Programmer & Developer
.Net, WSS 3.0, SQL DBA
Check out this awsome .Net Resource!

http://www.learnvisualstudio.net