Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am new to programming and have written this Data Access Layer. I am using this DAL in my projects. Now, I have a feeling that it is pathetic (when I wrote it, I was kind of happy). Do you guys also feel the same for this DAL. Kindly help me write a better one. There is no Transaction in my DAL and I have no idea how to implement it in this DAL. I am expecting some criticism.

Connections.vb Class File

Option Explicit On
Option Strict On
Imports Microsoft.VisualBasic
Imports System.Data.SqlClient

Public Class Connection
    Public Shared Function GetDbConnection() As SqlConnection
        Dim _conString As String = ConfigurationManager.ConnectionStrings("CoachingConnectionString").ConnectionString
        Dim _connection As New SqlConnection(_conString)
        _connection.Open()
        Return _connection
    End Function
End Class

DataAccess.vb Class File

Option Explicit On
Option Strict On
Imports Microsoft.VisualBasic
Imports System.Data.SqlClient
Imports System.Data

Public Class DataAccess
    Public Overloads Shared Function GetDataTable(ByVal _sql As String, ByVal _parameterNames() As String, ByVal _parameterVals() As String) As DataTable
        Dim _connection As SqlConnection = Global.Connection.GetDbConnection()
        Dim _command As New SqlDataAdapter(_sql, _connection)
        Dim _table As New DataTable
        Try
            If _parameterNames IsNot Nothing Then
                For i = 0 To _parameterNames.Length - 1
                    _command.SelectCommand.Parameters.AddWithValue(_parameterNames(i), _parameterVals(i))
                Next
            End If
            _command.Fill(_table)
        Catch ex As Exception
            'MsgBox(ex.Message)
            _table = Nothing
        Finally
            If _connection.State = ConnectionState.Open Then
                _connection.Close()
                _connection.Dispose()
                _command.Dispose()
            End If
        End Try

        Return _table
    End Function

    Public Overloads Shared Function GetDataTable(ByVal _sql As String) As DataTable
        Dim _connection As SqlConnection = Global.Connection.GetDbConnection()
        Dim _command As New SqlDataAdapter(_sql, _connection)
        Dim _table As New DataTable
        Try
            _command.Fill(_table)
        Catch ex As Exception
            'MsgBox(ex.Message)
            _table = Nothing
        Finally
            If _connection.State = ConnectionState.Open Then
                _connection.Close()
                _connection.Dispose()
                _command.Dispose()
            End If
        End Try

        Return _table
    End Function

    Public Shared Function SelectScalar(ByVal _sql As String, ByVal _parameterNames() As String, ByVal _parameterVals() As String) As String
        Dim _returnVal As String
        Dim _connection As SqlConnection = Global.Connection.GetDbConnection()
        Dim _command As New SqlCommand(_sql, _connection)
        Dim _value As String

        Try
            If _parameterNames IsNot Nothing Then
                For i = 0 To _parameterNames.Length - 1
                    _command.Parameters.AddWithValue(_parameterNames(i), _parameterVals(i))
                Next
            End If

            _value = CStr(_command.ExecuteScalar)
            _returnVal = _value
        Catch ex As Exception
            _returnVal = Nothing
        Finally
            If _connection.State = ConnectionState.Open Then
                _connection.Close()
                _connection.Dispose()
                _command.Dispose()
            End If
        End Try

        Return _returnVal
    End Function
    Public Shared Function SelectScalar(ByVal _sql As String) As String
        Dim _returnVal As String
        Dim _connection As SqlConnection = Global.Connection.GetDbConnection()
        Dim _command As New SqlCommand(_sql, _connection)
        Dim _value As String

        Try


            _value = CStr(_command.ExecuteScalar)
            _returnVal = _value
        Catch ex As Exception
            _returnVal = Nothing
        Finally
            If _connection.State = ConnectionState.Open Then
                _connection.Close()
                _connection.Dispose()
                _command.Dispose()
            End If
        End Try

        Return _returnVal
    End Function
    Public Shared Function CRUD(ByVal _sql As String, ByVal _parameterNames() As String, ByVal _parameterVals() As String) As Integer
        Dim _noOfRowsAffected As Integer
        Dim _connection As SqlConnection = Global.Connection.GetDbConnection()
        Dim _command As New SqlCommand(_sql, _connection)

        Try
            If _parameterNames IsNot Nothing Then
                For i = 0 To _parameterNames.Length - 1
                    _command.Parameters.AddWithValue(_parameterNames(i), _parameterVals(i))
                Next
            End If

            _noOfRowsAffected = _command.ExecuteNonQuery()
        Catch ex As Exception
            'MsgBox(ex.Message)
            _noOfRowsAffected = -1
        Finally
            If _connection.State = ConnectionState.Open Then
                _connection.Close()
                _connection.Dispose()
                _command.Dispose()
            End If
        End Try

        Return _noOfRowsAffected
    End Function
End Class
share|improve this question
2  
Why not use ORM / MVC / EF ? –  Leonid Aug 25 '12 at 18:17
 
Actually am using ASP.NET web forms and new to programming...so i hope you understand..what is ORM..and kindly point out the major cons with my DAL so that i can learn what are my mistakes. –  user1593175 Aug 25 '12 at 18:21
 
Watch this whole video for a hint - the Object Relational Mapping (ORM) is taken care of by the Entity Framework youtube.com/watch?v=2AWVs7SzOXM&feature=related –  Leonid Aug 25 '12 at 19:03
 
@Leonid Thanks for the link...i am watching it now. –  user1593175 Aug 25 '12 at 19:13

2 Answers

up vote 6 down vote accepted

The first part of the answer has already been given by Leonid: "don't create your own data access methodology". Use something like Entity Framework. This is a frequently-used "wheel": don't reinvent it.

That said, there are a number of issues with your code:

  1. You do not need a separate class for your GetDbConnection function. Put it into your DataAccess class and make it Private.
  2. The SqlConnection and SqlDataAdapter classes implement the IDisposable interface. When you create an object of such a type, and consume it entirely within your method, you should place the object in a Using block. This ensures it gets cleaned up as you are doing in your Finally.
  3. Given #2, you don't need Try/Catch blocks at all. In your case, they have the primary effect of hiding any exceptions that may be thrown, so that you will never know what's wrong with your code.
  4. You have duplicated the code for filling the parameters. Extract that into a separate method.
  5. I recommend that you use a leading "_" only for class-level fields, and not for the parameters to a Function or Sub.

Here's the result of a quick refactoring of your code to resolve those issues:

Option Explicit On
Option Strict On

Imports System.Data.SqlClient
Imports System.Configuration

Public Class DataAccess
    Public Overloads Shared Function GetDataTable(ByVal sql As String, ByVal parameterNames() As String, ByVal parameterVals() As String) As DataTable
        Using connection As SqlConnection = GetDbConnection()
            Using da As SqlDataAdapter = New SqlDataAdapter(sql, connection)
                Dim table As New DataTable
                FillParameters(da.SelectCommand, parameterNames, parameterVals)
                da.Fill(table)
                Return table
            End Using
        End Using
    End Function

    Public Overloads Shared Function GetDataTable(ByVal sql As String) As DataTable
        Using connection As SqlConnection = GetDbConnection()
            Using da As New SqlDataAdapter(sql, connection)
                Dim table As New DataTable
                da.Fill(table)
                Return table
            End Using
        End Using
    End Function

    Public Shared Function SelectScalar(ByVal sql As String, ByVal parameterNames() As String, ByVal parameterVals() As String) As String
        Using connection As SqlConnection = GetDbConnection()
            Using command As SqlCommand = New SqlCommand(sql, connection)
                FillParameters(command, parameterNames, parameterVals)
                Return CStr(command.ExecuteScalar)
            End Using
        End Using
    End Function

    Public Shared Function SelectScalar(ByVal sql As String) As String
        Using connection As SqlConnection = GetDbConnection()
            Using command As New SqlCommand(sql, connection)
                Return CStr(command.ExecuteScalar)
            End Using
        End Using
    End Function

    Public Shared Function CRUD(ByVal sql As String, ByVal parameterNames() As String, ByVal parameterVals() As String) As Integer
        Using connection As SqlConnection = GetDbConnection()
            Using command As New SqlCommand(sql, connection)
                FillParameters(command, parameterNames, parameterVals)
                Return command.ExecuteNonQuery()
            End Using
        End Using
    End Function

    Private Shared Sub FillParameters(ByVal command As SqlCommand, ByVal parameterNames As String(), ByVal parameterVals As String())
       If parameterNames IsNot Nothing Then
            For i = 0 To parameterNames.Length - 1
                command.Parameters.AddWithValue(parameterNames(i), parameterVals(i))
            Next
        End If
    End Sub


    Private Shared Function GetDbConnection() As SqlConnection
        Dim conString As String = ConfigurationManager.ConnectionStrings("CoachingConnectionString").ConnectionString
        Dim connection As SqlConnection = New SqlConnection(conString)
        connection.Open()
        Return connection
    End Function
End Class
share|improve this answer
 
Which ORM would you recommend?? I want something easy to learn with not a very steep learning curve. Which one would you prefer ?? Microsoft Enterprise Library 5.0 or MS Entity Framework or NHibernate. Any other suggestions? –  user1593175 Aug 26 '12 at 15:59
1  
Entity Framework. It's from Microsoft. Enterprise Library is not an ORM. –  John Saunders Aug 26 '12 at 17:30

you can create http://en.wikipedia.org/wiki/Strategy_pattern if you have more than one database to connect, so that you can dynamically change your strategy (database type like msaccess, mysql, oracle etc). I recommend this approach.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.