PAREANDO: Como você escreveria esse código? Consegue deixar ele mais simples?
Olá pessoal. Tudo certo!?
Eis um novo “tipo de post” que gostaria de propor. Apresento um pequeno bloco de código, algumas considerações, e conto com a ajuda de vocês para chegarmos a uma boa implementação.
Para começar, gostaria de propor uma revisão de um pequeno trecho de código do meu post anterior.
Ponto de partida
Considere o seguinte método:
public override Task WriteToStreamAsync(Type type, object value, Stream stream,
HttpContentHeaders contentHeaders,
TransportContext transportContext)
{
return Task.Factory.StartNew(() => {
var contacts = (value is Contact)
? new[] {value as Contact}
: value as IEnumerable<Contact>;
using (var sw = new StreamWriter(stream))
{
foreach (var contact in contacts)
{
sw.WriteLine("BEGIN:VCARD");
sw.WriteLine("VERSION:2.1");
sw.WriteLine("N:{0};{1}", contact.LastName,
contact.FirstName);
sw.WriteLine("FN:{0} {1}", contact.FirstName,
contact.LastName);
sw.WriteLine("EMAIL:{0}", contact.Email);
sw.WriteLine("END:VCARD");
}
}
});
}
O que não gosto nessa solução?
- Método está fazendo muita coisa. Logo, é difícil de testar;
- Método está grande (muitas linhas);
O que eu gosto?
- Código é simples;
- Lógica está explícita
Refactoring óbvio
Comecemos com o refactoring mais evidente. Observe:
public override Task WriteToStreamAsync(Type type, object value, Stream stream,
HttpContentHeaders contentHeaders,
TransportContext transportContext)
{
return Task.Factory.StartNew(() => {
using (var sw = new StreamWriter(stream))
{
foreach (var contact in ToContacts(value))
WriteContactTo(contact, sw);
}
});
}
private IEnumerable<Contact> ToContacts(object source)
{
if (source is Contact)
return new [] { source as Contact };
return source as IEnumerable<Contact>;
}
private void WriteContactTo(Contact contact, StreamWriter sw)
{
sw.WriteLine("BEGIN:VCARD");
sw.WriteLine("VERSION:2.1");
sw.WriteLine("N:{0};{1}", contact.LastName,
contact.FirstName);
sw.WriteLine("FN:{0} {1}", contact.FirstName,
contact.LastName);
sw.WriteLine("EMAIL:{0}", contact.Email);
sw.WriteLine("END:VCARD");
}
O que fizemos?
- Começamos extraíndo blocos evidentes de código da função principal;
- Simplifiquei drasticamente o método que gera o “formato”.
Do que ainda não gosto?
- Minha solução usa métodos privados e, atualmente, para mim, isso é um “bad smell”;
Extension Methods?
Como disse, tenho uma certa “implicância” com métodos privados. Eles são difícieis de testar (já que o teste deve ser indireto). Além disso, é difícil fazer reaproveitamento.
Comecemos com um Extension Method simples. Veja:
public static class EnumerableExtensions
{
public static IEnumerable<T> AsEnumerable<T>(this object source)
{
if (source is IEnumerable<T>)
return source as IEnumerable<T>;
return new[] {(T) source};
}
}
Vantagem? Nosso método era privado. Agora é público, testável e reaproveitável.
Quem sabe fazer o mesmo com o método que gera o vCard?
Vejamos:
public static class ContactVCardExtensions
{
public static void WriteVCardTo(this IEnumerable<Contact> contacts, Stream stream)
{
using (var sw = new StreamWriter(stream))
{
contacts
.WriteVCardTo(sw);
}
}
public static void WriteVCardTo(this IEnumerable<Contact> contacts, StreamWriter sw)
{
foreach (var contact in contacts)
contact.WriteVCardTo(sw);
}
public static void WriteVCardTo(this Contact contact, StreamWriter sw)
{
sw.WriteLine("BEGIN:VCARD");
sw.WriteLine("VERSION:2.1");
sw.WriteLine("N:{0};{1}", contact.LastName,
contact.FirstName);
sw.WriteLine("FN:{0} {1}", contact.FirstName,
contact.LastName);
sw.WriteLine("EMAIL:{0}", contact.Email);
sw.WriteLine("END:VCARD");
}
}
Vejamos como está nosso método?!
public override Task WriteToStreamAsync(Type type, object value, Stream stream,
HttpContentHeaders contentHeaders,
TransportContext transportContext)
{
return Task.Factory.StartNew(() =>
value
.AsEnumerable<Contact>()
.WriteVCardTo(stream)
);
}
O bom é que dá para testar tudo. Também temos possibilidade algum reaproveitamento.
Extension Methods!? Acho que não.
Extension Methods geralmente indicam oportunidades de revisão de design.
Vamos tentar escrever nosso pequeno conjunto de métodos “úteis” como algo mais OO. Veja:
public sealed class ContactsVCardWriter
{
private readonly IEnumerable<Contact> source;
public ContactsVCardWriter(IEnumerable<Contact> source)
{
Throw.IfArgumentNull(source, "source");
this.source = source;
}
public Task WriteToAsync(Stream stream)
{
Throw.IfArgumentNull(stream, "stream");
return Task.Factory.StartNew(() => WriteTo(stream));
}
public void WriteTo(Stream stream)
{
Throw.IfArgumentNull(stream, "stream");
using (var sw = new StreamWriter(stream))
{
WriteTo(sw);
}
}
public void WriteTo(StreamWriter sw)
{
Throw.IfArgumentNull(sw, "stream");
foreach (var contact in source)
WriteVCardTo(contact, sw);
}
private void WriteVCardTo(Contact contact, StreamWriter sw)
{
sw.WriteLine("BEGIN:VCARD");
sw.WriteLine("VERSION:2.1");
sw.WriteLine("N:{0};{1}", contact.LastName,
contact.FirstName);
sw.WriteLine("FN:{0} {1}", contact.FirstName,
contact.LastName);
sw.WriteLine("EMAIL:{0}", contact.Email);
sw.WriteLine("END:VCARD");
}
}
Agora sim! Temos uma classe testável e rica. Além disso, reaproveitável. Repare, inclusive que a possibilidade de testar tornou nosso código mais “consistente”. Afinal, estamos validando argumentos.
Nosso método!?
public override Task WriteToStreamAsync(Type type, object value, Stream stream,
HttpContentHeaders contentHeaders,
TransportContext transportContext)
{
var writer = new ContactsVCardWriter(value.AsEnumerable<Contact>());
return writer.WriteToAsync(stream);
}
Para pensar
O código final, embora mais testável, ficou maior. Isso é um problema?
O que você teria feito?
Espero Gists e propostas.
Grande Elemar,
Achei muito legal o tipo de post, code review é sempre positivo e nos ensina muito.
Tem uma coisa que não gosto muito no código que é ficar testando argumentos para ver se eles são null. Faço isso somente quando um “null” poderia ter um efeito colateral permanente.
No caso acima, vejo a linha “Throw.IfArgumentNull(source, “source”);” somente como poluição, visto que não soma muito a legibilidade. Imagine que tivéssemos 3 argumentos, você colocaria 3 linhas ali?
Minha regra de outro é “nunca passe null” e nunca faça um método que aceite null como algo válido.
Qual sua opinião sobre isso?
Grande abraço!
André Carlucci
C#, infelizmente, não tem uma forma efetiva e global para representar vazio e “sem valor” . Por isso, o valor semântico de Null é questionável. As vezes, é necessário.
Acredito em programação defensiva. Acho relevante que os métodos validem seus parâmetros e falhem “softly” quando receberem valor inadequado.
Acho importante dar o feedback correto. Se null não é aceitável como valor de parâmetro, essa deverá ser a exception. Ganha-se tempo de depuração.
Entendo que um acordo entre quem desenvolve e quem consome APIs poderia salvar algumas linhas de validação. Entretanto, para times grandes, isso pode ser confuso.
defendo programação defensiva por entender que ela salva “enganos”
Era isso.
Para que aquele parâmetro Type se vc nunca usou ele?
É uma implementação do formatter do web api. É um override, man.
Ai Elemar, ficou 100% esse post.
Cara, muito bom mesmo. Justamente nessa semana estava conversando com um colega sobre a importância de dividir os métodos de acordo com suas responsabilidade para que não haja métodos muitos longos.
Excelente! Coesão alta, e baixo acoplamento.Mas poderíamos generalizar a classe(facilmente), e até aplicar em vCard.Acredito que essa seria a solução final, não?
não sei se entendi. O que acha de esboçar a ideia com um gist?!
Quanto ao número de linhas, acredito que temos que pensar a longo prazo.Como exemplo, ouso pegar a própria OO, em que se você olhar superficialmente, temos um número maior de arquivos e linhas de código, comparado à programação procedural.Mas é o reaproveitamento do código, a utilização do mesmo, que torna o código menor, e o mais importante: mais legível.
Bem observado. Em geral, concordo com você. Entretanto, penso que esse aumento na abstração também promove aumento de complexidade acidental. Por isso, não penso que esse “carinho” com o código deva ser aplicado como regra.
O problema é: como definir o momento de parar?
Acho que a pergunta deveria ser pelo outro lado, quando devemos ter carinho e não quando devíamos parar de ter carinho.
rabens pelo excelente blog!
Concordo contigo em alguns pontos. Realmente “o carinho” com o código pode trazer algumas complexidades, mas isto se paga com o propósito principal da O.O. que é o s.
Talvez não vejamos beneficio em um componente que será usado uma única vez, mas isto tem valor para componentes que estarão presentes em diversos istemas.
i em meu cenário 50% do budget da gerencia de desenvolvimento é para manutenção em dois sistemas, e isso se justifica devido a grande complexidade que é manter esses sistemas legados e suas amarrações que tornam o código altamente acoplado.
quando da construção desses sistemas, esse “carinho” tivesse sido apliado, tenho certeza que este custo hoje seria menor.
Acho que então o quando começar e quando parar pode ser determinado pelo tipo de desenvolvimento que estoufazendo. Acho que sempre devemos pregar pela legibilidade do código, afinal de contas, aalguem um dia terá que mante-lo, ms o quanto de tempo será dedicado pode se determinar pelo tipo de componente.
Outra duvida quetenho ése no desenvolvimento da solução acima se voce tivesse começado pelos testes se teria chegado no mesmo resultado ja da primeira vez, ja que o design seria enriquecido pelos testes que tornariam suaapi publica visível desde o inicio.
Abraços
Aí está, Magoo
As vezes, gosto de escrever código sem escrever os testes. Gosto de “ver funcionando” antes. Depois, aplico os testes.
No geral, concordo com você. Mas, perceba que gosto de questionar as coisas. A primeira solução é “menos testável”, mas, é mais “direta”.
Elemar,
Tambem sou bastante questionador.
Acredito que em uma POC é natural não se ter os testes primeiro, mas em um projeto real a falta de testes pode comprometer a qualidade do trabalho já que pode ser que ao concluir o desenvolvimento não terei tempo habil para fazer os testes de forma eficiente.
Ao mesmo tempo, como deficiente visual, o TDD é a melhor maneira de garantir que estou fazendo um design aderente as boas práticas da OO já que se eu começar primeiro pelas implementações corro sérios riscos de cair no BDUF.
Além disso, esse meu trabalho com os testes pode permitir, via ferramentas de design e engenharia reversa gerar um modelo para discussão com demais membros da equipe antes mesmo de implementar (enquanto ainda tenho mocks) e caso todos concordem, ou não encontremos nada que invalide meu raciocionio, continuar implementado
Como eu brinquei com voce no DNAD sobre suas palestras, seria uma das formas de eu “pintar as caixinhas”.
Voltando agora pra implementação concordo tambem com a programação defensiva. É dificil garantir que nenhum desenvolvedor usuário de nossa API em algum momento não vai passar valores inválidos a nosso código e informá-lo disso da maneira mais clara possível poupará horas de debug por parte dele e diversos chamados em sua caixa pois sempre o primeiro culpado é o framework que o cara está usando.
No demais continue com o excelente trabalho no blog!
Abraços