Ir para o conteúdo
Nazgulled

Algum problema em ter o meu código assim?

Mensagens Recomendadas

Nazgulled    8
Nazgulled

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?

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites
TheDark    0
TheDark

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.

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites
Nazgulled    8
Nazgulled

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.

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites
Betovsky    2
Betovsky

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

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites
TheDark    0
TheDark

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.

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites
Betovsky    2
Betovsky

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.

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites
TheDark    0
TheDark

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.

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites
Nazgulled    8
Nazgulled

@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.

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites
TheDark    0
TheDark

@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.

Partilhar esta mensagem


Link para a mensagem
Partilhar noutros sites
Nazgulled    8
Nazgulled

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á.

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


×

Aviso Sobre Cookies

Ao usar este site você aceita os nossos Termos de Uso e Política de Privacidade