Melhoria do código do ACBr com o uso da ferramenta FixInsight

[EMBarbosa]

Foto por David Siglin em Unsplash.

 

Olá pessoal,

 

   É bom quando encontramos uma ferramenta que facilita ou melhora nosso trabalho, não?

 

   Todos devem ter notado que ultimamente temos enviado vários commits ao SVN de remoção de warnings e hints, muitas vezes mencionando a ferramenta FixInsight. Para quem não conhece, essa ferramenta faz uma análise do seu código e aponta possíveis erros e sugere otimizações. Ela é uma ferramenta muito boa, tanto que foi comprada pela TMS e se tornou TMS FixInsight.

   Já tem um tempo que conheço a ferramenta e sempre tive o desejo de rodá-la em todo o código do ACBr. Mas devido ao tempo não tinha sido possível. Depois de um incentivo (valeu @Waldir Paim), eu resolvi baixar a versão trial e fazer isso. E que bom que fiz.

   Gostaríamos de compartilhar com vocês algumas coisas que encontramos no nosso código com a ajuda dessa ferramenta.

 

Encontrando pequenos problemas num código gigante

 

   Vamos começar por um código que estava no ACBrValidador. Vejam esse código, onde a função ValidarCEP de baixo chama a função ValidarCEP de cima, e tente encontrar um problema:

function ValidarCEP(const ACEP, AUF: String): String;
begin
  Result := ValidarDocumento( docCEP, ACEP, AUF);
end;
function ValidarCEP(const ACEP: Integer; AUF: String): String; 
begin 
  ValidarCEP( FormatarCEP(ACEP), AUF ); 
end;

Conseguiu ver o problema? Essa função nunca retornaria que um CEP é inválido se você passasse o CEP como inteiro. Precisava de um “Result := ” no início.

Simples? Nem tanto quando lembramos do tamanho do projeto ACBr. Temos mais de 200 componentes e mais de 779 mil linhas de código, contribuídos por dezenas ou talvez centenas de programadores, embora a nossa equipe de commiters seja realmente pequena. Só a unit ACBrValidador.pas em questão tem atualmente cerca de 2070 linhas.

Não fica muito mais fácil quando uma ferramenta aponta pra você?

[FixInsight Warning] ACBrValidador.pas(294): W521 Return value of function 'ValidarCEP' might be undefined

 

 

   Vamos a outro exemplo no pacote ACBrSerial, componente ACBrECF:

[FixInsight Warning] ACBrECFDaruma.pas(4638): W503 Assignment right hand side is equal to its left hand side

Veja o código (só a parte interessante):

  else if StrToIntDef(fsNumVersao, -1) >= 345 then
  begin
    RetCmd := EnviaComando( ESC + #240 );
    RetCmd := Copy(RetCmd, 92, Length(RetCmd));
    RetCmd := RetCmd;   //<--- Viu aqui???
    for A := 0 to fpAliquotas.Count-1 do
    begin
      fpAliquotas[A].Total := RoundTo( StrToFloatDef(Copy(RetCmd,(A*14)+1,14),0)
                                         / 100, -2 );
    end;
  end;
end;

   Uma linha que não faz absolutamente nada a não ser gastar espaço, memória e CPU.  

   Uma linha desnecessária a menos no código.

 

   E você consegue encontrar um no seu aplicativo código que nunca será executado? Ainda no mesmo pacote, veja esse exemplo: 

[FixInsight Warning] ACBrECFDataRegis.pas(1838): W509 Unreachable code

Nesse código:

  if (fsArqPrgBcoTXT <> '') and (not FileExists( fsArqPrgBcoTXT )) then
  begin
     Msg := ACBrStr( 'Arquivo '+fsArqPrgBcoTXT+' não encontrado. '+
                     'Valores padrões serão utilizados.' ) ;
     raise EACBrECFErro.Create( Msg );

     fsArqPrgBcoTXT := '' ; //Essa linha nunca vai ser executada porque tem um raise acima.
  end ;

   Mais uma vez, tente imaginar procurar esse problema num projeto tão grande. Não é facilmente percebido se você não tiver olhos treinados e estiver procurando problemas.

 

   Vamos a outro exemplo ainda no componente ACBrECF: 

   [FixInsight Warning] ACBrECFEscECF.pas(1222): W517 Variable 'CHK' hides a class field, method or property 

   Veja esse código:

procedure TACBrECFEscECFResposta.SetResposta(const AValue: AnsiString);
Var
  Soma, I, F, LenCmd : Integer ;
  CHK  : Byte ;
begin

   O problema desse código é que ele confunde uma variável local (CHK) com uma propriedade da classe (TACBrECFEscECFResposta.CHK). É preciso analisar todo código em cada lugar que isso acontece para ter certeza quando você está se referindo a propriedade e quando é a variável. Imagine se você confunde uma com a outra. Uma hora você pensa que sua variável está recebendo valores estranhos. Outra hora você pensa que sua propriedade não está sendo atualizada.

   Nesse caso específico, a variável foi renomeada para vCHK evitando a confusão com a propriedade CHK. O importante é que quando você for ler o código, não precise ficar pensando “Isso aqui é uma variável ou uma propriedade?”.

 

   Veja outro exemplo, agora no ACBrSMS:

[FixInsight Warning] ACBrSMSClass.pas(192): W511 Object 'ListaSMS' created in TRY block

begin
  try
    Self.Clear;
    if not FileExists(APath) then
      raise EACBrSMSException.CreateFmt('Arquivo "%s" não encontrado.', [APath]);
    ListaSMS := TStringList.Create;
    ListaSMS.LoadFromFile(APath);
    if ListaSMS.Count = 0 then
      Exit;
    //(bla bla bla...)
  finally
    FreeAndNil(ListaSMS);
  end;

   Não é apropriado esse código. O correto é mover a criação do objeto para fora do try..finally. Pense bem, se o objeto não for construído, você não quer que ele seja destruído.

   A mensagem ajudou a perceber também que esse bloco poderia ser escrito de outra maneira. Aquele raise não precisava estar dentro do try..finally.

 

Evitando problemas futuros

 

Rodando no pacote ACBrOpenSSL tivemos a seguinte mensagem no componente ACBrEAD:

[FixInsight Optimization] ACBrEAD.pas(268): O804 Method parameter 'AChavePublicaOpenSSL' is declared but never used

Quer dizer, parâmetro ‘AchavePublicaOpenSSL’ declarado mas não utilizado. Veja abaixo a a parte importante da função:

function TACBrEAD.ConverteChavePublicaParaOpenSSH( const AChavePublicaOpenSSL: String): String;
Var
  Buffer, Modulo, Expoente: AnsiString;
{...}
begin
  // https://www.netmeister.org/blog/ssh2pkcs8.html

  CalcularModuloeExpoente(Modulo, Expoente);

  Buffer := EncodeBufferSSH('ssh-rsa') +
            EncodeHexaSSH(Expoente) +
            EncodeHexaSSH('00'+Modulo);
  Result := 'ssh-rsa '+ EncodeBase64(Buffer);
end;

   É estranho esse método ConverteChavePublicaParaOpenSSH não utilizar o parâmetro da chavePública. Qualquer pessoa que visse o método e tentasse chamar passando a chave pública não teria o resultado desejado. Analisando o código melhor vemos que o componente lê a chave pública por meio do método “LerChavePublica”.

   Nesse caso o correto seria remover o parâmetro para que não haja nenhuma confusão.

   E essa mensagem no TACBrBALToledo2090:

[FixInsight Warning] ACBrBALToledo2090.pas(107): W508 Variable is assigned twice successively

    if (Length(wStrListDados[1]) = 16) then
      wDecimais := 1000;

    {APENAS BLOCO PROCESSADO}
    wResposta := wStrListDados[1]; //<---- sobreposto pela linha seguinte
    wResposta := Copy(wStrListDados[1], 5, 7);

    if (Length(wResposta) <= 0) then
      Exit;

   Veja que os dados de uma linha é sobreposta pela outra. O compilador nunca daria um aviso sobre isso.

   Mais dois exemplos de mensagens e o código a seguir:

[FixInsight Warning] ACBrEscEpsonP2.pas(97): W514 Loop iterator could be out of range (missing -1?)
[FixInsight Warning] ACBrEscEpsonP2.pas(100): W514 Loop iterator could be out of range (missing -1?)

  For I := 0 to Length(cTAGS_BARRAS) do
    TagsNaoSuportadas.Add( cTAGS_BARRAS[I] );

  For I := 0 to Length(cTAGS_ALINHAMENTO) do
    TagsNaoSuportadas.Add( cTAGS_ALINHAMENTO[I] );

   Essa eu não sei como não foi detectada antes. Por algum motivo não está sendo emitida a mensagem estouro quando o valor de I chega a 16 no primeiro caso e 3 no segundo.

 

Encontrando erros gerados por Ctrl+C..Ctrl+V

 

   No pacote ACBrPAF veja a mensagem gerada:

[FixInsight Optimization] ACBrPAF_T_Class.pas(137): O804 Method parameter 'ACampo2' is declared but never used

function OrdenarT2(const ACampo1, ACampo2: Pointer): Integer;
var
  Campo1, Campo2: String;
begin
  Campo1 := FormatDateTime('YYYYMMDD', TRegistroT2(ACampo1).DT_MOV) +
            TRegistroT2(ACampo1).TP_DOCTO +
            TRegistroT2(ACampo1).SERIE +
            TRegistroT2(ACampo1).NUM_ECF;
  Campo2 := FormatDateTime('YYYYMMDD', TRegistroT2(ACampo1).DT_MOV) +
            TRegistroT2(ACampo1).TP_DOCTO +
            TRegistroT2(ACampo1).SERIE +
            TRegistroT2(ACampo1).NUM_ECF;

  Result := AnsiCompareText(Campo1, Campo2);
end;

   Essa função é utilizada para ordenar os registros T2 do PAF. Mas veja que ela compara o registro “ACampo1” com ele mesmo.

   Suspeita: Ctrl+C e Ctrl+V... Quem nunca??...

   Outra situação diferente, mas relacionada com ordenação apareceu no ACBrSintegra. Na verdade 4 situações no ACBrSintegra, semelhantes entre si. Vou mostrar apenas uma, mas dessa vez a mensagem do FixInsight fica pra depois.

   Vamos a um jogo dos sete erros entre os ifs e else no código abaixo:

function Sort60A(Item1, Item2: Pointer): Integer;
var
  witem1, witem2 : TRegistro60A;
begin
  witem1 := TRegistro60A(Item1);
  witem2 := TRegistro60A(Item2);
  if witem1.Emissao>witem2.Emissao then
  begin
    if witem1.NumSerie>witem2.NumSerie then
      Result:=1
    else if witem1.NumSerie=witem2.NumSerie then
      Result:=0
    else
      Result:=-1;
  end
  else if witem1.Emissao = witem2.Emissao then
  begin
    if witem1.NumSerie>witem2.NumSerie then
      Result:=1
    else if witem1.NumSerie=witem2.NumSerie then
      Result:=0
    else
      Result:=-1;
  end
  else
  begin
    if witem1.NumSerie>witem2.NumSerie then
      Result:=1
    else if witem1.NumSerie=witem2.NumSerie then
      Result:=0
    else
      Result:=-1;
  end;
end;

   Conseguiu encontrar os erros?

   Bem, se você procurou diferenças, não deve ter encontrado nada. E não existe mesmo. Veja a mensagem da ferramenta:

[FixInsight Warning] ACBrSintegra.pas(3410): W507 THEN statement is equal to ELSE statement

   São dois if e um else pra fazer a mesma coisa... A correção foi remover o IFs e ELSE. 

   Agora vamos ao pacote ACBrSPED.

   Depois de remover muitos e muitos parâmetros desnecessários apontados pelo FixInsight, veja esse código:

function CodAjToStr(const AValue: TACBrCodAj): string;
begin
  if AValue = codAjAcaoJudicial then
    Result := '01'
  else if AValue = codAjAcaoJudicial then
    Result :=  '02'
  else if AValue = codAjLegTributaria then
    Result := '03'
  else if AValue = codAjEspRTI then
    Result := '04'
  else if AValue = codAjOutrasSituacaoes then
    Result := '05'
  else if AValue = codAjEstorno then
    Result := '06';
end;

   A mensagem é a seguinte:

[FixInsight Warning] ACBrEPCBlocos.pas(2071): W512 Odd ELSE-IF condition (review lines 2071 and 2073)

   Viu lá?

   Os dois primeiros ifs estão comparando AValue com o mesmo valor, "codAjAcaoJudicial". O segundo deveria ser "codAjProAdministrativo". Provavelmente mais um Ctrl+C..Ctrl+V.

 

Mensagens para otimização de código

 

   Nem todas as mensagens geradas são de erros. Algumas são mensagens de otimização. Muitos dos commits que temos feito estão relacionados a uma mensagem como estas abaixo:

[FixInsight Optimization] ACBrSATClass.pas(776): O801 CONST missing for unmodified string parameter 'CNPJvalue'

[FixInsight Optimization] ACBrSATClass.pas(776): O801 CONST missing for unmodified string parameter 'assinaturaCNPJs'

   Ela pode ser gerada numa função como essa:

function TACBrSATClass.AssociarAssinatura( CNPJvalue, assinaturaCNPJs : AnsiString) : String ;
begin

...// um código que não altera nenhum dos parâmetros citados

end;

   Essas mensagens estão dizendo que os parâmetros 'CNPJvalue' e ‘assinaturaCNPJs’ do tipo string não estão sendo alterados dentro da função a que eles pertencem. Nesse caso é bem provável que os parâmetros devessem ter um prefixo CONST na sua declaração, como abaixo:

function TACBrSATClass.AssociarAssinatura(const CNPJvalue, assinaturaCNPJs : AnsiString) : String ;
begin

...// um código que não altera nenhum dos parâmetros citados

end;

   Não vou entrar em muitos detalhes sobre isso, mas usar CONST tem alguns benefícios, principalmente em caso de strings:

• A execução é mais rápida, porque o compilador pode otimizar o código. No caso de strings, não tem contagem de referências;

• O compilador garante que você não vai alterar os parâmetros passados gerando um efeito colateral indesejado em quem chamou as funções;

• O código fica mais legível, porque você pode ler que a intenção é não alterar o parâmetro passado;

• Como os parâmetros são imutáveis, pode tornar o código mais ThreadSafe;

   Se quer saber um pouco mais sobre isso, recomendo os seguintes links:

• All hail the “const” parameters!

• Is the use of ‘const’ dogmatic or rational?

 

Concluindo...

 

   Bom pessoal, ainda temos bastante pra fazer. Contudo, queremos dizer que o FixInsight tem nos ajudado melhorar nosso código. Ficamos tão satisfeitos que entramos em contato com a TMS e eles generosamente nos cederam uma licença da versão Pro pra continuar nosso trabalho. Muito obrigado TMS.

   Agora, se você quer nossa opinião, essa é uma ferramenta altamente recomendada e está disponível pra toda versão do Delphi a partir do Delphi 2006. Se você tem alguma dúvida, baixe a versão trial e comece agora mesmo a usar no seu código.

   A versão trial limita as mensagens a 5 por units e funciona por 30 dias. Mas é o suficiente pra se perceber como é muito útil, como aconteceu com a gente.

   Quer um passo a passo em como utilizá-la? Veja o próximo post logo abaixo.

[EMBarbosa]

Instalando o FixInsight para utilizar no seu projeto

Para utilizar o TMS FixInsight no seu projeto, primeiro faça o download da versão trial no site oficial. O FixInsight está disponível para todas as versões do Delphi a partir do Delphi 2006.

A versão pro possui linha de comando, permitindo você executar a ferramenta mesmo quando o Delphi não está aberto. Isso permite você integrar com seu sistema de Build ou sistema de integração contínua.

A instalação é muito simples, bastando escolher em qual versão do Delphi você quer instalar.

Após a instalação as seguintes entradas vão aparecer no menu Project:

E também no "Project Manager" (clique com botão direito no projeto):

A entrada "FixInsight Settings..." configura a ferramenta e pode habilitar ou desabilitar as mensagens geradas.

Ela abre uma tela como essa:

Na imagem acima você pode observar que a mensagem "C101 Method '%s' is too long (%d lines)" está selecionada e permite a configuração de quantas linhas para você um método, function ou procedure não deve exceder.

Dá pra ver também que as mensagens C102 e C103 estão desabilitadas e assim não geram avisos.

A entrada "Run FixInsight for unit1.pas" executa a ferramenta para a unit aberta atualmente (neste caso Unit1.pas).
A entrada "Run FixInsight" executa a ferramenta no projeto atual inteiro.

 

Rodando o FixInsight no seu projeto

Como mencionado, é por meio da entrada 'Run FixInsight" que você executa a ferramenta no seu projeto.

Então basta abrir o seu projeto e executar por meio do menu Project -> Run FixInsight.

Ele vai ser executado e abrir uma aba na janela de mensagens como na imagem abaixo. Dois cliques te jogam na unit e linha relacionada a mensagem:

Agora é com você. Você analisa a mensagem o código e verifica se algo pode ser feito.

[sismais]

@EMBarbosa que ferramenta TOP.

Parabéns por implementar no ACBr, e também por compartilhar exemplos de uso, isso vai ajudar muita gente.