How to avoid code repetition between classes?

Asked

Viewed 881 times

8

To report on my application I have created several classes, and each class is responsible for a report, however, there is a repetition of a lot of code by those classes that I would like to avoid, rewriting the code in a way that makes the most of the language’s object orientation features C#.

I will present two classes to illustrate the situation, and all the others will have the structure similar to few attributes that would sweep.

Class 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;
        }
    }
}

Class 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;
        }
    }
}

Class implementation:

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();
    }
}

Repetition between methods and attributes

The following attributes and methods repeat much between classes, they are:

Attributes

  1. Attribute reportViewer: private RdlViewer reportViewer;
  2. Attribute reportStrip: private ViewerToolstrip reportStrip;
  3. Filename attribute: private String nomeArquivo;
  4. Query attribute: private StringBuilder query;
  5. Error attribute: private List<string> erros;
  6. Related attribute: private DALConexao conexao;

Methods

  1. Method issuing(): public void emitir()
  2. Method to obtain Bee(): private DataTable obterTabela()

The way it defined the classes does not seem to me the best way, I know there is a way, but my knowledge is still limited.


Question

How can I avoid all this code repetition, if it is possible to use inheritance or interface or some other feature of object orientation, within good programming practices? Or even if there is some standard or tool within software engineering to avoid the above situation?

Obs: I use Microsoft Visual C# 2012 and the C# version is 5.0 along with the Sqlite database, and the reporting tool is the My-Fyireporting.

  • Normally I don’t like code review questions very much unless they are very specific (which may stop being code review) or trivial, but I will try to do something, it may not be that complicated. Confirm to me if the only different things in the two classes are the name of the RDL file to be used, the query SQL and an existing parameter in one of them. Out of curiosity, this reporter’s original project was abandoned?

  • The file name represents the report, and the query also changes, each report has its query, some querys have parameters, the type may vary. but most have no parameter at all.

  • The question is whether those are the only differences.

  • The original project was abandoned, plus this and a Fork of the original and a new version, managed by Peter Gill.

  • The class has few differences even, most characteristics repeat in the other class, the codes arrive to be exactly equal.

  • 1

    I edited the answer by putting examples. I was avoiding because it’s more complicated than it looks. You probably have mistakes in mine, but at least you do what you need (in the general idea). To see how complicated it is to analyze, the answer that has more votes does not do what you need and still received several votes.

Show 1 more comment

2 answers

8


I will not talk about all the aspects that could be different, how it treats exception, etc. Staying in the focus of the question has the most object oriented path or the most procedural/modular path (although some will say that everything is OOP, after all some like to say that any good solution is OOP and anything bad or is not OOP or is OOP wrong).

Redundancy

The basic technique to eliminate redundancy is to isolate all equal parts and put in an auxiliary method or class. At least when everything is closed.

Eliminating redundancy is not the same as DRY and many people confuse this.

Options

When something is open it can complicate. And this is one of the OOP failures. In general you have to "guess" everything you might need in the future. Of course, this is a widespread problem using any paradigm. When you do OOP and create inheritance, it becomes complicated to improve the base class without running the risk of spoiling the derived classes. Of course this can give a lot of flexibility also in certain scenarios. But inheritance is not the only way to make things more flexible.

Since the classes are almost identical, it would be easy to create only one (possibly abstract) class with equal parts and then inherit from it to create these two. Probably would have to isolate more the responsibilities, mainly of the builders.

It is quite obvious that the strings the name of the RDL file and the query are variables and should be parameterized. It can even be in the constructor (see below) or it can have its own methods for this.

You would have to have some way to inject parameters, perhaps by the constructor itself (see below) or with methods. This could without done in some ways, probably would have to pass some information, probably the name of the parameter in the query, the type and argument.

You can modularize the process further and have a virtual method that does this and is always called at the right time. Obviously it would be empty in the abstract class and would only be overwritten if you have a parameter in the specific derivative (example case RelAlunoAtivoOuInativo).

Or you could have a lambda passed in the constructor that adds the parameter, which would eliminate the need to have a property for this parameter, it will already be encapsulated inside the lambda.

The least OOP way is to just use parameters in the constructor that gets these varying data. The code started doing this by injecting the connection (and it seems to me that I didn’t even need it, but I don’t know, injection anyway always gives more flexibility). Why not do the rest? This way the application would only have the reporting class, it could even be sealed, and the builder of it would receive the parts that vary, which are two strings and a lambda (or if you prefer to do more at hand the 3 data needed to build the addition of the parameter in the query "ativo", DbType.Int32, 1 ou 0).

Do you find OOP less? Okay, create isolated classes with information that varies and insert the specific report class into the report class. Basically it’s just encapsulating all these parameters that you would pass in the constructor in a class and then instead of taking these parameters, you would take the members of the last class (injected). Thus it makes composition and avoids the malignant inheritance, giving enough flexibility and allowing to improve the reporting class with less risk of breaking something.

Of course, in any case a new future requirement can always make it impossible to use this class. Choose your nightmare in future maintenance.

Probably I would do it in a very different way, but without knowing the context nor can I state anything.

And beware of academicisms. There are many people who say that the right way is such without it bringing real advantage.

Inheritance

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) {}
}

I didn’t want to change much, but I took some things that I thought were too much. I know you probably wouldn’t follow this, but I had to. I took a lot of code redundancy that I had for other reasons. This class is more complicated than it seems to need. A lot that exists for no apparent reason. I was going to simplify more, but maybe I want to do it for some reason that I don’t know. I left it in the middle. I also think she has too many responsibilities. And if you solve this problem, maybe she won’t even be needed.

Inheriting:

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)) {}
}

I put in the Github for future reference.

Since it’s to simplify take advantage and do this:

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);
}

Every time someone catches Exception I think you’re doing something wrong. So I took it. You need a better mechanism to deal with errors, so in this way, it’s unnecessary and it’s probably creating redundancies all over the application.

Have you noticed that you don’t need any variable? You don’t even need a normal class? It may be that in some specific context not demonstrated in this code may be useful, but I think all this coding style is something unnecessary.

The proponents of OOP are having trembling seeing this their switch. The ideal would be to have a class that would handle this, so whenever you have a new report, the report itself takes charge of providing a mechanism for your call, avoiding the switch that creates a maintenance problem whenever you have new report. But that’s another subject I think is beyond the scope here.

Composition

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") {}
}

I put in the Github for future reference.

Oopsless

I do not know the specific case, but in many situations none of this is necessary, it can be done simply without harm. Where it is used in one place, where there is no need for unit tests at this point (it does not mean that it does not need in others or that it does not need any test there), so that keep creating classes, instances?

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);
}

I put in the Github for future reference.

Obviously do not test any of this and there may be small problems, mainly because I may have lost myself without having the VS/Resharper available at the moment. I just wanted to run it by.

Certainly other options are still possible.

  • The last example without OOP, is procedural?

  • Yes, it is one of the procedural forms. It is simpler and solves most situations well without bringing any real disadvantage. You can still improve this code, but I didn’t want to mess with your structure too much.

7

Yes, there are several ways to avoid code repetition, and you’re getting off on criticizing such repetition. A basic principle in programming is DRY (don’t repeat yourself) and preaches just that. Categorically, if you are repeating the same code twice, there will ALWAYS be a way to abstract such logic and reuse it. Code repetition is really one of the worst problems when it comes to code maintenance. However, just consider adding extreme complexity to a code just to avoid repeating two or three lines of code (more than that, it’s probably best to abstract).

In your code there are several refactorings to be made, among the basics are:

  • Bilingual code (avoid using Portuguese and English, be consistent).
  • C# Coding standards: Methods are pascal case, members (class) start with undescore, etc
  • "Implicitly Typed Local Variables": the use of "var"implicitly forces the good nomenclature of members, which leads to a code cleaner and readable (including without repetition, see in its code, several times the variable has the same class name, which is unnecessarily repetitive in the same line).

Other refactorings are a little more complex, and require a greater knowledge about object orientation, good practices and patterns, I suggest you take a look at each of them:

  • SRP - Single responsability principle: A class must have only one responsibility
  • ORM: In . Net the most used lonoge ORM framework is the Entity Framework and with its use you avoid having hardcoded queries in your classes, using resources like Migrations and also leave your code testable.
  • Dependency injection: The use of DI (dependency Injection) allows the creation of unit tests (through mocks) and decouple the layers of your project.

Finally, another good practice is the development through "baby-Steps", that is, small steps, avoiding large refactoring which can cause problems in identifying bugs introduced by the same. Combining the use of tests with these "baby-Steps" is recommended (create a test, refactor and then see if the test still passes).

As you have already identified, the first step is really to remove the duplicity of all this code. It is worth remembering once again that, if it needed to repeat, even just once, it is possible to abstract and reuse.

The following example is just an initial suggestion for refactoring, you can over time mainly use a ORM and dependency injection to be able to test your code unitarily, and have the same "hardcoded" SQL code free. In your case, using inheritance, a basic refactoring would be the following (untested code, only for demonstration of how to do the refactoring):

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;
    }
}

With very few changes, a "base class" was extracted with all the functionalities and code that were previously duplicated.

Both classes would look like this:

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 ){ }
}

A suggestion for a first refactoring in the method is to extract the instantiation of its classes from Reports (using a simple Factory).

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)); 
        }
    }
}

With a few minutes it was possible to abstract practically all the duplicity of its code. From here, I suggest you read the suggestions above and continue refactoring. Remember just one small detail: over engeneering. It’s easy to "get lost" in refactoring and want to embrace the world using all the good practices, Patterns design and possible programming principles (and I see this happening day after day). Consider only such refactoring/abstractions with the cost to be implemented and also the final complexity of the code. After all, there is no point in a perfect code if the product has been so late to be developed that it no longer meets, or even a code so full of Patterns that other less experienced developers (who are going to keep the code), take 1 day just to understand what happens...

  • How could I pass a parameter to the class builder RelAlunoAtivoOuInativo which in this case would be the parameter _ativoOuInativo, through the class SelectedReportFactory, it is possible to do this?

  • Yes, there are several alternatives to that. The first, still using inheritance (being simpler and not requiring other refactoring) would be through the creation of a structure for the passage of these parameters. This structure should be generic enough to meet all possible instances of Factory and at the same time should not expose specific details of one instance to another (cohesion).

  • Without knowing the whole context it is difficult to give such detailed directives as this, but depending on it, the use of inheritance can complicate more than resolve, and if this is the case, the next refactoring would be to remove inheritance and introduce the composition.

  • In this case, you would no longer have abstract class and daughter classes, nor a Factory that would solve everything, what would be classes of sole responsibility for each of the functionalities, that make up a so-called "service".

  • This class in turn would be responsible only for delegating the execution to its dependencies, so you have the flexibility to resolve the dependencies (in this case the classes "Relgeralaluno" and "Relalunoativoouinativo") with the required parameters and pass the instance to the "Service" class. Note that in order to use the composition in this way, the logic contained in the constructor must be moved to a method (since the instantiation will occur in the resolution of dependencies and not in the use).

  • It would solve the problem if I create a class that represents all the parameters and pass it through the class constructor SelectedReportFactory?

  • The fact that there are reports with parameters and reports without parameters complicates a little, but it is a requirement, even so I think that inheritance already solves a lot of the problem in view of what was before.

  • Exactly for this reason many preach against the use of inheritance (in favor of composition). Note that even for a simple scenario, the changes start not to make much sense and make your domain lose cohesion (a class that does not need parameters will receive an instance of these "parameters" only with empty attributes. Anyway, it is up to you to check whether this is not so critical according to the context of the project.

Show 3 more comments

Browser other questions tagged

You are not signed in. Login or sign up in order to post.