3
\$\begingroup\$

The company I am with has a lot of old code migrated over from ALPHA Basic, written back in the 1980s. As development languages have evolved (should I say, "died off"?) the code has been migrated to newer languages.

The company also hires a lot of new graduates from a local college. Sometimes, the code is written in a textbook fashion. I see a lot of code that could be written differently.

Here is one piece of code that I have encountered in a project I maintain:

Public ReadOnly Property HasModifiedRows() As Boolean
    Get
        Dim dt As DataTable

        dt = Nothing
        Try
            dt = Me.GetTableObj.GetChanges(DataRowState.Modified)
            If dt IsNot Nothing Then
                Return True
            Else
                Return False
            End If
        Finally
            If dt IsNot Nothing Then
                dt.Dispose()
                dt = Nothing
            End If
        End Try
    End Get
End Property

The code looks deliberately spelled out, step for step. This could be to help our new developers understand what is going on, or it could be code that was migrated over from older languages that required all variables be defined at the top.

I do not know why, but seeing all of the unnecessary lines of code is like fingernails on a chalkboard.

My planned rewrite

I am constantly compelled to rewrite pieces of code like that above to this:

Public ReadOnly Property HasModifiedRows() As Boolean
    Get
        Using table As DataTable = GetTableObj.GetChanges(DataRowState.Modified)
            Return True
        End Using
        Return False
    End Get
End Property

Question and concerns

  • Is one design pattern better than another?
  • Is there a performance difference in the two?
  • Would my version with the using syntax have any unanticipated results?

As I understand the VB Using Statement, if an object enters a using block, it cannot be null and it will always dispose of itself whenever it goes out of scope. I could of course be wrong though. I do not know how to guarantee this in .Net.

Should I leave the old code alone?

If there is any benefit to rewriting it, I am certainly happier with the streamlined look.

I'd really like to get some opinions on if I am doing any good by changing the code. If I am doing harm, I certainly would like to know!

\$\endgroup\$
1
\$\begingroup\$

Is one design pattern better than another?

Every code statement is a potential defect. If you can express yourself in a shorter form without sacrificing readability, then by all means you should. The less code, the less to read and debug, the fewer suspicious elements.

Is there a performance difference in the two?

I don't actually know VB, but this is a common pattern in many languages, and it's clear that the performance should be the same, or the second might be even faster, thanks to some optimization magic by the compiler.

Would my version with the using syntax have any unanticipated results?

Increased understanding among readers? You changed not just the writing style, but you also gave the variables more meaningful names. It's intuitive and a lot easier to read than the first one. Both seem to accomplish exactly the same thing.

Should I leave the old code alone?

Probably. Don't fix what ain't broken. Although the original method is not implemented pretty, from the outside it doesn't really matter: it has a good name, and it does exactly what the name says and nothing else, it works. It's nice to make it nicer, but there isn't really a need. Now that you looked at it closely and know that it works, try to treat it as a black box, and remember not to open again. Use your energy for more creative tasks.

\$\endgroup\$
  • \$\begingroup\$ It's a black box and leave it alone. I'll need to remember that. :) \$\endgroup\$ – jp2code Jun 3 '15 at 13:49

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.