4

Ao desenvolver uma tela para uma determinada parte de um sistema, me deparo com uma situação clássica de possuir alguns ifs para determinar qual ação deverá ser executada. Logo associei esse caso a exemplos de violação do OCP.

Contudo, vi que a tela é bem coesa e com um escopo definido, onde não vejo, e nem a equipe vê em algum momento próximo, uma possível mudança nas ações. Ou seja, não serão, teoricamente, adicionados novas condições.

Em casos como esse, deve-se seguir o OCP fielmente, criar a classe base e suas extensões, ou manter os ifs? levando em consideração que não devem aparecer novas demandas?

Esse é o código atual que teoricamente viola o OCP.

Controlador:


        [HttpPost]
        public ActionResult InserirOrcamento(OrcamentoViewModel orcamento, string acao)
        {

            if (acao == "consultaCPF")
            {
                var retornoConsultaCPF = OrcamentoService.consultaCPF(orcamento);

                return Json(retornoConsultaCPF, JsonRequestBehavior.AllowGet);
            }

            if (acao == "listaTipoProduto")
            {
                var retornoTipoProduto = orcamento.lstTipoProduto = OrcamentoService.listaTipoProduto(orcamento);

                return Json(retornoTipoProduto, JsonRequestBehavior.AllowGet);
            }
        }

Service

 public class OrcamentoService
    {
        public static OrcamentoViewModel consultaCPF(OrcamentoViewModel orcamento)
        {
            using(ToqueEntities db = new ToqueEntities()){ 

                var retorno = db.PESSOA_FISICA
                                        .Where(x => x.PESF_CPF == orcamento.cpfPessoa)
                                        // .Join(db.CONTATOS, x => x.PES_CDID, c => c.PES_CDID, (x,c) => new { x, c })
                                        .Select(x => new OrcamentoViewModel
                                        {
                                            cpfPessoa = x.PESF_CPF,
                                            nomeClienteOrcamento = x.PESF_NOME,
                                            sobrenomeClienteOrcamento = x.PESF_SOBRENOME,
                                            PES_CDID = x.PES_CDID
                                        }).FirstOrDefault();

                    if (!retorno.Equals(null))
                    {
                        var lstContatos = db.CONTATOS.Where(x => x.PES_CDID == retorno.PES_CDID && x.CON_TIPOCONTATO != "EMAIL")
                        .Select(x => x.CON_CONTEUDO)
                        .ToList();

                            if (!lstContatos.Count().Equals(0))
                            {
                                foreach (var item in lstContatos)
                                {
                                    retorno.Contatos += item + ";";
                                }
                            }
                            else
                            {
                                retorno.Contatos = "";
                            }
                    }

                orcamento.nomeClienteOrcamento = retorno.nomeClienteOrcamento;
                orcamento.sobrenomeClienteOrcamento = retorno.sobrenomeClienteOrcamento;
                orcamento.cpfPessoa = retorno.cpfPessoa;
                orcamento.Contatos = retorno.Contatos;

                return orcamento;
           }
        }

0

2 Respostas 2

5

Open Close Principle

Na minha visão o OCP é o princípio mais complicado de seguir dentro do SOLID. E frequentemente ele é adotado como receita de bolo. Por sorte está questionando ele.

Não sei se está considerando que escrito uma classe não pode mais mexer nela em hipótese alguma, se fosse assim que o princípio pregasse ele seria a coisa mais esdrúxula já inventada. Você pode mexer nela, só tem que tomar cuidado com os contratos.

Note que o que está falando não é mexer nem mesmo na classe, é apenas mudar o método, e até onde entendi muda a implementação sem mexer no resultado final.

Você sempre deve se questionar porque deve fazer algo que mandaram você fazer. Precisa ter uma justificativa, precisa ter um ganho colocar algo em um código, especialmente se ele complica o código e o valor não é muito aparente.

O código

Eu acho que segregar responsabilidades é muito mais importante, e de certa forma demonstra estar fazendo em um ponto (criando o serviço que não sei se deveria ser um, mas enfim, isola). Se você faz isso você isola onde deve fazer modificação.

Parece ter consciência que é necessário dar coesão. Não vou entrar no mérito se está completamente coeso neste caso até por não conhecer todos os detalhes do problema, e não existe fórmula mágica que determine se é ou não coeso. Coesão é importante, seguir princípios não são, pelo menos não na mesmo proporção.

Eu nem posso afirmar que há violação do OCP aí, mas é verdade que tem coisas esquisitas como tratar de uma ação dentro de um método que deveria ser uma ação, eu acho que o problema aí é ferir o SRP porque essa action faz várias ações. Claramente ali são 2 actions diferentes, então está preocupado com um problema que vem depois de outro mais importante. E aí pode ficar mais óbvio a violação do OCP.

O entendimento correto do OCP é que se alguém for fazer alguma outra coisa diferente desta que está fazendo, ou seja, esse mesmo objeto deverá ser criado em um contexto diferente deste, então não deveria tratar tudo no mesmo objeto, deveria criar uma classe herdada desta adicionando a nova funcionalidade, assim dá flexibilidade sem mexer no que existe. Mas isso tem lá seu problema, cria acoplamento, dependência, dificulta certa flexibilidade. Por isso tem uma forma de delegação de função (talvez usando lambda).

Não vejo essa necessidade neste código.

Pra que usar OCP?

Por que deve manter algo aberto para extensão? Se não tiver um motivo não deve. Por exemplo String é um tipo que não está aberto para extensão em condições normais (todos os tipos em C# estão abertos para extensão através dos métodos de extensão, quando criaram esse princípio nem consideraram que uma linguagem poderia ter um mecanismo assim). Há muita gente que defende que as classes deveriam ser sealed por padrão de tão importante que é não deixar aberto o que não precisa. Extensão é problemática, só deve ter isto se for muito útil.

Por que o seu caso precisa ficar aberto para extensão?

E por que esta classe deveria ser fechada para modificação?

Essa ideia foi criada para dar uma certa estabilidade, garantir que sempre que alguém chamar isso acontecerá o mesmo. É uma boa ideia, mas já viu engenheiros consagrados sair fazendo isso em todo lugar?

Eles são pragmáticos, eles sabem que tentar fazer isso é complicado, em muitos casos é melhor até ter que fazer outras coisa e tornar o atual obsoleto em alguns casos extremos do que complicar algo para dar uma possível estabilidade e flexibilidade que talvez nunca seja necessária.

OCP no MVC

Pro meu entendimento o jeito que se faz o MVC não costuma causar esses problemas que são ditos se manter a classe aberta para modificação. Claro que tem que tomar algum cuidado, mas não é algo complicado, e de fato as pessoas mexem muito nessas classes que são ações, estendendo a capacidade do controlador ali mesmo. O mecanismo nem foi tão pensado em para ficar fechado para modificação e aberto para extensão.

Se alguém apresentar um possível problema eu posso mudar de ideia.

Outros problemas

Nem vou falar esses Equals() que são horríveis, apesar de funcionar. Mas isso pode ser um indício que não estão pensando com clareza no que fazer. Pode ser só estilo ruim escolhido e ter sido pensado. Tenho mais medo desse acao.executarAcao() em um dado que é do tipo String que me parece errado demais. Então tem aí preocupações muito mais sérias. Também não falei do problema da concatenação de strings que é mais básico. E não olhei se outras partes não tem problemas, ou se o ToList() é necessário aí ou está usado indevidamente como quase todo mundo faz. Também não parece muito adequado um método chamado ConsultaCPF() modificar o conteúdo do orcamento, ou até se ele deve ser mudado, isso é muito pior que violar o OCP. Parece que o código tem muitos outros problemas mais básicos do que seguir a modinha do SOLID.

Coloquei no GitHub para referência futura.

6
  • agradeço por comentar esse ponto do acao.executarAcao(), não tinha visto que havia colocado, ele n faz parte do código em questão, faz parte da minha tentativa de implementar o OCP, eu vou até editar. Commented 27/11/2019 às 14:37
  • Eu coloquei uma string acao pois são ações executadas via Ajax. Por exemplo, o consultaCPF é para quando o usuário digitar o CPF e tirar o foco do input, ai é feita a consulta e retornas os dados dessa pessoa nos respectivos inputs, como o nome e o sobrenome. Ou seja tudo tem de estar nessa View InserirOrcamento para que depois eu submeta o formulário e consiga dar o devido processamento. Mas vou tentar quebrar em 3 Actions diferentes, agradeço a observação. Commented 27/11/2019 às 14:52
  • 2
    Isso não é uma view, isso é uma action de um controller. EU ach oque não está sabendo usar o MVC corretamente precisa resolver isso, você não deveria colocar uma action dentro de outra action, precisa reformular do esse controller, mesmo eu não vendo o resto.
    – Maniero
    Commented 27/11/2019 às 14:57
  • Sim, eu acabei de refatorar esse Controller, separei as 2 actions que estavam dentro dele. Bem mais organizado e de melhor manutenção. Realmente o problema do SRP estava bem mais latente e poderia vir a dificultar uma manutenção no futuro. Eu acabei me expressando errado na colocação acima, quando disse "view", me referia à view vinculada àquele controller . Separando melhor, vi que poderia retornar apenas o JSON e deixar o JS lidar com o preenchimento dos campos. Commented 27/11/2019 às 15:08
  • Como está usando como WebAPI, nem view tem.
    – Maniero
    Commented 27/11/2019 às 15:10
0

Sugiro estudar um pouco sobre a diferença entre o Any(), All(), Count() e Count, para que não tenha problemas de performance no código e legibilidade.

Por exemplo, trocar "!lstContatos.Count().Equals(0)" por lstContatos.Any() ou caso a variável seja uma List (como nesse caso, mesmo achando que não é necessário que seja) pode ser utilizado lstContatos.Count > 0.

Na minha opinião "!retorno.Equals(null)" fica bem ilegível, talvez trocar por "retorno != null" seja mais interessante.

Outro problema é a concatenação de strings, estude o quantidade de concatenações que serão feitas, e dependendo dela, verifique a necessidade de utilizar StringBuilder (mais performática para grandes quantidades de concatenações).

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 .