Pau que nasce torto não cresce direito

 

Uma prática bastante comum no desenvolvimento de sistemas é o que eu chamo de CPOP [1]– “Copy & Paste Oriented Programming”.

A prática é bastante utilizada, pois fornece um template inicial a partir da qual o módulo sendo trabalhado é rapidamente reproduzido e adaptado. Com isto economiza-se tempo no desenvolvimento.

Por conta CPOP, é de suma importância que o modelo original tenha a melhor qualidade possível.

Vamos pegar um exemplo: O “Guia do Desenvolvimento” de um determinado projeto em que trabalhei que dava o seguinte modelo para a implementação das classes na camada de acesso a dados.

Friend Class AD_GrupoSerie

 

    Private Sub New()

    End Sub

 

    Public Shared Function Construtor(ByVal reader As IDataRecord) As EN_GrupoSerie

 

        If reader Is Nothing Then

            Throw New ArgumentNullException("reader")

        End If

 

        ‘ Obtém posição dos campos no data reader

        Dim iGrupoSerieIdx As Integer = reader.GetOrdinal("COD_GRUPO_SERIE")

        Dim iNomGrupoSerieIdx As Integer = reader.GetOrdinal("NOM_GRUPO_SERIE")

 

        ‘ Cria objeto da entidade de negócio

        Dim objGrupoSerie As EN_GrupoSerie = New EN_GrupoSerie()

 

        ‘ Alimenta propriedades da entidade de negócio

        objGrupoSerie.Codigo = reader.GetInt32(iGrupoSerieIdx)

 

        ‘ Para campo que pode ser nulo, é necessário atribuir valor default.

        If Not reader.IsDBNull(iNomGrupoSerieIdx) Then

            objGrupoSerie.Nome = reader.GetString(iNomGrupoSerieIdx)

        Else

            objGrupoSerie.Nome = String.Empty

        End If

 

        ‘ Retorna objeto que representa a entidade de negócio

        Return objGrupoSerie

 

    End Function

 

End Class

 

O código tem uma coisa legal que é a validação do parâmetro de entrada “reader” (isto é menos comum no código deste projeto do que deveria ser), mas o código tem oportunidades de melhoria tanto quando olhado isoladamente quando olhada no contexto em que o método é usado.

DMTCRI[2]

Primeiro vamos ao problema que considero mais relevante que é o contexto onde o método é usado: Este método é chamado pelo método AD_GrupoSerie_Acao.Selecionar dentro de um loop – uma vez para cada registro retornado do banco de dados.

Public Class AD_GrupoSerie_Acao

    Public Function Selecionar(ByVal strNome As String) As List(Of EN_GrupoSerie)

 

        Dim resultado As List(Of EN_GrupoSerie) = New List(Of EN_GrupoSerie)()

 

        Dim db As Database = DatabaseFactory.CreateDatabase(strDBNome)

 

        If db Is Nothing Then

            Throw New ArgumentNullException("db")

        End If

 

        Dim command As DbCommand = db.GetStoredProcCommand("dbo.SPPJ_SELECIONAR_GRUPOSERIE")

 

        ‘ Alimenta parâmetros da pesquisa

        db.AddInParameter(command, "P_NOM_GRUPOSERIE", DbType.String, strNome)

 

        Dim rdr As IDataReader = db.ExecuteReader(command)

 

        While rdr.Read()

            resultado.Add(AD_GrupoSerie.Construtor(rdr))

        End While

 

        Return resultado

 

    End Function

 

End Class

 

Acontece que parte das operações realizadas dentro do método Construtor avaliam coisas que são imutáveis durante o tempo de vida do loop.

Uma vez que o código entre no loop, reader nunca será Nothing e a posição das colunas nunca vai mudar. A gente está simplesmente fazendo o código repetir a se mesmo. Podemos evitar isto movendo o código que trata destas duas coisas para fora do loop.

Friend Class AD_GrupoSerie

    Private reader As IDataRecord

    Private iGrupoSerieIdx As Integer

    Private iNomGrupoSerieIdx As Integer

    Private iIndAtivoIdx As Integer

 

    Public Sub New(ByVal reader As IDataRecord)

        If reader Is Nothing Then

            Throw New ArgumentNullException("reader")

        End If

        Me.reader = reader

 

        ‘ Obtém posição dos campos no data reader

        iGrupoSerieIdx = reader.GetOrdinal("COD_GRUPO_SERIE")

        iNomGrupoSerieIdx = reader.GetOrdinal("NOM_GRUPO_SERIE")

        iIndAtivoIdx = reader.GetOrdinal("IND_ATIVO")

    End Sub

 

    Public Function Construir() As EN_GrupoSerie

        ‘ Cria objeto da entidade de negócio

        Dim objGrupoSerie As EN_GrupoSerie = New EN_GrupoSerie()

 

        ‘ Alimenta propriedades da entidade de negócio

        objGrupoSerie.Codigo = reader.GetInt32(iGrupoSerieIdx)

 

        ‘ Para campo que pode ser nulo, é necessário atribuir valor

        If Not reader.IsDBNull(iNomGrupoSerieIdx) Then

            objGrupoSerie.Nome = reader.GetString(iNomGrupoSerieIdx)

            objGrupoSerie.IndAtivo = reader.GetBoolean(iIndAtivoIdx)

        Else

            objGrupoSerie.Nome = String.Empty

        End If

 

        ‘ Retorna objeto que representa a entidade de negócio

        Return objGrupoSerie

    End Function

End Class

 

    Public Function Selecionar(ByVal strNome As String, ByVal iAtivo As Integer) As List(Of EN_GrupoSerie)

        Dim db As Database = DatabaseFactory.CreateDatabase(strDBNome)

 

        Using command As DbCommand = db.GetStoredProcCommand("dbo.SPPJ_SEL_GRUPO_SERIE")

            db.AddInParameter(command, "P_NOM_GRUPOSERIE", DbType.String, strNome)

            db.AddInParameter(command, "P_IND_ATIVO", DbType.Int32, iAtivo)

 

            Dim resultado As List(Of EN_GrupoSerie) = New List(Of EN_GrupoSerie)()

            Using reader As IDataReader = db.ExecuteReader(command)

                Dim factory As New AD_GrupoSerie(reader)

                While reader.Read()

                    resultado.Add(factory.Construir())

                End While

            End Using

            Return resultado

        End Using

    End Function

As operações foram movidas para o construtor de AD_GrupoSerie que está fora do loop em AD_GrupoSerie_Acao.Selecionar e com isto não se tem o custo de verificação do parâmetro por nulo e a localização da posição das colunas para cada linha do resultset.

Dado ao fato do pessoal do projeto usar CPOP a torto e a direito, o mesmo erro foi replicado em dezenas de locais diferentes. Sair consertando tudo é inviável financeiramente, então é importante revisar no detalhe qualquer código que possa vir a ser utilizado como original (template) para o CPOP.


[1] Ao procurar o verbete para DRY no Wikipedia, eu acabei encontrando uma entrada para “Copy and paste programing” que trata justamente do que eu vinha chamando de CPOP – Copy & Paste Oriented Programming. Eu ainda acho CPOP mais legal por conta da alusão a OOP.

Por falar em OOP, a idéia não é fazer uma apologia ao CPOP. Se o cenário permitir, DRY nele!

[2] DMTCRI – Don’t Make The Code Repeat Itself é uma alusão a DRY – Don’t Repeat Yourself, uma filosofia de programação que prega a redução da duplicação de código. Aqui a idéia que quero passar é a de se evitar fazer com que o programa execute repetidas vezes operações que sempre trarão os mesmos resultados.

You probably shouldn’t be using ReDim Preserve inside a loop

These days I was doing a code review when I went across a method which contained a ReDim Preserve inside a loop.

I can’t show the real code because it is from a customer’s code base, but here’s a simplification of what I found:

    Sub CopyAndPasteOrientedProgramming()

        Dim c As New Customer()

        For i As Integer = 0 To 250000

            ReDim Preserve c.Orders(c.Orders.Length + 1)

            c.Orders(c.Orders.Length – 1) = New Order()

        Next

        Console.WriteLine(c.Orders.Length)

    End Sub

 

I found the developer who checked in the code and went on to tell him to use a List(Of Order) and he replied “Oh, don’t bother with that. It’s just a piece of code that I copied from a similar class and since it doesn’t have a business rule for adding the elements, I took off the If statement.

Wait a minute, I said. You’re telling me there’s more code like this scattered throughout the code base?

Yup, he said.

So to simplify, the business rule I’ll use will be the order being a multiple of 3:

    Sub ShowRedim()

        Dim c As New Customer()

        For i As Integer = 0 To 250000

            If i Mod 3 = 0 Then

                ReDim Preserve c.Orders(c.Orders.Length + 1)

                c.Orders(c.Orders.Length – 1) = New Order()

            End If

        Next

        Console.WriteLine(c.Orders.Length)

    End Sub

 

The problem with the code is that for each iteration of the loop, a new array will be created with one more element than earlier and the previous array will be copied into the new one. Yeap… It’s going to create 250,000 arrays on the first sample and 83,334 on the second. Poor Garbage Collector!

A smarter decision would be to use a generic List. If you don’t initialize its capacity, it’ll default to 16 when you add the first element. Then it will double in size each time it reaches its capacity. I’m too lazy to do the math but I guarantee you that it’s far less than the previous example.

    Sub ShowList()

        Dim c As New Customer()

        Dim o As New List(Of Order)

        For i As Integer = 0 To 250000

            If i Mod 3 = 0 Then

                o.Add(New Order)

            End If

        Next

        c.Orders = o.ToArray()

        Console.WriteLine(c.Orders.Length)

    End Sub

 

This is a very simple alternative. If you really want to be clever and you have an idea of the percentage of orders that will fulfill the criteria, you can initialize the List with a meaningful capacity.

So please stop using ReDim Preserve inside loops!!!

 

 

PS. List(Of T).ToArray() creates a new array, but when compared with the original code that doesn’t hurt, does it?

Keep it simple (and help to save the planet)

Today I came across a piece of code like the following.

Dim strSQL As New StringBuilder
strSQL.Append("SELECT ")
strSQL.Append(" ProductID, ")
strSQL.Append(" Name ")
strSQL.Append("FROM Production.Product")

Try
    cnnConn.Open()
    Dim cmd As New SqlCommand(strSQL.ToString, cnnConn)

When I encounter code I don’t agree with, I try to understand what made the developer code it that way.

Looking at this one I can only presume:

  1. He wanted to make his code more legible by separating the SQL Command to different lines.
  2. In doing that, he wanted to avoid string concatenation, because "string concatenation is performance wise evil"!

My answers to these points are:

  1. In this particular case, the code became harder to read than if it was stated in a single line.

    Dim strSQL As String = "SELECT ProductID, Name FROM Production.Product"

  2. That may be true in a lot of situations, but is not true in this particular one: concatenating small string constants, as the compiler is smart enough to generate one single string constant comprised of what we see in code as being multiple strings being concatenated

    Dim strSQL As String = _
    "SELECT " + _
        " ProductID, " + _
        " Name " + _
        "FROM Production.Product"

    Try
        cnnConn.Open()
        Dim cmd As New SqlCommand(strSQL, cnnConn)

    gets compiled to

    L_001f: ldstr "SELECT  ProductID,  Name FROM Production.Product"
    L_0024: stloc.1
    L_0025: nop
    L_0026: ldloc.2
    L_0027: callvirt instance void [System.Data]System.Data.SqlClient.SqlConnection::Open()
    L_002c: nop
    L_002d: ldloc.1
    L_002e: ldloc.2
    L_002f: newobj instance void [System.Data]System.Data.SqlClient.SqlCommand::.ctor(string, class [System.Data]System.Data.SqlClient.SqlConnection)

 

* Q: So what does this post has to do with saving the planet?

A: Keep it simple -> spend less processor cycles -> spend less energy -> … -> … ->…. Oooh… you get it!

Lisa Feigenbaum & Amanda Silver on Codificando .NET e-magazine

This week, Codificando .NET released the November’s issue of their e-magazine.

It brings an interview I did back in September when I was at the Microsoft Campus in Redmond.

We talked about their careers and about features coming in VB 2005 and got a glimpse on what may show up on post Orcas.

The magazine is in Portuguese, but you can see the original video in English here:

 

 

Bring on the VB Ladies!