Search code examples
vb.netoptimizationrefactoringclone

VB.NET "Fixing" Code Clone


I'm working on refactoring a decently large project, and I'm interested in searching out and reducing Code Clone for better standardization as well as ease of development.

I've got a code snippet that keeps coming up in "Exact Matches" (using Visual Studio 2012's "Find Code Clones" feature).

Here it is:

                End If
            End Using
        Catch ex As Exception
            MsgBox(ex.Message)
        End Try
    End Using
End Using
Return Nothing

Basically, I have a whole host of similar functions (similar in structure but not in actual function) that all open with something like this:

Const sql As String = ...
Using cn As SqlConnection...
    Using cmd As SqlCommand(sql,cn)
        ... Maybe add some SqlParameters
        cn.Open()
        ... Something with cmd.Execute...

Now, I recognize that the first code block is IDENTICAL across many, many methods, but I can't figure out a way to pull that code out, write it once and simply call it each time I need that functionality. It seems to me that there is too much control flow occurring.

So, I'm stumped as to how to fix this (and I'm only assuming that I can "fix" this because Microsoft has identified it as "cloned code").

I'm thinking of something along the lines of making a small number of functions that do the same type of thing (like returning a count from a table, returning a top value, etc...) and only really differ by the SQL that is executed. That could be a bit tricky, though, since sometimes the parameters (type and number) differ.

Any thoughts?


Solution

  • I wouldn't concern yourself with those. Your common-sense first impression is correct. While, technically speaking, the syntax is repeated multiple times, it is not really a repetition of logic or an algorithm of any kind. That's not to say there's no way to reduce that repetition, it's just that by doing so, you will likely end up with a worse design in your code.

    For instance, you could create a single method that does all the setup and tear-down and then just calls a method in the middle that actually uses the connection to do the work, such as:

    Public Function PerformDbTask(task As IMyDbTask)
        Using cn As SqlConnection...
            Using cmd As SqlCommand = cn.CreateCommand()
                Try
                    cn.Open()
                    task.Perform(cmd)
                Catch ex As Exception
                    MsgBox(ex.Message)
                End Try
            End Using
        End Using
        Return Nothing
    End Function
    

    However, what have you really gained? Probably not much at all, yet you've lost a lot of flexibility. So, unless that kind of design is actually necessary for what you are trying to do, I wouldn't waste time trying to solve a problem that doesn't exist.