Business Layer advice needed

[Solarion]

Is there another way to get away from this Case statement? Just don't like it.

Business Layer

    Public Function GetDiscById(ByVal disc As String, ByVal progCode As Integer) As DataTable
        Dim script As String = ""

        Select Case disc
            Case "Ford"
                script = Scripts.SqlGetCategoryOne
            Case "Opel"
                script = Scripts.SqlGetCategoryTwo
            Case "BMW"
                script = Scripts.SqlGetCategoryThree
            End Select
        Dim dataTable As New DataTable
        Dim db As New Data.DayCategoryAccess
        dataTable = db.GetData(progCode, script)

        Return dataTable
    End Function

Data Layer

    Public Function GetData(ByVal progCode As Integer, ByVal sqlScript As String) As DataTable
        Dim dataTable As New DataTable

        Using sqlDataAdapter As New SqlDataAdapter
            sqlDataAdapter.SelectCommand = New SqlCommand
            sqlDataAdapter.SelectCommand.Connection = ConnectionAccess.GetConnection()
            sqlDataAdapter.SelectCommand.CommandType = CommandType.Text
            sqlDataAdapter.SelectCommand.CommandText = sqlScript
            sqlDataAdapter.SelectCommand.Parameters.AddWithValue("@ProgCode", progCode)
            sqlDataAdapter.Fill(dataTable)

            Return dataTable
        End Using
    End Function

 

[patrick123]

Possibly store your scripts as a collection then it would be a simple matter of:

    script = Scripts.SqlGet( disc)

 

[Solarion]

Possibly store your scripts as a collection then it would be a simple matter of:

    script = Scripts.SqlGet( disc)

Oh I like that!

 

[Webnoob]

use a dictionary becuase the lookup is O(1)

 

[Hamster]

...or stick to the case statement because it is the right tool for the job? Why make your code less readable?

 

[FarligOpptreden]

...or stick to the case statement because it is the right tool for the job? Why make your code less readable?

Because it will be more extensible? What happens when more options become available, add more case statements to the switch?

Consider adding the scripts to a config file for your application, that way you don't have to write more code when new options are added. Just add the options in the config file and off you go. Consider caching the options in memory on application startup to minimise disk IO and setting the file as a cache dependency so the cache refreshes when the file changes.

 

[Solarion]

Because it will be more extensible? What happens when more options become available, add more case statements to the switch?

Consider adding the scripts to a config file for your application, that way you don't have to write more code when new options are added. Just add the options in the config file and off you go. Consider caching the options in memory on application startup to minimise disk IO and setting the file as a cache dependency so the cache refreshes when the file changes.

That's exactly why it just doesn't feel and look right. It's too "permanent" for my liking. I will investigate your suggestions thanks a million.

 

[[)roi(]]

A simple array of enum cases with associated values would have sufficed, but c# or vb.net don't support that yet. F# however does have something similar with discriminating unions.

The need is however quite common, lookup examples for the Factory Pattern.

 

[Hamster]

Because it will be more extensible? What happens when more options become available, add more case statements to the switch?

Consider adding the scripts to a config file for your application, that way you don't have to write more code when new options are added. Just add the options in the config file and off you go. Consider caching the options in memory on application startup to minimise disk IO and setting the file as a cache dependency so the cache refreshes when the file changes.

His example looks like practice work. You wouldn't hardcode "BMW".

 

[Hamster]

If this is a production system you'd have those car makes in the database and have their category associated with them there. It's difficult to say exactly what Mr BeerIsAwesomesauce is trying to do here.

 

[Noah]

...or stick to the case statement because it is the right tool for the job? Why make your code less readable?

*4 years time looks back at readable code and sees 500 case statements*

 

[Solarion]

Public Function GetDiscById(ByVal script As String, ByVal progCode As Integer) As DataTable
      
        Dim dataTable As New DataTable
        Dim db As New Data.DayCategoryAccess
        dataTable = db.GetData(progCode, script)

        Return dataTable
    End Function

I just removed the selects and depending on category I'm trying to retrieve, I just pass that in directly.

Private Sub GetCategoryOne()
        Dim db as new Data.Scripts
        Dim dt as new DataTable

        dt = GetDiscById(db.SqlGetCategoryOne, progCode)
End Sub

 

[Hamster]

*4 years time looks back at readable code and sees 500 case statements*

Like I said, it looks like practice code. You would never hardcode car names like that or call your procs [something]One/Two/Three

 

[Solarion]

Like I said, it looks like practice code. You would never hardcode car names like that or call your procs [something]One/Two/Three

It is practice yes.

 

[gkm]

...
Consider adding the scripts to a config file for your application, that way you don't have to write more code when new options are added. ....

Please remember that configuration is also code, often just code that is harder to test than real code. I would recommend to only put stuff in configuration that is different between different environments (dev/qa/prod) and rather keep you code in code to make it easier to read, rather than fragment the code over code and config, causing lots of paging between files to figure out what something does.

 

[Webnoob]

Please remember that configuration is also code, often just code that is harder to test than real code. I would recommend to only put stuff in configuration that is different between different environments (dev/qa/prod) and rather keep you code in code to make it easier to read, rather than fragment the code over code and config, causing lots of paging between files to figure out what something does.

^^ This....

 

[Solarion]

Please remember that configuration is also code, often just code that is harder to test than real code. I would recommend to only put stuff in configuration that is different between different environments (dev/qa/prod) and rather keep you code in code to make it easier to read, rather than fragment the code over code and config, causing lots of paging between files to figure out what something does.

Yup that is great advice. Splitting code up into layer upon layer is what I'm trying to avoid. 3 layers must do the job, no more. If I'm doing something and it's not working then I'm probably doing it wrong.

 

[Hamster]

It is practice yes.

Called it!

 

[Solarion]

One more thing, parameters. Should I keep them in the business layer?

Business Layer

 Public Function EditUpdateLog(ByVal query As String, ByVal info As SupportObject)

        Dim sqlParameters As SqlParameter() = New SqlParameter(10) {}

        sqlParameters(0) = New SqlParameter("@Id", SqlDbType.VarChar)
        sqlParameters(0).Value = info.Id
        sqlParameters(1) = New SqlParameter("@StartTime", SqlDbType.VarChar)
        sqlParameters(1).Value = info.StartTime
        sqlParameters(2) = New SqlParameter("@EndTime", SqlDbType.VarChar)
        sqlParameters(2).Value = info.EndTime
        conn.EditUpdate(query, info, sqlParameters)
        Return Nothing
    End Function

 

[Hamster]

Why do you have SQL stuff in your Business Layer and not your Data Layer?