HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Performing actions based on command line arguments

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
performingactionsargumentslinebasedcommand

Problem

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

Solution

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

Code Snippets

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
Private Shared Sub WriteArgs(ByVal cmdArgs() As String)
    Trace.TraceInformation("Arguments: " & String.Join(" ", cmdArgs))
End Sub
Dim task = New UpgradeSybaseDatabases
With task
    .ScriptFolder = scriptFolder
End With

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

Context

StackExchange Code Review Q#79575, answer score: 4

Revisions (0)

No revisions yet.