8

Para emissão de relatórios da minha aplicação eu criei várias classes, e cada classe é responsável por um relatório, entretanto, existe repetição de muito código por parte dessas classes que eu gostaria de evitar, reescrevendo o código de uma forma que aproveite mais os recursos de orientação a objeto da linguagem C#.

Vou apresentar duas classes para ilustrar a situação, e todas as outras terão a estrutura parecida com poucos atributos que varriam.

Classe RelAlunoAtivoOuInativo:

using fyiReporting.RDL;
using fyiReporting.RdlViewer;
using System.Data;
using System.Data.SQLite;
using DAL;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace BLL
{
    public class RelAlunoAtivoOuInativo
    {
        private RdlViewer reportViewer;
        private ViewerToolstrip reportStrip;
        private String nomeArquivo;
        private StringBuilder query;
        private List<string> erros;
        private DALConexao conexao;
        private Boolean ativoOuInativo;

        public RelAlunoAtivoOuInativo(DALConexao conexao, bool ativoOuInativo) 
        {            
            this.conexao = conexao;
            this.erros = new List<string>();
            this.reportViewer = new RdlViewer();
            this.reportStrip = new ViewerToolstrip(this.reportViewer);
            this.reportStrip.Viewer = this.reportViewer;
            this.query = new StringBuilder();
            this.query.Append("SELECT id_aluno, nome, telefone, informatica, participa_encontro FROM Alunos WHERE ativo = ?");
            this.nomeArquivo = "relatorioGeralAlunoAtivoInativo.rdl";
            this.ativoOuInativo = ativoOuInativo;
            emitir();
        }

        public void emitir() 
        {
            PathRelatorio caminhoArquivo = new PathRelatorio(this.nomeArquivo);

            if (caminhoArquivo.existeArquivo())
            {
                try
                {
                    this.reportViewer.SourceFile = new Uri(caminhoArquivo.Path);
                    this.reportViewer.Parameters += string.Format("conexao={0}", this.conexao.Conexao.ConnectionString);
                    this.reportViewer.Report.DataSets["Data"].SetData(obterTabela());
                    this.reportViewer.Rebuild();
                    this.reportViewer.HideRunButton();
                    this.reportViewer.Dock = System.Windows.Forms.DockStyle.Fill;
                }
                catch (Exception ex)
                {
                    this.erros.Add(string.Format("Erro: {0}", ex.Message));
                }
            }
        }

        public RdlViewer ReportViewer
        {
            get
            {
                return this.reportViewer;
            }
        }

        public ViewerToolstrip ReportStrip
        {
            get
            {
                return this.reportStrip;
            }
        }

        public List<string> Erros
        {
            get
            {
                return this.erros;
            }
        }

        private DataTable obterTabela()
        {
            DataTable tabela = new DataTable();

            try
            {
                using (SQLiteConnection conexao = this.conexao.Conexao)
                {
                    using (SQLiteCommand command = conexao.CreateCommand())
                    {
                        command.CommandType = CommandType.Text;
                        command.CommandText = this.query.ToString();
                        command.Parameters.Add("ativo", DbType.Int32).Value = this.ativoOuInativo ? 1 : 0;

                        conexao.Open();

                        using (SQLiteDataAdapter dataAdapter = new SQLiteDataAdapter(command))
                        {
                            dataAdapter.Fill(tabela);

                            conexao.Close();
                        }
                    }
                }
            }
            catch (Exception ex)
            {
                this.erros.Add("Erro:" + ex.Message);

                return null;
            }

            return tabela;
        }
    }
}

Classe RelGeralAluno:

using fyiReporting.RDL;
using fyiReporting.RdlViewer;
using System.Data;
using System.Data.SQLite;
using DAL;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace BLL
{
    public class RelGeralAluno
    {
        private RdlViewer reportViewer;
        private ViewerToolstrip reportStrip;
        private String nomeArquivo;
        private StringBuilder query;
        private List<string> erros;
        private DALConexao conexao;

        public RelGeralAluno(DALConexao conexao) 
        {
            this.conexao = conexao;            
            this.erros = new List<string>();            
            this.reportViewer = new RdlViewer();            
            this.reportStrip = new ViewerToolstrip(this.reportViewer);
            this.reportStrip.Viewer = this.reportViewer;
            this.query = new StringBuilder();
            this.query.Append("SELECT nome, data_cadastro, telefone, celular, endereco, email, idade FROM Alunos ");
            this.nomeArquivo = "relatorioGeralAlunos.rdl";
            emitir();
        }

        public void emitir()
        {
            PathRelatorio caminhoArquivo = new PathRelatorio(this.nomeArquivo);

            if (caminhoArquivo.existeArquivo()) 
            {
                try
                {
                    this.reportViewer.SourceFile = new Uri(caminhoArquivo.Path);
                    this.reportViewer.Parameters += string.Format("conexao={0}", this.conexao.Conexao.ConnectionString);
                    this.reportViewer.Report.DataSets["Data"].SetData(obterTabela());
                    this.reportViewer.Rebuild();
                    this.reportViewer.HideRunButton();
                    this.reportViewer.Dock = System.Windows.Forms.DockStyle.Fill;
                }
                catch (Exception ex) 
                {
                    this.erros.Add(string.Format("Erro: {0}", ex.Message));
                }
            }
        }

        public RdlViewer ReportViewer
        {
            get
            {
                return this.reportViewer;
            }
        }

        public ViewerToolstrip ReportStrip
        {
            get
            {
                return this.reportStrip;
            }
        }

        public List<string> Erros
        {
            get
            {
                return this.erros;
            }
        }

        private DataTable obterTabela()
        {
            DataTable tabela = new DataTable();

            try
            {
                using (SQLiteConnection conexao = this.conexao.Conexao)
                {
                    using (SQLiteCommand command = conexao.CreateCommand())
                    {
                        command.CommandType = CommandType.Text;
                        command.CommandText = this.query.ToString();

                        conexao.Open();

                        using (SQLiteDataAdapter dataAdapter = new SQLiteDataAdapter(command))
                        {
                            dataAdapter.Fill(tabela);

                            conexao.Close();
                        }
                    }
                }
            }
            catch (Exception ex)
            {
                this.erros.Add("Erro:" + ex.Message);

                return null;
            }

            return tabela;
        }
    }
}

Implementação das classes:

private void FormRunRelatorio_Load(object sender, EventArgs e)
{
    var erros = new List<string>();

    switch (this.relatorioSelecionado) 
    {
        case "GeralAluno":
            RelGeralAluno relGeralAluno = new RelGeralAluno(new DALConexao(new DadosConexao().String_Conexao));
            this.Controls.Add(relGeralAluno.ReportStrip);
            this.Controls.Add(relGeralAluno.ReportViewer);
            erros = relGeralAluno.Erros;                    
            break;

        case "GeralAlunoInativoOuAtivo":
            RelAlunoAtivoOuInativo relAlunoAtivoInativo = new RelAlunoAtivoOuInativo(new DALConexao(new DadosConexao().String_Conexao), false);
            this.Controls.Add(relAlunoAtivoInativo.ReportStrip);
            this.Controls.Add(relAlunoAtivoInativo.ReportViewer);
            erros = relAlunoAtivoInativo.Erros;
            break;
        
        case mais condicao...
        mais codigo...
    }
   
    if (erros.Count > 0)
    {
        foreach (var erro in erros)
            MessageBox.Show(erro, "Erro", MessageBoxButtons.OK, MessageBoxIcon.Error);

        this.Close();
    }
}

Repetição entre métodos e atributos

Os seguintes atributos e métodos se repetem muito entre as classes, são eles:

Atributos

  1. Atributo reportViewer: private RdlViewer reportViewer;
  2. Atributo reportStrip: private ViewerToolstrip reportStrip;
  3. Atributo nomeArquivo: private String nomeArquivo;
  4. Atributo query: private StringBuilder query;
  5. Atributo erros: private List<string> erros;
  6. Atributo conexao: private DALConexao conexao;

Métodos

  1. Método emitir(): public void emitir()
  2. Método obterTabela(): private DataTable obterTabela()

A maneira como esta definida as classes não me parecer a melhor forma, sei que existe uma forma, porem meu conhecimento ainda é limitado.


Pergunta

Como eu posso evitar toda esta repetição de código, se é possível utilizar herança ou interface ou alguma outra característica da orientação a objeto, dentro das boas praticas de programação? Ou até mesmo se existe algum padrão ou ferramenta dentro da engenharia de software para evitar a situação acima?

Obs: Utilizo o Microsoft Visual C# 2012 e a versão do C# é 5.0 junto com o banco de dados SQLite, e a ferramenta de emissão de relatório é o My-FyiReporting.

6
  • Normalmente eu não gosto muito de perguntas de revisão de código a não ser que elas sejam bem específicas (o que talvez deixe de ser revisão de código) ou triviais, mas vou tentar fazer alguma coisa, pode ser que não seja tão complicado. Me confirme se as únicas coisas diferentes nas duas classes são o nome do arquivo RDL a ser usado, a query SQL e um parâmetro existente em uma delas. Por curiosidade, o projeto original desse reporter foi abandonado?
    – Maniero
    Commented 26/03/2016 às 1:37
  • O nome do arquivo representa o relatório, e a query também muda, cada relatório tem a sua query, algumas querys tem parâmetros, o tipo pode variar. mais a maioria não possui parâmetro algum.
    – gato
    Commented 26/03/2016 às 1:53
  • A dúvida é se essas são as únicas diferenças.
    – Maniero
    Commented 26/03/2016 às 1:53
  • O projeto original foi abandonado, mais este e um fork do original e uma nova versão, gerenciada pelo Peter Gill.
    – gato
    Commented 26/03/2016 às 1:54
  • 1
    Eu editei a resposta colocando exemplos. Eu estava evitando por que é mais complicado do que parece. Provavelmente tem erros na minha, mas pelo menos faz o que precisa (na ideia geral). Pra ver como é complicado até analisar, a resposta que está com mais votos não faz o que precisa e mesmo assim recebeu vários votos.
    – Maniero
    Commented 27/03/2016 às 17:00

2 Respostas 2

8

Não falarei de todos os aspectos que poderia ser diferente, de como trata exceção, etc. Ficando no foco da pergunta tem o caminho mais orientado a objeto ou o caminho mais procedural/modular (ainda que alguns irão dizer que tudo é OOP, afinal alguns gostam que dizer que qualquer solução boa é OOP e qualquer coisa ruim ou não é OOP ou é OOP errado).

Redundância

A técnica básica para eliminar redundância é isolar todas partes iguais e colocar em um método ou classe auxiliar. Pelo menos quando tem tudo fechado.

Eliminar redundância não é o mesmo que DRY e muita gente confunde isto.

Opções

Quando algo está em aberto pode complicar. E esta é uma das falhas de OOP. Em geral você tem que "adivinhar" tudo o que poderá precisar no futuro. Claro que isto é problema generalizado usando qualquer paradigma. Quando você faz OOP e cria herança, fica complicado melhorar a classe base sem correr risco de estragar as classes derivadas. Claro que isto pode dar bastante flexibilidade também em certos cenários. Mas herança não é a única forma de flexibilizar.

Como as classes são quase idênticas, seria fácil criar apenas uma classe (possivelmente abstrata) com as partes iguais e aí poderia herdar dela para criar essas duas. Provavelmente teria que isolar mais as responsabilidades, principalmente dos construtores.

É bastante óbvio que as strings do nome do arquivo RDL e da query são variáveis e deve ser parametrizadas. Até pode ser no construtor (veja abaixo) ou pode ter métodos próprios para isto.

Você teria que ter alguma forma de injetar parâmetros, talvez pelo próprio construtor (veja abaixo) ou com métodos. Isto poderia sem feito de algumas formas, provavelmente teria que passar algumas informações, provavelmente o nome do parâmetro na query, o tipo e o argumento.

Pode modularizar mais o processo e ter um método virtual que faça isto e seja sempre chamado no momento adequado. Obviamente que ele seria vazio na classe abstrata e só seria sobrescrito se tiver um parâmetro nas derivada específica (caso do exemplo RelAlunoAtivoOuInativo).

Ou poderia ter uma lambda passada no construtor que faça a adição do parâmetro, o que eliminaria a necessidade de ter uma propriedade para este parâmetro, ela já será encapsulada dentro da lambda.

O jeito menos OOP é justamente usar parâmetros no construtor que receba esses dados que variam. O código começou fazendo isto injetando a conexão (e me parece que nem precisava disto, mas sei lá, de qualquer forma injeção sempre dá mais flexibilidade). Porque não fazer com o resto? Desta forma a aplicação só teria a classe de relatório, ela poderia ser até mesmo selada, e o construtor dela receberia as partes que variam, que são duas strings e uma lambda (ou se preferir fazer mais na mão os 3 dados necessários para construir a adição do parâmetro na query "ativo", DbType.Int32, 1 ou 0).

Acha OOP de menos? Ok, crie classes isoladas com as informações que variam e injete a classe do relatório específico na classe de relatório. No fundo é apenas encapsular todos essas parâmetros que você passaria no construtor em uma classe e aí ao invés de pegar esses parâmetros, pegaria os membros da classe passada (injetada). Assim faz composição e evita a maligna herança, dando bastante flexibilidade e permitindo melhorar a classe de relatório com menos riscos de quebrar alguma coisa.

É claro que em qualquer caso um requisito novo futuro sempre poderá inviabilizar o uso desta classe. Escolha seu pesadelo na manutenção futura.

Provavelmente eu faria isto de uma forma bem diferente, mas sem saber o contexto nem posso afirmar nada.

E cuidado com academicismos. Tem muita gente que diz que o jeito certo é tal sem que aquilo traga vantagem real.

Herança

public abstract class Relatorio {
    private string nomeArquivo;
    private stringBuilder query = new StringBuilder();
    private DALConexao dalConexao;

    public RdlViewer ReportViewer { get; private set; } = new RdlViewer();

    public ViewerToolstrip ReportStrip { get; private set; }

    public Relatorio(DALConexao conexao, string consulta, string arquivo) {            
        dalConexao = conexao;
        //provavelmente dá para colocar na propriedade mas não posso testar
        ReportStrip = new ViewerToolstrip(ReportViewer);
        ReportStrip.Viewer = ReportViewer;
        query.Append(consulta);
        nomeArquivo = arquivo;
        emitir();
    }

    private void emitir() {
        //não gostei nada desse código, mas enfim....
        var caminhoArquivo = new PathRelatorio(nomeArquivo);
        if (caminhoArquivo.existeArquivo()) { //não gosto deisto
            ReportViewer.SourceFile = new Uri(caminhoArquivo.Path);
            ReportViewer.Parameters += string.Format("conexao={0}", dalConexao.Conexao.ConnectionString);
            ReportViewer.Report.DataSets["Data"].SetData(obterTabela());
            ReportViewer.Rebuild();
            ReportViewer.HideRunButton();
            ReportViewer.Dock = DockStyle.Fill;
        }
    }

    private DataTable obterTabela() {
        var tabela = new DataTable();
        using var conexao = dalConexao.Conexao);
        using var command = conexao.CreateCommand();
        command.CommandType = CommandType.Text;
        command.CommandText = query.ToString();
        AdicionarParametros(command);
        conexao.Open();
        using var dataAdapter = new SQLiteDataAdapter(command);
        dataAdapter.Fill(tabela);
        return tabela;
    }

    protected virtual void AdicionarParametros(SQLiteCommand command) {}
}

Não quis mudar muito, mas tirei algumas coisas que eu achei que estavam demais. Sei que provavelmente não vai seguir isto, mas eu tinha que fazer. Tirei muita redundância de código que tinha por outros motivos. Esta classe está mais complicada do que parece precisar. Muita coisa que existe sem um motivo aparente. Eu ia simplificar mais, mas talvez queira fazer assim por algum motivo que eu desconheço. Deixei no meio termo. Também me parece que ela tem responsabilidades demais. E se resolver este problema, talvez ela nem seja necessária.

Herdando:

public class RelAlunoAtivoOuInativo : Relatorio {
    public bool AtivoOuInativo { get; private set; }

    public RelAlunoAtivoOuInativo(DALConexao conexao, bool ativoOuInativo) : base(new DALConexao(new DadosConexao().String_Conexao), "SELECT nome, data_cadastro, telefone, celular, endereco, email, idade FROM Alunos ", "relatorioGeralAlunoAtivoInativo.rdl") {
        AtivoOuInativo = ativoOuInativo;
    }

    public RelAlunoAtivoOuInativo(bool ativoOuInativo) : this(new DALConexao(new DadosConexao().String_Conexao), ativoOuInativo) {}

    protected override void AdicionarParametros(SQLiteCommand command) => command.Parameters.Add("ativo", DbType.Int32).Value = AtivoOuInativo ? 1 : 0;
}

public class RelGeralAluno : Relatorio {
    public RelGeralAluno(DALConexao conexao) : base(conexao, "SELECT nome, data_cadastro, telefone, celular, endereco, email, idade FROM Alunos, "relatorioGeralAlunos.rdl") {}

    public RelGeralAluno() : this(new DALConexao(new DadosConexao().String_Conexao)) {}
}

Coloquei no GitHub para referência futura.

Já que é para simplificar aproveite e faça isto:

private void FormRunRelatorio_Load(object sender, EventArgs e) {
    switch (relatorioSelecionado) {
        case "GeralAluno": //Vai string mesmo?
            new RelGeralAluno();
            break;
        case "GeralAlunoInativoOuAtivo":
            new RelAlunoAtivoOuInativo(false);
            break;
        //case mais condicao...
        //    mais codigo...
    }
    Controls.Add(relGeralAluno.ReportStrip);
    Controls.Add(relGeralAluno.ReportViewer);
}

Toda vez que alguém captura Exception eu acho que está fazendo algo errado. Então tirei. Precisa de um mecanismo melhor pra tratar erros, aí, desta forma, é desnecessário e provavelmente está criando redundâncias aos montes por toda aplicação.

Notou que não precisa de variável alguma? Na verdade não precisa nem de um classe normal? Pode ser que em algum contexto específico não demonstrado neste código possa ser útil, mas acho que todo este estilo de codificação é algo desnecessário.

Os proponentes de OOP estão tendo tremeliques vendo esse seu switch. O ideal seria ter uma classe que tratasse isso, assim sempre que tiver um relatório novo, o próprio relatório se encarrega de fornecer um mecanismo de sua chamada, evitando o switch que cria um problema de manutenção sempre que tem relatório novo. Mas isso é outro assunto que acho que foge do escopo aqui.

Composição

public class Relatorio {
    private string nomeArquivo;
    private stringBuilder query = new StringBuilder();
    private DALConexao dalConexao = new DALConexao(new DadosConexao().String_Conexao);

    public RdlViewer ReportViewer { get; private set; } = new RdlViewer();

    public ViewerToolstrip ReportStrip { get; private set; }

    public Func<SQLiteCommand> AdicionarParametros { get; private set; } = dummy => {};

    public Relatorio(string consulta, string arquivo) {            
        ReportStrip = new ViewerToolstrip(ReportViewer);
        ReportStrip.Viewer = ReportViewer;
        query.Append(consulta);
        nomeArquivo = arquivo;
        emitir();
    }

    public Relatorio(string consulta, string arquivo, Func<SQLiteCommand> metodoParametros) : this (consulta, arquivo) => AdicionarParametros = metodoParametros;

    private void emitir() {
        //não gostei nada desse código, mas enfim....
        var caminhoArquivo = new PathRelatorio(nomeArquivo);
        if (caminhoArquivo.existeArquivo()) { //não gosto deisto
            ReportViewer.SourceFile = new Uri(caminhoArquivo.Path);
            ReportViewer.Parameters += string.Format("conexao={0}", dalConexao.Conexao.ConnectionString);
            ReportViewer.Report.DataSets["Data"].SetData(obterTabela());
            ReportViewer.Rebuild();
            ReportViewer.HideRunButton();
            ReportViewer.Dock = DockStyle.Fill;
        }
    }

    private DataTable obterTabela() {
        var tabela = new DataTable();
        using var conexao = dalConexao.Conexao);
        using var command = conexao.CreateCommand();
        command.CommandType = CommandType.Text;
        command.CommandText = query.ToString();
        AdicionarParametros(command);
        conexao.Open();
        using var dataAdapter = new SQLiteDataAdapter(command);
        dataAdapter.Fill(tabela);
        return tabela;
    }
}

public class RelAlunoAtivoOuInativo {
    public bool AtivoOuInativo { get; private set; }

    public RelAlunoAtivoOuInativo(bool ativoOuInativo) : base("SELECT nome, data_cadastro, telefone, celular, endereco, email, idade FROM Alunos ", "relatorioGeralAlunoAtivoInativo.rdl", ((command) => command.Parameters.Add("ativo", DbType.Int32).Value = ativoOuInativo ? 1 : 0)) {}
}

public class RelGeralAluno {
    public RelGeralAluno() : base("SELECT nome, data_cadastro, telefone, celular, endereco, email, idade FROM Alunos, "relatorioGeralAlunos.rdl") {}
}

Coloquei no GitHub para referência futura.

Sem OOP

Não conheço o caso específico, mas em muitas situações nada disto é necessário, pode ser feito de forma simples sem prejuízos. Onde se usa em um só lugar, onde não se precisa testes de unidade neste ponto (não quer dizer que não precise em outros ou que não precise de nenhum teste aí), pra que ficar criando classes, instâncias?

public static class Relatorio {
    public void Emitir(string consulta, string arquivo, Func<SQLiteCommand> metodoParametros) {
        //não gostei nada desse código, mas enfim....
        var reportViewer = new RdlViewer();
        var reportStrip = new ViewerToolstrip(ReportViewer);
        reportStrip.Viewer = ReportViewer;
        var query = new StringBuilder().Append(consulta);
        var dalConexao = dalConexao = new DALConexao(new DadosConexao().String_Conexao);
        var caminhoArquivo = new PathRelatorio(arquivo);
        if (caminhoArquivo.existeArquivo()) { //não gosto disto
            reportViewer.SourceFile = new Uri(caminhoArquivo.Path);
            reportViewer.Parameters += "conexao={dalConexao.Conexao.ConnectionString}";
            reportViewer.Report.DataSets["Data"].SetData(obterTabela(dalConexao, metodoParametros));
            reportViewer.Rebuild();
            reportViewer.HideRunButton();
            reportViewer.Dock = DockStyle.Fill;
        }
    }

    public void Emitir(string consulta, string arquivo) => Emitir(consulta, arquivo, dummy => {});

    private DataTable obterTabela(string dalConexao, Func<SQLiteCommand> adicionarParametros) {
        var tabela = new DataTable();
        using var conexao = dalConexao.Conexao;
        using var command = conexao.CreateCommand();
        command.CommandType = CommandType.Text;
        command.CommandText = query.ToString();
        adicionarParametros(command);
        conexao.Open();
        using var dataAdapter = new SQLiteDataAdapter(command);
        dataAdapter.Fill(tabela);
        return tabela;
    }
}

private void FormRunRelatorio_Load(object sender, EventArgs e) {
    switch (relatorioSelecionado) {
        case "GeralAluno":
            Relatorio.Emitir("SELECT nome, data_cadastro, telefone, celular, endereco, email, idade FROM Alunos, "relatorioGeralAlunos.rdl");
            break;
        case "GeralAlunoInativoOuAtivo":
            Relatorio.Emitir("SELECT nome, data_cadastro, telefone, celular, endereco, email, idade FROM Alunos ", "relatorioGeralAlunoAtivoInativo.rdl", ((command) => command.Parameters.Add("ativo", DbType.Int32).Value = 0));
            break;
        //case mais condicao...
        //    mais codigo...
    }
    Controls.Add(relGeralAluno.ReportStrip);
    Controls.Add(relGeralAluno.ReportViewer);
}

Coloquei no GitHub para referência futura.

Obviamente não teste nada disto e pode haver pequenos problemas, principalmente porque eu posso ter me perdido sem ter o VS/Resharper disponível no momento. Só quis passar as ideias.

Certamente outras opções ainda são possíveis.

2
  • O ultimo exemplo sem OOP, é de forma procedural?
    – gato
    Commented 28/03/2016 às 11:21
  • Sim, é uma das formas procedurais. É mais simples e resolve bem a maioria das situações sem trazer nenhuma desvantagem real. Ainda dá pra melhorar este código, mas não quis mexer demais na sua estrutura.
    – Maniero
    Commented 28/03/2016 às 11:25
7

Sim, existem diversas formas de evitar a repetição de código, e você está começando bem em criticar tal repetição. Um princípio básico em programação é o DRY (don't repeat yourself) e prega justamente isso. Categoricamente, se você estiver repetindo 2 vezes o mesmo código, SEMPRE haverá uma forma de abstrair tal lógica e reaproveitar. Repetição de código é realmente um dos piores problemas quando se fala em manutenção de código. No entanto, apenas pondere entre adicionar complexidade extrema a um código apenas para evitar repetição de duas ou três linhas de código (mais que isso, provavelmente é melhor abstrair).

Em seu código existem diversas refatorações a serem feitas, entre as básicas estão:

  • Código bilingue (evite usar portugues e inglês, seja coerente).
  • C# Coding standards: Métodos são pascal case, membros (de classe) começam com undescore, etc
  • "Implicitly Typed Local Variables": a utilização do "var" força implicitamente a boa nomenclatura de membros, que dirige para um código mais limpo e legível (inclusive sem repetição, veja em seu código, várias vezes a variável possui o mesmo nome da classe, que fica desnecessariamente repetitivo na mesma linha).

Outras refatorações são um pouco mais complexas, e demandam um conhecimento maior sobre orientação a objeto, boas práticas e padrões, sugiro que dê uma olhada em cada uma delas:

  • SRP - Single responsability principle: Uma classe deve possuir apenas uma responsabilidade
  • ORM: Em .Net o framework para ORM de lonoge mais utilizado é o Entity Framework e com sua utilização você evita de ter queries hardcoded em suas classes, utilizar recursos como o migrations e também deixar seu código testável.
  • Injeção de dependência: A utilização de DI (dependency injection) permite a criação de testes unitários (através de Mocks) e desacoplam as camadas de seu projeto.

Por fim, outra boa prática é o desenvolvimento através de "baby-steps", ou seja, pequenos passos, evitando grandes refatorações o que podem causar problemas na identificação de bugs introduzidos pela mesma. A combinação da utilização de testes com esses "baby-steps" é recomendada (crie um teste, refatore e depois veja se o teste ainda passa).

Como já identificou, o primeiro passo é realmente a remoção da duplicidade de todo esse código. Vale lembrar mais uma vez que, se precisou repetir, mesmo que apenas uma vez, é possível abstrair e reaproveitar.

O exemplo a seguir é apenas uma sugestão inicial para refatoração, você pode, com o tempo, usar principalmente um ORM e injeção de dependência para conseguir testar unitariamente seu código, e ter o mesmo livre de códigos SQL "hardcoded". Em seu caso, utilizando herança, uma refatoração básica seria a seguinte (código não testado, apenas para demonstração de como fazer as refatorações):

public abstract class BaseReport
{
    private RdlViewer _reportViewer;
    private ViewerToolstrip _reportStrip;
    private String _nomeArquivo;
    private String _query;
    private List<string> _erros;
    private DALConexao _conexao;

    public BaseReport(DALConexao conexao, string query, string nomeArquivo) 
    {            
        _conexao = conexao;
        _erros = new List<string>();
        _reportViewer = new RdlViewer();
        _reportStrip = new ViewerToolstrip(_reportViewer);
        _reportStrip.Viewer = _reportViewer;
        _query = query;
        _nomeArquivo = nomeArquivo;
        Emitir();
    }

    protected abstract AdicionarParametros( SQLiteCommand cmd );

    private void Emitir() 
    {
        PathRelatorio caminhoArquivo = new PathRelatorio(_nomeArquivo);

        if (caminhoArquivo.existeArquivo())
        {
            try
            {
                _reportViewer.SourceFile = new Uri(caminhoArquivo.Path);
                _reportViewer.Parameters += string.Format("conexao={0}", _conexao.Conexao.ConnectionString);
                _reportViewer.Report.DataSets["Data"].SetData(ObterTabela());
                _reportViewer.Rebuild();
                _reportViewer.HideRunButton();
                _reportViewer.Dock = System.Windows.Forms.DockStyle.Fill;
            }
            catch (Exception ex)
            {
                _erros.Add(string.Format("Erro: {0}", ex.Message));
            }
        }
    }

    public RdlViewer ReportViewer
    {
        get
        {
            return _reportViewer;
        }
    }

    public ViewerToolstrip ReportStrip
    {
        get
        {
            return _reportStrip;
        }
    }

    public List<string> Erros
    {
        get
        {
            return _erros;
        }
    }




    private DataTable ObterTabela()
    {
        DataTable tabela = new DataTable();

        try
        {
            using (SQLiteConnection conexao = _conexao.Conexao)
            {
                using (SQLiteCommand command = conexao.CreateCommand())
                {
                    command.CommandType = CommandType.Text;
                    command.CommandText = _query.ToString();

                    adicionarParametros(command);

                    conexao.Open();

                    using (SQLiteDataAdapter dataAdapter = new SQLiteDataAdapter(command))
                    {
                        dataAdapter.Fill(tabela);

                        conexao.Close();
                    }
                }
            }
        }
        catch (Exception ex)
        {
            _erros.Add("Erro:" + ex.Message);

            return null;
        }

        return tabela;
    }
}

Com pouquíssimas alterações, foi extraída uma "classe base" com todas as funcionalidades e código que antes eram duplicados.

Ambas as classes ficariam assim:

public class RelAlunoAtivoOuInativo : BaseReport
{
    public RelAlunoAtivoOuInativo(DALConexao conexao) : base( conexao,  "SELECT nome, data_cadastro, telefone, celular, endereco, email, idade FROM Alunos ", "relatorioGeralAlunoAtivoInativo.rdl" ) 
    { }

    protected AdicionarParametros( SQLiteCommand cmd ){
        cmd.Parameters.Add("ativo", DbType.Int32).Value = this.ativoOuInativo ? 1 : 0;
    }
}

public class RelGeralAluno: BaseReport
{
    public RelGeralAluno(DALConexao conexao) : base( conexao,  "SELECT nome, data_cadastro, telefone, celular, endereco, email, idade FROM Alunos , "relatorioGeralAlunos.rdl" ) 
    { }

    protected AdicionarParametros( SQLiteCommand cmd ){ }
}

Uma sugestão para uma primeira refatoração no método é a extração da instanciação de suas classes de reports (utilização de uma factory simples).

private void FormRunRelatorio_Load(object sender, EventArgs e)
{
    var erros = new List<string>();
    var reportFactory = new SelectedReportFactory();
    var reportInstance = reportFactory.CreateReportFor(this.relatorioSelecionado);

    Controls.Add(reportInstance.ReportStrip);
    Controls.Add(reportInstance.ReportViewer);

    if (reportInstance.Erros.Count > 0)
    {
        foreach (var erro in reportInstance.Erros)
            MessageBox.Show(erro, "Erro", MessageBoxButtons.OK, MessageBoxIcon.Error);

        this.Close();
    }
}

public class SelectedReportFactory{
    public BaseReport CreateReportFor(string name){
        switch (name) 
        {
            case "GeralAluno":
                return new RelGeralAluno(new DALConexao(new DadosConexao().String_Conexao));

            case "GeralAlunoInativoOuAtivo":
                return new RelAlunoAtivoOuInativo(new DALConexao(new DadosConexao().String_Conexao), false);

            default: throw new Exception(string.Format("Invalid report name [{0}]", name)); 
        }
    }
}

Com poucos minutos foi possível abstrair praticamente toda a duplicidade de seu código. A partir daqui, sugiro que leia as sugestões passadas acima e dê continuidade a refatoração. Vale lembrar apenas sobre um pequeno detalhe: over engeneering. É fácil "se perder" nas refatorações e querer abraçar o mundo utilizando todas as boas praticas, design patterns e princípios de programação possíveis (e vejo isso acontecer dia após dia). Pondere apenas tais refatorações/abstrações com o custo a ser implementado e também complexidade final do código. Afinal, de nada adianta um código perfeito se o produto atrasou tanto a ser desenvolvido que não atende mais, ou mesmo, um código tão cheio de patterns que outros desenvolvedores menos experientes (que são os que irão manter o código), demoram 1 dia apenas para entender o que acontece...

8
  • Como eu poderia passar um parâmetro para o construtor da classe RelAlunoAtivoOuInativo que neste caso seria o parâmetro _ativoOuInativo, através da classe SelectedReportFactory, é possível fazer isso?
    – gato
    Commented 26/03/2016 às 13:20
  • Sim, existem diversas alternativas para isso. A primeira, ainda utilizando herança (sendo mais simples e não demandando outras refatorações) seria através da criação de uma estrutura para a passagem desses parâmetros. Essa estrutura deve ser genérica suficiente para atender todas as possíveis instâncias da factory e ao mesmo tempo não deve expôr detalhes específicos de uma instância para outra (coesão). Commented 26/03/2016 às 14:06
  • Sem conhecer todo o contexto é difícil dar diretivas tão detalhistas assim, mas dependendo do mesmo, a utilização de herança possa complicar mais do que resolver, e se esse for o caso, a próxima refatoração seria remover herança e introduzir a composição. Commented 26/03/2016 às 14:07
  • Nesse caso, você não teria mais classe abstrata e classes filhas, tão pouco uma factory que resolveria tudo, o que teria seriam classes de única responsabilidade para cada uma das funcionalidades, que compõem um chamado "service". Commented 26/03/2016 às 14:08
  • Essa classe por sua vez seria responsável apenas por delegar a execução para suas dependências, dessa forma, você tem a flexibilidade de resolver as dependências (no caso as classes "RelGeralAluno" e "RelAlunoAtivoOuInativo") com os parâmetros necessários e passar a instância para a classe" Service" . Note que, para utilizar a composição dessa forma, a lógica contidas no construtor deve ser movida para um método (visto que a instânciação ocorrerá na resolução de dependências e não na utilização). Commented 26/03/2016 às 14:08

Você deve fazer log-in para responder a esta pergunta.

Esta não é a resposta que você está procurando? Pesquise outras perguntas com a tag .