4
\$\begingroup\$

I'm currently reviewing a bit of an older console application which performs certain tasks depending on the command line argument that is given. These tasks are called through Windows Task Scheduler.

However reflecting on this I find the code to be ugly and I'm wondering if there is a better way to do something like this.

Public Shared Sub ParseTasks(ByVal CmdArgs() As String)
        ConvertAndWriteArgs(CmdArgs)

        Try
            Select Case CmdArgs(0)
                Case "SCRIPTS"
                    'SCRIPTS has 1 input parameter: ScheduleType
                    If CmdArgs.Length < 2 Then
                       ExceptionLogger.LogExceptionEvent("Invalid Arguments", Severity.Error)
                    Else
                        Scripts(sheduleType:=CmdArgs(1).ToString)
                    End If
            Case "CHECKTRANSFERREPLICATION"
                CheckTransferReplication()
            Case "CREATESYBASEBACKUP11"
                CreateSybaseBackup11()
            Case "SQLCOMMAND"
                SqlCommand()
            Case "REPLICATION_IN"
                'Replication In has 1 input parameter: Range
                If CmdArgs.Length < 2 Then
                    ExceptionLogger.LogExceptionEvent("Invalid Arguments", Severity.Error)
                Else
                    ReplicationImport(range:=CmdArgs(1).ToString())
                End If
            Case "REPLICATION_OUT"
                'Replication Out has 1 input parameter: Range
                If CmdArgs.Length < 2 Then
                    ExceptionLogger.LogExceptionEvent("Invalid Arguments", Severity.Error)
                Else
                    ReplicationExport(Range:=CmdArgs(1).ToString)
                End If
            Case "REEXPORTFILE"
                'Re export file Out has 1 input parameter: FileName
                If CmdArgs.Length < 2 Then
                    ExceptionLogger.LogExceptionEvent("Invalid Arguments", Severity.Error)
                Else
                    ReExportFile(fileName:=CmdArgs(1).ToString)
                End If
            Case "UPGRADE"
                'Upgrade has 1 input parameter: ScriptFolder
                If CmdArgs.Length < 2 Then
                    ExceptionLogger.LogExceptionEvent("Invalid Arguments", Severity.Error)
                Else
                    Upgrade(scriptFolder:=CmdArgs(1).ToString)
                End If
            Case "DUPLICATES"
                Duplicates()
            Case Else
                ExceptionLogger.LogExceptionEvent("Invalid Arguments", Severity.Error)
            End Select

        Catch ex As Exception
           ExceptionLogger.LogExceptionEvent(ex, Severity.Error)
        End Try
    End Sub


#Region "Task functions"

    Private Shared Sub AutoScripts(ByVal sheduleType As String)
        Trace.TraceInformation("Creating AutoScripts Task.")

        Dim task = New AutoScripts
        With task
            .ScheduleType = ConvertScheduleType(sheduleType)
        End With

        Trace.TraceInformation("Schedule Type: " & task.ScheduleType.ToString)

        task.Start()
    End Sub

    Private Shared Sub CheckTransferReplication()
        Trace.TraceInformation("Creating CheckTransferReplication Task.")

        Dim task = New CheckTransferReplication
        task.Start()
    End Sub

    Private Shared Sub CreateSybaseBackup11()
        Trace.TraceInformation("Creating CreateSybaseBackup11 Task.")

        Dim task = New CreateSybaseBackup
        task.Start()
    End Sub

    Private Shared Sub SqlCommand()
        Trace.TraceInformation("Creating SqlCommand Task.")

        Dim task = New Business.Task.ExecuteSqlCommand
        task.Start()
    End Sub

    Private Shared Sub ReExportFile(ByVal fileName As String)
        Trace.TraceInformation("Creating ReExportFile Task.")

        Dim task = New ReExportFile
        With task
            .FileName = fileName
        End With

        task.Start()
    End Sub

    Private Shared Sub Upgrade(ByVal scriptFolder As String)
        Trace.TraceInformation("Creating UpgradeSybaseDatabases Task.")

        Dim task = New UpgradeSybaseDatabases
        With task
            .ScriptFolder = scriptFolder
        End With

        task.Start()
    End Sub

#Region "Replication"

    Private Shared Sub ReplicationImport(ByVal range As String)
        Trace.TraceInformation("Creating ReplicationImport Task.")

        Dim task = New ExecuteReplication
        task.ReplicationType = Business.ReplicationType.Import

        If range.Equals("001", StringComparison.OrdinalIgnoreCase) Then
            task.ReplicationGroup = Business.ReplicationGroup.HeadOffice
            task.Start()

            Try
                EdimarObjectDAO.ProcessRefErrors()
            Catch ex As Exception
                ExceptionLogger.LogExceptionEvent(ex, ProblemSeverity.Error)
            End Try

            Dim sqlCommandTask = New ExecuteSqlCommand
            SqlCommandTask.Start()
        Else
            task.ReplicationGroup = Business.ReplicationGroup.Sattelite
            task.ReplicationSatteliteRange = range

            task.Start()
        End If
    End Sub

    Private Shared Sub ReplicationExport(ByVal range As String)
        Trace.TraceInformation("Creating ReplicationExport Task.")

        Dim task = New ExecuteReplication
        task.ReplicationType = Business.ReplicationType.Export

        If Range.Equals("001", StringComparison.OrdinalIgnoreCase) Then
            task.ReplicationGroup = Business.ReplicationGroup.HeadOffice
            task.Start()
        Else
            task.ReplicationGroup = Business.ReplicationGroup.Sattelite
            task.ReplicationSatteliteRange = Range

            task.Start()
        End If
    End Sub

    Private Shared Sub ProcessLogFiles()
        Trace.TraceInformation("Processing Replication Logfiles.")

        Dim task = New ProcessLogFiles
        task.Start()
    End Sub

#End Region

#End Region

#Region "Helper Functions"

    Private Shared Sub ConvertAndWriteArgs(ByVal CmdArgs() As String)
        Dim strArgs = String.Empty
        For Each arg In CmdArgs
            If strArgs = String.Empty Then
                strArgs = arg
            Else
                strArgs += " " & arg
            End If
        Next

        Trace.TraceInformation("Arguments: " & strArgs)
    End Sub

    Private Shared Function ConvertScheduleType(ByVal scheduleType As String) As Business.ScheduleType
        Dim schedule As Business.ScheduleType
        Select Case ScheduleType
            Case "M"
                schedule = Business.ScheduleType.Monthly
            Case "W"
                schedule = Business.ScheduleType.Weekly
            Case "W2"
                schedule = Business.ScheduleType.Weekly2
            Case "D"
                schedule = Business.ScheduleType.Daily
            Case "D2"
                schedule = Business.ScheduleType.Daily2
            Case Else
                ExceptionLogger.LogExceptionEvent("Invalid Arguments", ProblemSeverity.Error)
        End Select
        Return schedule
    End Function

#End Region
\$\endgroup\$
4
\$\begingroup\$

I don't see any issues with your switch itself. There will only ever be a finite number of arguments to be handled and this seems like a reasonable way to go about it. What I don't like is how you've repeated the check on CmdArgs.Length all over the code. (I do like your comments though. They've really clarified unclear things.)

I would recommend extracting the logic into a routine that takes a delegate function. You could declare it something like this.

Delegate Sub ExecuteRoutine(range As String)

And gets used in another (poorly named) method.

Private Sub CheckArgCountAndExecute(cmdArgs() As String, toExecute As ExecuteRoutine)
    If cmdArgs.Length < 2 Then
        ExceptionLogger.LogExceptionEvent("Invalid Arguments", Severity.Error)
    Else
        toExecute(range:=CmdArgs(1).ToString())
    End If
End Sub

And gets called in your switch like this.

Case "REPLICATION_IN"
    'Replication In has 1 input parameter: Range
    CheckArgCountAndExecute(CmdArgs(1), AddressOf ReplicationImport)
Case "REPLICATION_OUT"
    'Replication Out has 1 input parameter: Range
    CheckArgCountAndExecute(CmdArgs(1), AddressOf ReplicationExport)

You can read more about delegate functions here.


I agree with @Heslacher about this particular With statement, but I think it's a bit foolish to say that all Withs are bad. It's just senseless to use it for what could be one line of code.

    With task
        .ScheduleType = ConvertScheduleType(sheduleType)
    End With

That is much better as

task.ScheduleType = ConvertScheduleType(scheduleType)

However, I would say it would be valid to switch over to one here.

Private Shared Sub ReExportFile(ByVal fileName As String)
    Trace.TraceInformation("Creating ReExportFile Task.")

    Dim task = New ReExportFile
    With task
        .FileName = fileName
    End With

    task.Start()
End Sub

VS

Private Shared Sub ReExportFile(ByVal fileName As String)
    Trace.TraceInformation("Creating ReExportFile Task.")

    Dim task = New ReExportFile
    With task
        .FileName = fileName
        .Start()
    End With
End Sub
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks a lot! The delegate functions are really quite useful. :) I like both the answers on my question, but I can only choose one, this is difficult! \$\endgroup\$ – Schoof Feb 11 '15 at 8:30
  • \$\begingroup\$ You're welcome! I find them a handy screwdriver for the toolbox too! Glad I could help. \$\endgroup\$ – RubberDuck Feb 11 '15 at 10:01
4
\$\begingroup\$

Based on the naming guidelines input parameters should be named using camelCase casing.


Calling ToString() on a String doesn't really add any value. A String is a String is a String.


You have a lot of duplicated code here which can be reduced by extracting the cases of single argument calls to a separate method like

Public Shared Sub ParseTasks(ByVal cmdArgs() As String)
    ConvertAndWriteArgs(cmdArgs)

    if CmdArgs.Length = 1 Then
        ParseTasks(cmdArgs(0))
        Exit Sub
    End if
    Dim argument as String = cmdArgs(1)
    Try
        Select Case cmdArgs(0)
            Case "SCRIPTS"
                Scripts(sheduleType:=argument)

            Case "REPLICATION_IN"
                ReplicationImport(range:=argument)

            Case "REPLICATION_OUT"
                ReplicationExport(Range:=argument)

            Case "REEXPORTFILE"
                ReExportFile(fileName:=argument)

            Case "UPGRADE"
                Upgrade(scriptFolder:=argument)

            Case Else
                ExceptionLogger.LogExceptionEvent("Invalid Arguments", Severity.Error)
        End Select

    Catch ex As Exception
       ExceptionLogger.LogExceptionEvent(ex, Severity.Error)
    End Try
End Sub


Private Shared Sub ParseTasks(ByVal argument As String)
    Try
        Select Case argument 
            Case "CHECKTRANSFERREPLICATION"
                CheckTransferReplication()
            Case "CREATESYBASEBACKUP11"
                CreateSybaseBackup11()
            Case "SQLCOMMAND"
                SqlCommand()
            Case "DUPLICATES"
                Duplicates()
            Case Else
                ExceptionLogger.LogExceptionEvent("Invalid Arguments", Severity.Error)
            End Select
    Catch ex As Exception
       ExceptionLogger.LogExceptionEvent(ex, Severity.Error)
    End Try
End Sub

The ConvertAndWriteArgs() method doesn't do any converting. It just adding spaces between the arguments and then writing the result to trace.

Instead of using this loop you could use the String.Join() method.

Private Shared Sub WriteArgs(ByVal cmdArgs() As String)
    Trace.TraceInformation("Arguments: " & String.Join(" ", cmdArgs))
End Sub  

Using the good old with shouldn't be done nowadays. If you have the feeling you should use it, don't do it if it isn't needed like this

Dim task = New UpgradeSybaseDatabases
With task
    .ScriptFolder = scriptFolder
End With

task.Start()

this would be much more readable like

Dim task = New UpgradeSybaseDatabases
task.ScriptFolder = scriptFolder
task.Start()  

If you want to initialize some public properties when you are creating the object, you can use the "new" way of using With like

Dim task = New UpgradeSybaseDatabases With { .ScriptFolder = scriptFolder }
tast.Start()

The ReplicationImport() and ReplicationExport() methods share a lot of duplicate code. You better extract this duplication to a separate method which is called by these methods.

Private Shared Sub ExecuteReplication(ByVal range As String, ByVal replicationType As Business.ReplicationType)  

    Dim task = New ExecuteReplication
    task.ReplicationType = replicationType 

    If range.Equals("001", StringComparison.OrdinalIgnoreCase) Then
        task.ReplicationGroup = Business.ReplicationGroup.HeadOffice
    Else
        task.ReplicationGroup = Business.ReplicationGroup.Sattelite
        task.ReplicationSatteliteRange = range
    End If

    task.Start()

End Sub  

Which is then called like

Private Shared Sub ReplicationExport(ByVal range As String)
    Trace.TraceInformation("Creating ReplicationExport Task.")

    ExecuteReplication(range, Business.ReplicationType.Export)
End Sub

and

Private Shared Sub ReplicationImport(ByVal range As String)
    Trace.TraceInformation("Creating ReplicationImport Task.")

    ExecuteReplication(range, Business.ReplicationType.Import)

    If range.Equals("001", StringComparison.OrdinalIgnoreCase) Then

        Try
            EdimarObjectDAO.ProcessRefErrors()
        Catch ex As Exception
            ExceptionLogger.LogExceptionEvent(ex, ProblemSeverity.Error)
        End Try

        Dim sqlCommandTask = New ExecuteSqlCommand
        SqlCommandTask.Start()
    End If
End Sub
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot! That is already a great help. I wish I could see all these things, are there any books you can recommend that help with this or does it mostly come with experience? \$\endgroup\$ – Schoof Feb 4 '15 at 15:54
  • 3
    \$\begingroup\$ A good book is Clean Code by Robert C.Martin. But most is coming from doing ;-) and also beeing active here on code review. \$\endgroup\$ – Heslacher Feb 4 '15 at 15:57
  • \$\begingroup\$ There's enough good stuff here that you got my upvote, but I'm not a fan of how the switch is spread across two different methods. \$\endgroup\$ – RubberDuck Feb 4 '15 at 16:20

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.