Switch statement

Go To Last Post
4 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Here is a fragment os a code i've written, all part of a big state machine. All cases below (except the default one), there are some calculations, conversions to ascii (sprintf) and then I send it to the uart and set CurState to SM_END.

switch (uart_getc())
{
   case CB_RD_SP:
      sprintf(buffer, "#03d", (tPIsat_params.sp * 50 + 68) / 136);
      uart_puts(buffer);

      CurState = SM_END;
      break;

   case CB_RD_KP:
      sprintf(buffer, "#04d", tPIsat_params.w_kp_pu);
      uart_puts(buffer);

      CurState = SM_END;
      break;

   case CB_RD_KI:
      sprintf(buffer, "#04d", tPIsat_params.w_ki_pu);
      uart_puts(buffer);

      CurState = SM_END;
      break;

   case CB_RD_VI:
      sprintf(buffer, "#03d", (VoltInp * 10 + 41) / 82);
      uart_puts(buffer);

      CurState = SM_END;
      break;

   case CB_RD_VO:
      ATOMIC_BLOCK(ATOMIC_FORCEON)
      {
         SP_novo = tPIsat_params.pv;
      }
      sprintf(buffer, "#03d", (SP_novo * 100) / 272);
      uart_puts(buffer);

      CurState = SM_END;
      break;

 
   default:
      CurState = SM_ERROR;
      TipoErro = CB_ERROR_RD;
      break;
} 

Is there a way to groups all the "uart_puts(buffer)" and "CurState = SM_END" in a single one? I've thought of using goto, but it is ugly, at least to me.

Using goto:

switch (uart_getc())
{
   case CB_RD_SP:
      sprintf(buffer, "#03d", (tPIsat_params.sp * 50 + 68) / 136);
      goto RD_END;

   case CB_RD_KP:
      sprintf(buffer, "#04d", tPIsat_params.w_kp_pu);
      goto RD_END;

   case CB_RD_KI:
      sprintf(buffer, "#04d", tPIsat_params.w_ki_pu);
      goto RD_END;

   case CB_RD_VI:
      sprintf(buffer, "#03d", (VoltInp * 10 + 41) / 82);
      goto RD_END;

   case CB_RD_VO:
      ATOMIC_BLOCK(ATOMIC_FORCEON)
      {
         SP_novo = tPIsat_params.pv;
      }
      sprintf(buffer, "#03d", (SP_novo * 100) / 272);
      goto RD_END;			// doesn't need this one

   RD_END:
      uart_puts(buffer);
      CurState = SM_END;
      break;
            
   default:
      CurState = SM_ERROR;
      TipoErro = CB_ERROR_RD;
      break;
}

Felipe Maimon

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Move the common code out of the switch.

e.g.


CurState=SM_END;

switch (uart_getc())
{
   case CB_RD_SP:
     format_string="%03d";
     parameter=(tPIsat_params.sp * 50 + 68) / 136);
     break;

   ..etc..

 
   default:
      CurState = SM_ERROR;
      TipoErro = CB_ERROR_RD;
      break;
}

if (CurState==SMD_END)
 {
 sprintf(buf,format_string,parameter);
 uart_puts(buffer);
 }
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You can give your default: case a special value. e.g. SM_ERROR
then at the end of the switch statement:

switch (uart_getc())
{
   case CB_RD_SP:
      sprintf(buffer, "#03d", (tPIsat_params.sp * 50 + 68) / 136); break;
   case CB_RD_KP:
      sprintf(buffer, "#04d", tPIsat_params.w_kp_pu); break;
   case CB_RD_KI:
      sprintf(buffer, "#04d", tPIsat_params.w_ki_pu); break;
   case CB_RD_VI:
      sprintf(buffer, "#03d", (VoltInp * 10 + 41) / 82); break;
   case CB_RD_VO:
      ATOMIC_BLOCK(ATOMIC_FORCEON)
      {
         SP_novo = tPIsat_params.pv;
      }
      sprintf(buffer, "#03d", (SP_novo * 100) / 272);
      break;         // doesn't need this one
   default:
      CurState = SM_ERROR;
      TipoErro = CB_ERROR_RD;
      break;
}

if (CurState != SM_ERROR) {
      uart_puts(buffer);
      CurState = SM_END;
}

But really what matters is that your code is clear to you. There is nothing inherently wrong with goto. Just think about what the Compiler actually generates: goto's everywhere to link blocks of code!

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you guys. I'm going with David's code as it doesn't need any new variables.

The goto code was clear to me. But it didn't feel right.

Thank you again.

Felipe Maimon