4
\$\begingroup\$

I have a list of tasks I get from my database (as strings) and then execute. However I'm uncertain on how to implement this correctly.

Currently I have implemented this in the following way:

Private Sub Monitoring(installationCode As String, drive As String, syncFolder As String)
        InitializeDatabaseConnection(installationCode)
        TraceInitialize(drive, "Monitoring")
        Trace.TraceInformation("Monitoring started for installation: " & installationCode)
        For Each param In JdnParamDAO.GetJdnParams(General.AmosRemoteTask)
            If param.Value Then
                RemoteTask.Deptid = Department.GetDepartmentsNotInOne().Where(Function(x) x.Code = "001").FirstOrDefault.DepartmentID

                Dim monitorTasks = TaskDefinitionDAO.GetActiveTaskDefinitionsByCategory(TaskDefinitionCategoryEnum.Monitor)

                Trace.TraceInformation(monitorTasks.Count() & " have been found.")
                'Perform each monitor task and save the results to the database
                For Each monitorTask In monitorTasks
                    Try
                        Trace.TraceInformation("MonitorTask: " & monitorTask.Description.ToLowerInvariant() & " found.")

                        Dim result = String.Empty

                        Select Case monitorTask.Description.ToLowerInvariant()
                            Case "getlastbackup"
                                result = RemoteTaskHelper.GetLastBackup(drive, installationCode)
                            Case "getamosrtversion"
                                result = General.AmosRTVersion
                            Case "getstockondummypercentage"
                                result = RemoteTaskHelper.GetStockOnDummyPercentage()
                            Case "getnumberofhistoryrecords"
                                result = RemoteTaskHelper.GetNumerOfHistoryRecords()
                            Case "getnumberofsparepartlogrecords"
                                result = RemoteTaskHelper.GetNumberOfSparePartLogRecords()
                            Case "getinconsistenciesbetweencompjobnextdueandlastdone"
                                result = RemoteTaskHelper.GetInconsistenciesBetweenCompJobNextDueAndLastDone()
                            Case "getdifferencesofcompjobnextdueandworkorderdue"
                                result = RemoteTaskHelper.GetDifferencesOfCompJobNextDueAndWorkOrderDue()
                            Case "getinconsistenciesbetweencompjobsprioandworkordersprio"
                                result = RemoteTaskHelper.GetWorkOrderCompJobPriority()
                            Case "getactivecompjobswithnextdue"
                                result = RemoteTaskHelper.GetActiveCompJobsWithNextDue()
                            Case "getactivecompjobswithoutnextdue"
                                result = RemoteTaskHelper.GetActiveCompJobsWithoutNextDue()
                            Case "getopenworkorders"
                                result = RemoteTaskHelper.GetOpenWorkOrders()
                            Case "getamosguiversion"
                                result = RemoteTaskHelper.GetAmosGuiVersion(syncFolder)
                            Case "checkaddinsactivation"
                                result = RemoteTaskHelper.CheckAddinsActivation()
                            Case "checkamosmobileactivation"
                                result = RemoteTaskHelper.CheckAmosMobileActivation()
                            Case "checkchangelogactivation"
                                result = RemoteTaskHelper.CheckChangelogActivation()
                            Case "retrievejdnmodules"
                                result = RemoteTaskHelper.RetrieveJdnModules(installationCode, syncFolder)
                            Case "retrieveamosparameters"
                                result = RemoteTaskHelper.RetrieveAmosParameters(installationCode, syncFolder)
                            Case "retrievestocksituation"
                                result = RemoteTaskHelper.RetrieveStockSituation(installationCode, syncFolder)
                            Case "executesql"
                                result = RemoteTaskHelper.ExecuteSQL(drive, installationCode)
                            Case "getfreespace"
                                result = RemoteTaskHelper.GetFreeSpace(drive)
                            Case Else
                                Trace.TraceInformation("Non excisting monitoring task: " + monitorTask.Description)
                                ExceptionMonitor.LogExceptionEvent(
                                    "Non excisting monitoring task: " + monitorTask.Description, ProblemSeverity.Error)
                        End Select

                        If Not String.IsNullOrEmpty(result) Then

                            Dim taskLog = New RemoteTaskLog()
                            taskLog.CreationDate = DateTime.Now()
                            taskLog.TaskDefinition = monitorTask
                            taskLog.SourceInstallation = Int32.Parse(installationCode)
                            taskLog.DestinationInstallation = CLng(General.AmosRTHeadOffice)
                            taskLog.AppVersion = General.AmosRTVersion
                            taskLog.TaskResult = result.ToString()

                            RemoteTaskLogDAO.Save(taskLog)
                        Else
                            Trace.TraceInformation("No result returned for monitoring task: " + monitorTask.Description)
                            ExceptionMonitor.LogExceptionEvent(
                                "No result returned for monitoring task: " + monitorTask.Description, ProblemSeverity.Error)
                        End If

                    Catch ex As Exception
                        Trace.TraceInformation("Error " + ex.ToString())
                        ExceptionMonitor.LogExceptionEvent(ex, ProblemSeverity.Error)
                    End Try
                Next
            End If
        Next
    End Sub

I am certain there is a better way to do this. I did some reading and always end up with the Strategy pattern (like asked here) but I don't seem to understand how to implement it in this situation.

Am I going in the correct direction here or not at all?

\$\endgroup\$
5
\$\begingroup\$

Important: Always set Option Explicit On

From what's an option strict and explicit?

Option Strict "restricts implicit data type conversions to only widening conversions". See [here][2]. With this option enabled, you can't accidentally convert one data type to another that is less precise (e.g. from an Integer to a Byte). Again, an option that should be turned on by default.

General

  • by using guard conditions you can save horizontal spacing which improves readability. A guard condition is a condition which guards the code from beeing executed if one ore more certain conditions are fullfilled. This is done by either returning at this point or by throwing an exception depending on which is suitable in this case.

  • extract the processing of param to a separate method.

  • extract the processing of monitorTask to a separate method.
  • extract the processing of the result to a separate method.

  • comments should describe why something is done. What is done should be described by the code itself.

  • calling ToString() on a String is not necessary.

this would lead for your Monitoringmethod to look like

Private Sub Monitoring(installationCode As String, drive As String, syncFolder As String)
        InitializeDatabaseConnection(installationCode)
        TraceInitialize(drive, "Monitoring")
        Trace.TraceInformation("Monitoring started for installation: " & installationCode)
        For Each param In JdnParamDAO.GetJdnParams(General.AmosRemoteTask)
            If Not param.Value Then continue For
            ProcessParameter(param)
        Next
End Sub

Private Sub ProcessParameter(param As someType, installationCode As String, drive As String, syncFolder As String)

    RemoteTask.Deptid = Department.GetDepartmentsNotInOne().Where(Function(x) x.Code = "001").FirstOrDefault.DepartmentID

    Dim monitorTasks = TaskDefinitionDAO.GetActiveTaskDefinitionsByCategory(TaskDefinitionCategoryEnum.Monitor)

    Trace.TraceInformation(monitorTasks.Count() & " have been found.")

    For Each monitorTask In monitorTasks
        Try
            Dim result as String = ProcessMonitorTasks(monitorTask.Description, installationCode, drive, syncFolder)
            ProcessMonitorResult(result, monitorTask, installationCode)
        Catch ex As Exception
            Trace.TraceInformation("Error " + ex.ToString())
            ExceptionMonitor.LogExceptionEvent(ex, ProblemSeverity.Error)
        End Try
    Next
End Sub

Private Function ProcessMonitorTasks(monitorTaskDescription As String, installationCode As String, drive As String, syncFolder As String) As String

    Select Case monitorTaskDescription.ToLowerInvariant()
        Case "getlastbackup"
            return RemoteTaskHelper.GetLastBackup(drive, installationCode)
        Case "getamosrtversion"
            return  General.AmosRTVersion
        Case "getstockondummypercentage"
            return RemoteTaskHelper.GetStockOnDummyPercentage()
        Case "getnumberofhistoryrecords"
            return RemoteTaskHelper.GetNumerOfHistoryRecords()
        Case "getnumberofsparepartlogrecords"
            return RemoteTaskHelper.GetNumberOfSparePartLogRecords()
        Case "getinconsistenciesbetweencompjobnextdueandlastdone"
            return  RemoteTaskHelper.GetInconsistenciesBetweenCompJobNextDueAndLastDone()
        Case "getdifferencesofcompjobnextdueandworkorderdue"
            return RemoteTaskHelper.GetDifferencesOfCompJobNextDueAndWorkOrderDue()
        Case "getinconsistenciesbetweencompjobsprioandworkordersprio"
            return RemoteTaskHelper.GetWorkOrderCompJobPriority()
        Case "getactivecompjobswithnextdue"
            return RemoteTaskHelper.GetActiveCompJobsWithNextDue()
        Case "getactivecompjobswithoutnextdue"
            return RemoteTaskHelper.GetActiveCompJobsWithoutNextDue()
        Case "getopenworkorders"
            return RemoteTaskHelper.GetOpenWorkOrders()
        Case "getamosguiversion"
            return RemoteTaskHelper.GetAmosGuiVersion(syncFolder)
        Case "checkaddinsactivation"
            return RemoteTaskHelper.CheckAddinsActivation()
        Case "checkamosmobileactivation"
            return RemoteTaskHelper.CheckAmosMobileActivation()
        Case "checkchangelogactivation"
            return RemoteTaskHelper.CheckChangelogActivation()
        Case "retrievejdnmodules"
            return RemoteTaskHelper.RetrieveJdnModules(installationCode, syncFolder)
        Case "retrieveamosparameters"
            return RemoteTaskHelper.RetrieveAmosParameters(installationCode, syncFolder)
        Case "retrievestocksituation"
            return RemoteTaskHelper.RetrieveStockSituation(installationCode, syncFolder)
        Case "executesql"
            return RemoteTaskHelper.ExecuteSQL(drive, installationCode)
        Case "getfreespace"
            return RemoteTaskHelper.GetFreeSpace(drive)
        Case Else
            Trace.TraceInformation("Non excisting monitoring task: " + monitorTask.Description)
            ExceptionMonitor.LogExceptionEvent(
                "Non excisting monitoring task: " + monitorTaskDescription, ProblemSeverity.Error)
    End Select
    return String.Empty
End Function

Private Sub ProcessMonitorResult(result As String, monitorTask As someObject, installationCode As String)

    If String.IsNullOrEmpty(result) Then
        Trace.TraceInformation("No result returned for monitoring task: " + monitorTask.Description)
        ExceptionMonitor.LogExceptionEvent(
            "No result returned for monitoring task: " + monitorTask.Description, ProblemSeverity.Error)
        Exit Sub
    End If

    Dim taskLog As RemoteTaskLog = New RemoteTaskLog()
    taskLog.CreationDate = DateTime.Now()
    taskLog.TaskDefinition = monitorTask
    taskLog.SourceInstallation = Int32.Parse(installationCode)
    taskLog.DestinationInstallation = CLng(General.AmosRTHeadOffice)
    taskLog.AppVersion = General.AmosRTVersion
    taskLog.TaskResult = result

    RemoteTaskLogDAO.Save(taskLog)

End Sub

Update:

I really don't like typing strings in code. I thought about using an Enum but I'm not entirely sure how that would work, any ideas?

This just depends on how monitorTask can/should be changed. If you want to use enums, the monitorTask will need to provide this enum too. Right now you are using the Description property which is a string.

If you want to/have to stick to the "string version" you should at least delare constants and use them in the Select..Case and whereever you need them.

\$\endgroup\$
  • \$\begingroup\$ Correct me if I'm wrong, but the reason for switching up the select-case (among other things) is because the returns are going to prevent it from checking all cases after the one that succeeds (as the original is lacking breaks), Yes? If that is the case, I figured it was worth mentioning in the off-case that the OP was not aware of this reasoning or failed to realize. \$\endgroup\$ – Shelby115 Jan 16 '15 at 16:58
  • 1
    \$\begingroup\$ In Vb.NET there is no slipping through the cases. If the desired case is found the Select will be left. \$\endgroup\$ – Heslacher Jan 16 '15 at 17:02
  • \$\begingroup\$ Thanks a lot! This has been a great help. What exactly are guard conditions? Google has lead me to AndAlso but I don't believe that's correct? What I'm still wondering is if there isn't a better way to do that big Select Case? I really don't like typing strings in code. I thought about using an Enum but I'm not entirely sure how that would work, any ideas? :) Thanks again! \$\endgroup\$ – Schoof Jan 17 '15 at 11:16
  • 1
    \$\begingroup\$ Updated answer. \$\endgroup\$ – Heslacher Jan 18 '15 at 11:38
  • 1
    \$\begingroup\$ @Thomas you may have better luck googling for "guard clause" instead ;) \$\endgroup\$ – Mathieu Guindon Jan 18 '15 at 14:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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