Lançar exceção ou deixar o código falhar

52

Gostaria de saber se existem prós e contras contra esse estilo:

private void LoadMaterial(string name)
{
    if (_Materials.ContainsKey(name))
    {
        throw new ArgumentException("The material named " + name + " has already been loaded.");
    }

    _Materials.Add(
        name,
        Resources.Load(string.Format("Materials/{0}", name)) as Material
    );
}

Esse método deve, para cada um name, ser executado apenas uma vez. _Materials.Add()emitirá uma exceção se for chamado várias vezes para o mesmo name. Minha guarda, como resultado, é completamente redundante ou há alguns benefícios menos óbvios?

Isso é C #, Unity, se alguém estiver interessado.

assíncrono
fonte
3
O que acontece se você carregar um material duas vezes sem a proteção? Será que _Materials.Addlançar uma exceção?
usar o seguinte comando
2
Na verdade, eu já mencionei em lança uma exceção: P
assíncrono
9
Notas secundárias: 1) Considere o uso de string.Formatconcatenação de cadeia de caracteres para criar a mensagem de exceção. 2) use apenas asse você espera que o elenco falhe e verifique o resultado null. Se você espera obter sempre um Material, use um elenco como (Material)Resources.Load(...). Uma exceção de conversão clara é mais fácil de depurar do que uma exceção de referência nula que ocorre posteriormente.
CodesInChaos
3
Nesse caso específico, os chamadores também podem achar útil um método complementar ( LoadMaterialIfNotLoadedou ReloadMaterialesse nome pode usar melhorias).
Jpmc26
11
Em outra nota: este padrão é aceitável para adicionar um item a uma lista. No entanto, não use esse padrão para preencher uma lista inteira, pois você encontrará uma situação O (n ^ 2) em que, com listas grandes, você encontrará problemas de desempenho. Você não notará isso em 100 entradas, mas em 1000 entradas você certamente e em 10.000 entradas, será um grande gargalo.
Pieter B

Respostas:

109

O benefício é que sua exceção "personalizada" possui uma mensagem de erro significativa para quem chama essa função sem saber como ela é implementada (que no futuro pode ser você!).

É verdade que, nesse caso, eles provavelmente poderiam adivinhar o que significava a exceção "padrão", mas você ainda está deixando claro que eles violaram seu contrato, em vez de tropeçarem em algum bug estranho no seu código.

Ixrec
fonte
3
Aceita. É quase sempre melhor lançar uma exceção para uma violação de pré-condição.
precisa saber é o seguinte
22
"O benefício é que sua exceção" personalizada "possui uma mensagem de erro significativa para qualquer pessoa que chama essa função sem saber como ela é implementada (que no futuro pode ser você!)." Melhores conselhos.
assíncrono
2
"Explícito é melhor que implícito." - Zen de Python
jpmc26
4
Mesmo que o erro gerado _Materials.Addnão seja o erro que você deseja transmitir ao chamador, acho que esse método de rotular novamente o erro é ineficiente. Para cada sucesso chamada de LoadMaterial(operação normal), você vai ter feito o mesmo teste de sanidade duas vezes , em LoadMateriale depois novamente em _Materials.Add. Se mais camadas forem agrupadas, cada uma empregando o mesmo estilo, você poderá ter muitas mais vezes o mesmo teste.
Marc van Leeuwen
15
... Em vez disso, considere mergulhar _Materials.Addincondicionalmente, depois pegue um erro em potencial e, no manipulador, lance um erro diferente. O trabalho extra agora é feito apenas em casos errôneos, o caminho de execução excepcional em que você realmente não se preocupa com a eficiência.
Marc van Leeuwen
100

Eu concordo com a resposta do Ixrec . No entanto, você pode considerar uma terceira alternativa: tornar a função idempotente. Em outras palavras, retorne cedo, em vez de jogar um ArgumentException. Isso geralmente é preferível se você for forçado a verificar se ele já foi carregado antes de ligar LoadMaterialtodas as vezes. Quanto menos condições prévias você tiver, menor será a carga cognitiva no programador.

Lançar uma exceção seria preferível se for realmente um erro do programador, onde deve ser óbvio e conhecido em tempo de compilação se o material já foi carregado, sem ter que verificar em tempo de execução antes de chamar.

Karl Bielefeldt
fonte
2
Por outro lado, a falha silenciosa de uma função pode causar um comportamento inesperado, resultando em erros mais difíceis de serem encontrados posteriormente.
Philipp
30
@ Philipp a função não falharia silenciosamente. Ser idempotente significa que executá-lo muitas vezes tem o mesmo efeito que executá-lo uma vez. Em outras palavras, se o material já estiver carregado, nossa especificação ("o material deve ser carregado") estará satisfeita, portanto, não precisamos fazer nada. Podemos apenas voltar.
Warbo
8
Eu gosto mais disso, na verdade. Obviamente, dependendo das restrições fornecidas pelos requisitos dos aplicativos, isso pode estar errado. Por outro lado, sou um fã absoluto de métodos idempotentes.
FP
6
@ Philipp, se pensarmos sobre isso, LoadMaterialtem as seguintes restrições: "Depois de chamá-lo, o material é sempre carregado e o número de materiais carregados não diminui". Lançar uma exceção para uma terceira restrição "O material não pode ser adicionado duas vezes" parece bobagem e contra-intuitivo para mim. Tornar a função idempotente reduz a dependência do código na ordem de execução e no estado do aplicativo. Parece uma vitória dupla para mim.
Benjamin Gruenbaum
7
Gosto dessa idéia, mas sugiro uma pequena alteração: retorne um booleano refletindo se algo foi carregado. Se o chamador realmente se importa com a lógica do aplicativo, ele pode verificar isso e lançar uma exceção.
user949300
8

A pergunta fundamental que você deve fazer é: qual a interface para sua função? Em particular, a condição é _Materials.ContainsKey(name)uma pré-condição da função?

Se é não uma condição prévia, a função deve fornecer um resultado bem definido para todos os vales possíveis de name. Nesse caso, a exceção lançada se namenão fizer parte _Materialsse torna parte da interface da função. Isso significa que ele precisa fazer parte da documentação da interface e, se você decidir alterar essa exceção no futuro, será uma alteração na interface .

A questão mais interessante é o que acontece se for uma pré-condição. Nesse caso, a pré-condição em si se torna parte da interface da função, mas a maneira como uma função se comporta quando essa condição é violada não é necessariamente parte da interface.

Nesse caso, a abordagem que você postou, verificando violações de pré-condições e relatando o erro, é conhecida como programação defensiva . A programação defensiva é boa, pois notifica o usuário logo que cometeu um erro e chamou a função com um argumento falso. É ruim, pois aumenta significativamente a carga de manutenção, pois o código do usuário pode depender que a função lide com violações de pré-condição de uma certa maneira. Em particular, se você achar que a verificação de tempo de execução se tornará um gargalo de desempenho no futuro (improvável para um caso tão simples, mas bastante comum para pré-condições mais complexas), talvez não seja mais possível removê-la.

Acontece que essas desvantagens podem ser muito significativas, o que deu à programação defensiva uma má reputação em certos círculos. No entanto, o objetivo inicial ainda é válido: queremos que os usuários de nossa função notem mais cedo quando cometerem um erro.

Portanto, hoje em dia muitos desenvolvedores propõem uma abordagem ligeiramente diferente para esses tipos de problemas. Em vez de lançar uma exceção, eles usam um mecanismo semelhante a uma declaração para verificar pré-condições. Ou seja, as pré-condições podem ser verificadas nas compilações de depuração para ajudar os usuários a detectar erros desde o início, mas eles não fazem parte da interface de funções. A diferença pode parecer sutil à primeira vista, mas pode fazer uma enorme diferença na prática.

Tecnicamente, chamar uma função com pré-condições violadas é um comportamento indefinido. Mas a implementação pode decidir detectar esses casos e notificar o usuário imediatamente, se isso acontecer. Infelizmente, as exceções não são uma boa ferramenta para implementar isso, pois o código do usuário pode reagir a elas e, portanto, começar a depender da presença delas.

Para obter uma explicação detalhada dos problemas com a abordagem defensiva clássica, bem como uma possível implementação para verificações de pré-condição no estilo de afirmação, consulte a Programação Defensiva de John Lakos, realizada diretamente no CppCon 2014 ( Slides , Video ).

ComicSansMS
fonte
4

Já existem algumas respostas aqui, mas aqui está a resposta que leva em consideração o Unity3D (a resposta é muito específica para o Unity3D, eu faria isso muito diferentemente na maioria dos contextos):

Em geral, o Unity3D não usa exceções tradicionalmente. Se você lançar uma exceção no Unity3D, não será como em um aplicativo .NET comum, a saber, não interromperá o programa, na maioria das vezes você poderá configurar o editor para pausar. Apenas será registrado. Isso pode facilmente colocar o jogo em um estado inválido e criar um efeito em cascata que dificulta o rastreamento dos erros. Então, eu diria que, no caso da Unity, deixar Addlançar uma exceção é uma opção especialmente indesejável.

Mas examinar a velocidade das exceções não é um caso de otimização prematura em alguns casos, devido ao modo como o Mono funciona no Unity em algumas plataformas. De fato, o Unity3D no iOS suporta algumas otimizações avançadas de script, e as exceções desabilitadas * são um efeito colateral de uma delas. Isso é realmente algo a considerar, porque essas otimizações se mostraram muito valiosas para muitos usuários, mostrando um argumento realista para considerar a limitação do uso de exceções no Unity3D. (* exceções gerenciadas do mecanismo, não seu código)

Eu diria que no Unity você pode querer adotar uma abordagem mais especializada. Ironicamente, um muito baixo votado resposta no momento da escrita deste , mostra uma maneira que eu poderia implementar algo parecido com isto especificamente no contexto da Unity3D (em outro lugar algo como isso realmente é inaceitável, e até mesmo em Unity é bastante deselegante).

Outra abordagem que eu consideraria na verdade não é indicar um erro no que diz respeito ao chamador, mas usar as Debug.LogXXfunções. Dessa forma, você obtém o mesmo comportamento de lançar uma exceção não tratada (por causa de como o Unity3D lida com eles) sem correr o risco de colocar algo em um estado estranho na linha. Considere também se isso realmente é um erro (a tentativa de carregar o mesmo material duas vezes necessariamente é um erro no seu caso? Ou pode ser um caso onde Debug.LogWarningé mais aplicável).

E no que diz respeito ao uso de coisas como Debug.LogXXfunções em vez de exceções, você ainda precisa considerar o que acontece quando uma exceção é lançada de algo que retorna um valor (como GetMaterial). Eu costumo abordar isso passando nulo junto com o log do erro ( novamente, apenas no Unity ). Em seguida, uso verificações nulas em meus MonoBehaviors para garantir que qualquer dependência, como um material, não seja um valor nulo e desabilito o MonoBehavior, se for o caso. Um exemplo para um comportamento simples que requer algumas dependências é algo como isto:

    public void Awake()
    {
        _inputParameters = GetComponent<VehicleInputParameters>();
        _rigidbody = GetComponent<Rigidbody>();
        _rigidbodyTransform = _rigidbody.transform;
        _raycastStrategySelector = GetComponent<RaycastStrategySelectionBehavior>();

        _trackParameters =
            SceneManager.InstanceOf.CurrentSceneData.GetValue<TrackParameters>();

        this.DisableIfNull(() => _rigidbody);
        this.DisableIfNull(() => _raycastStrategySelector);
        this.DisableIfNull(() => _inputParameters);
        this.DisableIfNull(() => _trackParameters);
    }

SceneData.GetValue<>é semelhante ao seu exemplo, pois chama uma função em um dicionário que gera uma exceção. Mas, em vez de lançar uma exceção, ele usa, Debug.LogErrorque fornece um rastreamento de pilha como uma exceção normal faria e retornaria nulo. As verificações a seguir * desabilitarão o comportamento em vez de permitir que ele continue a existir em um estado inválido.

* os cheques se parecem com isso por causa de um pequeno auxiliar que eu uso que imprime uma mensagem formatada quando desativa o objeto do jogo **. Verificações nulas simples com iftrabalho aqui (** as verificações do auxiliar são compiladas apenas nas compilações de depuração (como afirmações). O uso de lambdas e expressões como essa no Unity pode prejudicar o desempenho)

Selali Adobor
fonte
11
+1 por adicionar informações genuinamente novas à pilha. Eu não estava esperando isso.
Ixrec
3

Gosto das duas respostas principais, mas quero sugerir que os nomes das suas funções possam ser melhorados. Estou acostumado a Java, então YMMV, mas um método "add" não deve, IMO, lançar uma exceção se o item já estiver lá. Ele deve adicionar o item novamente ou, se o destino for um Conjunto, não faça nada. Como esse não é o comportamento de Materials.Add, ele deve ser renomeado como TryPut ou AddOnce ou AddOrThrow ou semelhante.

Da mesma forma, seu LoadMaterial deve ser renomeado para LoadIfAbsent ou Put ou TryLoad ou LoadOrThrow (dependendo de você usar a resposta nº 1 ou nº 2) ou algo parecido.

Siga as convenções de nomenclatura do C # Unity, sejam elas quais forem.

Isso será especialmente útil se você tiver outras funções AddFoo e LoadBar, nas quais é permitido carregar a mesma coisa duas vezes. Sem nomes claros, os desenvolvedores ficarão frustrados.

user949300
fonte
Há uma diferença entre a semântica Dictionary.Add () do C # (consulte msdn.microsoft.com/en-us/library/k7z0zy8k%28v=vs.110%29.aspx ) e a semântica Java Collection.add () (consulte a documentação. oracle.com/javase/8/docs/api/java/util/… ). C # retorna nulo e lança exceção. Considerando que, Java retorna bool e substitui o valor armazenado anteriormente pelo novo valor ou lança uma exceção quando o novo elemento não pode ser adicionado.
Kasper van den Berg
Kasper - obrigado e interessante. Como você substitui um valor em um dicionário C #? (Equivalente a um Java put ()). Não veja um método incorporado nessa página.
user949300
boa pergunta, alguém poderia fazer Dictionary.Remove () (consulte msdn.microsoft.com/en-us/library/bb356469%28v=vs.110%29.aspx ) seguido por um Dictionary.Add () (consulte msdn.microsoft .com / pt-br / library / bb338565% 28v = vs.110% 29.aspx ), mas isso parece um pouco estranho.
Kasper van den Berg
@ user949300 dicionário [key] = value substitui a entrada se ele já existe
Rupe
@Rupe e, presumivelmente, joga algo se não o fizer. Tudo parece muito estranho para mim. A maioria dos clientes que criam o dict não se importam wheteher uma entrada foi lá, eles só se preocupam que, após a sua código de configuração, que é lá. Marque um para a API Java Collections (IMHO). O PHP também é desagradável com dictos, foi um erro do C # seguir esse precedente.
user949300
3

Todas as respostas acrescentam idéias valiosas, gostaria de combiná-las:

Decida a semântica pretendida e esperada da LoadMaterial()operação. Existem pelo menos as seguintes opções:

  • Uma pré-condição em nameLoadedMaterials : →

    Quando a pré-condição é violada, o efeito de LoadMaterial()não é especificado (como em resposta pelo ComicSansMS ). Isso permite a maior liberdade na implementação e futuras alterações de LoadMaterial(). Ou,

  • efeitos da chamada LoadMaterial(name)com nameLoadedMaterials são especificados; ou:

    • a especificação afirma que uma exceção é lançada; ou
    • a especificação afirma que o resultado é idempotente (como em resposta por Karl Bielefeldt ),

Ao decidir a semântica, você deve escolher uma implementação. As seguintes opções e considerações foram propostas:

  • Lance uma exceção personalizada (como sugerido pelo Ixrec ) →

    O benefício é que sua exceção "personalizada" possui uma mensagem de erro significativa para quem chama essa função - Ixrec e User16547

    • Para evitar o custo de verificar repetidamente nameLoadedMaterials, você pode seguir o conselho de Marc van Leeuwen :

      ... Em vez disso, considere mergulhar em _Materials.Add incondicionalmente, depois pegue um erro em potencial e, no manipulador, lance um erro diferente.

  • Vamos Dictionay.Add lançar a exceção →

    O código de lançamento da exceção é redundante. - Jon Raynor

    Embora a maioria dos eleitores concorde mais com o Ixrec

    Razões adicionais para escolher esta implementação são:

    • que os chamadores já podem lidar com ArgumentExceptionexceções e
    • para evitar perder informações da pilha.

    Mas se esses dois motivos forem importantes, você também poderá derivar sua exceção personalizada ArgumentExceptione usar o original como uma exceção encadeada.

  • Torne-o LoadMaterial()idempotente como resposta por Karl Bielefeldt e com mais votos (75 vezes).

    As opções de implementação para esse comportamento:

    • Verifique com Dictionary.ContainsKey ()

    • Sempre chame Dictionary.Add () captura o que ArgumentExceptionele lança quando a chave a inserir já existe e ignora essa exceção. Documente que ignorar a exceção é o que você pretende fazer e por quê.

      • Quando LoadMaterials()é (quase) sempre chamado uma vez para cada um name, isso evita o custo de verificar repetidamente nameLoadedMaterials cf. Marc van Leeuwen . Contudo,
      • quando LoadedMaterials()muitas vezes é chamado várias vezes para o mesmo name, isso incorre no custo (caro) de lançar ArgumentExceptione desenrolar a pilha.
    • Pensei que existisse um TryAddanálogo -method para TryGet () que permitiria evitar a exceção cara de lançar e empilhar o desenrolamento de chamadas com falha para Dictionary.Add.

      Mas esse TryAddmétodo parece não existir.

Kasper van den Berg
fonte
11
+1 por ser a resposta mais abrangente. Provavelmente isso vai muito além do que o OP originalmente buscava, mas é um resumo útil de todas as opções.
Ixrec
1

O código de lançamento da exceção é redundante.

Se você ligar para:

_Materials.Add (name, Resources.Load (string.Format ("Materials / {0}", name)) como Material

com a mesma tecla, ele lançará um

System.ArgumentException

A mensagem será: "Um item com a mesma chave já foi adicionado."

A ContainsKeyverificação é redundante, pois essencialmente o mesmo comportamento é alcançado sem ela. O único item diferente é a mensagem real na exceção. Se houver um verdadeiro benefício em receber uma mensagem de erro personalizada, o código de proteção terá algum mérito.

Caso contrário, eu provavelmente refatoraria a guarda neste caso.

Jon Raynor
fonte
0

Eu acho que isso é mais uma questão sobre o design da API do que uma questão de convenção de codificação.

Qual é o resultado esperado (o contrato) da chamada:

LoadMaterial("wood");

Se o chamador pode esperar / é garantido que, depois de chamar esse método, o material "madeira" seja carregado, não vejo motivo para lançar uma exceção quando esse material já estiver carregado.

Se ocorrer um erro ao carregar o material, por exemplo, a conexão com o banco de dados não pôde ser aberta ou não houver material "madeira" no repositório, é correto lançar uma exceção para notificar o responsável pela chamada sobre esse problema.

oɔɯǝɹ
fonte
-2

Por que não mudar o método para que ele retorne 'true' se a matéria for adicionada e 'false' se não foi?

private bool LoadMaterial(string name)
{
   if (_Materials.ContainsKey(name))

    {

        return false; //already present
    }

    _Materials.Add(
        name,
        Resources.Load(string.Format("Materials/{0}", name)) as Material
    );

    return true;

}
MrIveck
fonte
15
As funções que retornam valores de erro são exatamente o antipadrão que as exceções devem tornar obsoletas.
Philipp
Esse sempre é o caso? Eu pensei que era apenas o caso do semipredicado ( pt.wikipedia.org/wiki/Semipredicate_problem ) Porque, no exemplo de código OP, deixar de adicionar um material ao sistema não é um problema real. O método visa garantir que o material esteja no sistema quando você o executa, e é exatamente isso que esse método faz. (mesmo que falhe) O booleano retornado apenas indica se o método já tentou adicionar esse assunto ou não. ps: Eu não levo em consideração que poderia haver vários assuntos com o mesmo nome.
precisa saber é o seguinte
11
Acho que encontrei a solução mim mesmo: en.wikipedia.org/wiki/... Isso aponta que a resposta de Ixrec é o mais correto
MrIveck
@ Philipp No caso em que o codificador / especificação decidiu que não há erro ao tentar carregar duas vezes. O valor de retorno é, nesse sentido, não um valor de erro, mas um valor informativo.
user949300