• Revista PROGRAMAR: Já está disponível a edição #53 da revista programar. Faz já o download aqui!

Nazgulled

Algum problema em ter o meu código assim?

10 mensagens neste tópico

A dúvida que tenho pode parecer um bocado estúpida mas decidi perguntar aqui na mesma porque eu simplesmente olho para este bocado de código que acabei de fazer e não sei porquê tenho um bad feeling, não sei, mas não me soa lá muito bem. Cá para mim existe algum problema nisto (apesar de funcionar) e/ou poderei melhorar o código.

Tenho a seguinte função:

private void fnCall_1(PropertyInfo propInfo, object newValue, object defaultValue) {
    try {
        if(newValue == null) {
            throw new ArgumentNullException();
        }

        if(propInfo.PropertyType == typeof(string)) {
            newValue = Convert.ToString(newValue);
        } else if(propInfo.PropertyType == typeof(int)) {
            newValue = Convert.ToInt32(newValue);
        }
    } catch {
        newValue = defaultValue;
    }

    propInfo.SetValue(CLASSNAME, newValue, null);
}

E esta função poderá ser invocada das seguintes formas:

string newValue = null;

// O valor de "newValue" é nulo, deve guardar o valor de "propConfig.DefaultValue"
fnCall_1(propInfo, (object)newValue, propConfig.DefaultValue);

// Deve guardar o valor de "newValue"
newValue = "TESTE";
fnCall_1(propInfo, (object)newValue, propConfig.DefaultValue);

// Deve guardar o valor de "newValue"
newValue = "1000";
fnCall_1(propInfo, (object)newValue, propConfig.DefaultValue);

// O "newValue" é suposto ser inteiro, deve guardar o valor de "propConfig.DefaultValue"
newValue = "x1000";
fnCall_1(propInfo, (object)newValue, propConfig.DefaultValue);

// Não existe qualquer valor em "newValue, deve guardar o valor de "propConfig.DefaultValue"
fnCall_1(propInfo, null, propConfig.DefaultValue);

E só para que entendam o contexto do problema... A variável newValue está declarada como string e definida como null inicialmente porque o valor dela será lido de um ficheiro XML e como tal, o ideal é ser uma string. Mas esta variável é temporária que só serve para guardar o valor lido do XML, posteriormente será usada para definir o valor de determinada propriedade. As propriedades podem ser de vários tipos (string ou inteiro neste exemplo), daí o casting feito para object. Tanto defaultValue como propConfig.DefaultValue são do tipo object porque como a variável anterior também podem conter vários tipos de dados e não há volta a dar, propConfig.DefaultValue tem mesmo de ser object. Alterar isso, iria alterar completamente a solução geral de todo o código...

A minha questão é, todos estas variáveis do tipo object, aquelas conversões para inteiros/strings só para verificar se a conversão é feita correctamente (no caso do newValue = "x1000", tem de dar erro porque é suposto ser um inteiro e ao tentar fazer a covnersão para inteiro vai ser lançada uma excepção e como tal deve ser guardado o valor por omissão previamente definido no código), o lançamento manual daquela excepção se o valor de newValue for nulo.

Ou seja, todas estas pequenas coisas, estão bem feitas ou existe algum problema com este código? Vocês fariam de outra forma?

0

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites

Sinceramente, aquele throw dentro do try está a mais. Em vez do try { if { throw } } catch, devias usar um simples if {} else. O que fizeste abusa um pouco do mecanismo de lançamento de excepções. Como o lançamento e tratamento da excepção é local e detectável de outro modo (nomeadamente através do if onde fazes throw), não faz sentido utilizar o try catch, que pesa mais na máquina virtual.

0

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites

A ideia é não ter de repetir a linha que está dentro do catch e como o objectivo do if é fazer exactamente o mesmo que será necessário fazer quando realmente ocorrer uma excepção em algum dos métodos do Convert, porque não tirar partido do catch?

E acho que não percebeste bem o que está a acontecer... Segundo entendi do teu post, estás a dizer para eu eliminar por completo to try catch mas isso não é possível. O try catch não está ali apenas por causa do throw, mas sim porque no caso onde newValue = "x1000" o Convert.ToInt32(newValue) vai lançar uma excepção e se isto acontecer, é porque o ficheiro XML tem "erros" (pode ter sido modificado manualmente por alguém que não sabia o que estava a fazer, por exemplo) e nesta situação, é suposto o valor por omissão ser usado em vez do valor lido.

0

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites

Bem, visto que a tua variável provém de um XML, acho que o mais lógico seria ser do tipo String e não Object. Mas independentemente disso, acho que esses ifs encadeados ficam um pouco mal. E com um switch fica melhor. Algo do género

private void fnCall_1(PropertyInfo propInfo, object newValue, object defaultValue) {
  // Trata o caso, se for null, necessario senao no convert iria retornar o default para o tipo em questão
  newValue = newValue ?? defaultValue;

  // Conforme os tipos, faz o convert, se for o defaultValue penso que não faz nada
  try {
    switch(Type.GetTypeCode(propInfo.PropertyType)) {
      case TypeCode.String: newValue = Convert.ToString(newValue); break;
      case TypeCode.Int32:  newValue = Convert.ToInt32(newValue); break;
    }
  } catch {
    newValue = defaultValue;
  }

  propInfo.SetValue(CLASSNAME, newValue, null);
}

0

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites

Não disse para tirares o try catch, disse apenas para não fazeres throw, mas isto é programação tendo mais em conta performance que outra coisa, é como estou habituado a programar.

A solução do Betovsky é mais elegante, embora a 1ª linha pudesse desaparecer. Porque se for null, vai fazer à mesma o try catch, e supondo que o defaultValue é sempre correcto (ou o catch não faria sentido) das duas uma:

  • ou newValue era null e ficou com defaultValue, que já é do tipo esperado, e vai convertê-lo para o tipo que já é (conversão redundante);
  • ou newValue não era null, caso em que faz sentido todo o bloco try-catch.

Para tirar partido de testar se o newValue é null, fazia if (newValue==null) newValue=defaultValue; e metia o try catch dentro do else deste if.

0

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites

ou newValue era null e ficou com defaultValue, que já é do tipo esperado, e vai convertê-lo para o tipo que já é (conversão redundante);

Pois, dai ter dito, que penso que não faz nada.

Sei que, no caso de ser String, em termos de processamento é indiferente, ou seja, nem faz conversão. Poupa-se mais um bloco de código (o else) ficando o código mais "bonito".

A minha dúvida cai nos outros tipos, neste caso o int. Não sei se fazer a conversão de int para int, também seria ignorada.

0

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites

Sei que, no caso de ser String, em termos de processamento é indiferente, ou seja, nem faz conversão. Poupa-se mais um bloco de código (o else) ficando o código mais "bonito".

Mesmo que não faça a conversão, o switch implica mais uma comparação, mais um salto, mais uma passagem de parâmetros, um call (possivelmente virtual, o que implicará mais algumas transferências de dados da memória para descobrir para onde é o salto final), o respectivo return, a cópia da referência, e novo salto (do break). Tudo isto pode ser evitado com o if-else, no caso de newValue ser null.

0

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites

@TheDark

Não estou a perceber bem onde queres chegar, podes dar um exemplo de código de como farias assim como o Betvosky fez? É que nem estou a perceber se achas bem o switch que ele fez ou não, com um exemplo de código de como farias era mais fácil entender-te...

Para tirar partido de testar se o newValue é null, fazia if (newValue==null) newValue=defaultValue; e metia o try catch dentro do else deste if.

Mas tipo, a ideia era não repetir essa linha duas vezes, uma no if e outra no catch. Com o meu throw porque o valor era nulo, ele ia parar ao catch e fazia a mesma coisa, por isso é que programei dessa forma. Para não ter de repetir a mesma linha de código. Ate pode ter ficado um código mais extenso com o throw, mas o "problema" mesmo era estar a repetir o código, que n me soava bem...

@Betovsky

Eu desde o inicio que queria fazer um switch mas perguntei no #p@p e ninguem me soube dizer como o faria porque os cases não era simples constantes e eu não sabia fazer, mas se da maneira que colocaste funciona, é o que vou fazer.

Para terminar e talvez me possam ajudar a melhorar o código ai, eu realmente não gosto muito daquelas conversões mas não arranjei outra forma de o fazer. Por exemplo, no caso do newValue = "x1000" onde este valor será guardado numa propriedade do tipo int a conversão serve para ver se ele consegue converter para inteiro, se conseguir está tudo bem, se não conseguir é porque o valor não é inteiro e deve ser usado o valor por omissão.

0

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites

@TheDark

Não estou a perceber bem onde queres chegar, podes dar um exemplo de código de como farias assim como o Betvosky fez? É que nem estou a perceber se achas bem o switch que ele fez ou não, com um exemplo de código de como farias era mais fácil entender-te...

Gosto do switch sim senhor. A alteração que propus serve apenas para não correr código desnecessariamente (a parte do try catch quando o newValue é null), que em termos de IL ainda eram umas quantas instruções. Pegando no código do Betovsky, era algo assim:

private void fnCall_1(PropertyInfo propInfo, object newValue, object defaultValue) {
  // se for null, afecta com o valor por omissão, que já é do tipo pretendido
  if (newValue == null) { newValue = defaultValue; }
  // senão, trata de converter para o tipo correcto
  else {
    try {
      switch(Type.GetTypeCode(propInfo.PropertyType)) {
        case TypeCode.String: newValue = Convert.ToString(newValue); break;
        case TypeCode.Int32:  newValue = Convert.ToInt32(newValue); break;
      }
    } catch {
      newValue = defaultValue;
    }
  }
  // neste momento:
  // - ou newValue era null, foi afectado com defaultValue e saltou para aqui,
  //   evitando a execução do código de conversão;
  // - ou newValue tinha um valor válido, e foi convertido;
  // - ou newValue não era null mas continha um valor inválido, e no tratamento da
  //   excepção foi afectado com defaultValue.
  propInfo.SetValue(CLASSNAME, newValue, null);
}

Mas tipo, a ideia era não repetir essa linha duas vezes, uma no if e outra no catch. Com o meu throw porque o valor era nulo, ele ia parar ao catch e fazia a mesma coisa, por isso é que programei dessa forma. Para não ter de repetir a mesma linha de código. Ate pode ter ficado um código mais extenso com o throw, mas o "problema" mesmo era estar a repetir o código, que n me soava bem...

O problema para mim não é o código ser extenso, é mesmo o estar a correr código desnecessário e evitar o tratamento da excepção quando pode ser resolvido sem lançamentos, o que em termos de performance é mais eficiente.

Para terminar e talvez me possam ajudar a melhorar o código ai, eu realmente não gosto muito daquelas conversões mas não arranjei outra forma de o fazer. Por exemplo, no caso do newValue = "x1000" onde este valor será guardado numa propriedade do tipo int a conversão serve para ver se ele consegue converter para inteiro, se conseguir está tudo bem, se não conseguir é porque o valor não é inteiro e deve ser usado o valor por omissão.

Nesse caso acho que fizeste bem, pois as circunstâncias em que o valor é inválido (tipo o "x1000") são mesmo uma excepção, num caso em que algo correu mal.

O que me ensinaram foi que o lançamento e tratamento de excepções é para ser feito em casos excepcionais (como o próprio nome indica :thumbsup: ), e não em código que seja para executar normalmente. Uma excepção não deve ser algo que ocorra habitualmente.

0

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites

Acho que já percebi o que querias dizer.

Quanto ao resto, não veem mais nenhum problema no código? :thumbsup:

Obrigado aos dois desde já.

0

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites

Crie uma conta ou ligue-se para comentar

Só membros podem comentar

Criar nova conta

Registe para ter uma conta na nossa comunidade. É fácil!


Registar nova conta

Entra

Já tem conta? Inicie sessão aqui.


Entrar Agora