HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Translating warning codes as messages to be printed

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
translatingcodesmessageswarningprinted

Problem

I am writing an X application with Xlib in C, and this code is for error handling and error printing.

How to make it simpler and clearer without using a large amount of fprintf?

int XB3DS_Warn_Code (int Warn_Code)
{
  switch(Warn_Code)
  {
    case WARN_Failed_To_Connect_To_Display:
      fprintf(stdout, "[  WARNING  ]: Failed to connect to display.\n");
      break;
    case WARN_Width_Or_Height_Is_Unacceptable:
      fprintf(stdout, "[  WARNING  ]: Width or height is unacceptable.\n");
      break;
    case WARN_No_Visual_Is_Matched_For_Screen:
      fprintf(stdout, "[  WARNING  ]: No visual is matched for screen.\n");
      break;
    case WARN_The_Window_Mode_Is_Unacceptable:
      fprintf(stdout, "[  WARNING  ]: The window mode is unacceptable.\n");
      break;
    case WARN_Insufficient_Memory_To_Allocate:
      fprintf(stdout, "[  WARNING  ]: Insufficient memory to allocate.\n");
      break;
  }
  return Warn_Code;
}

Solution

Store the variable part of the message in a variable, and do the printing after the switch, for example:

int XB3DS_Warn_Code (int Warn_Code)
{
  const char * msg;
  switch(Warn_Code)
  {
    case WARN_Failed_To_Connect_To_Display:
      msg = "Failed to connect to display.";
      break;
    case WARN_Width_Or_Height_Is_Unacceptable:
      msg = "Width or height is unacceptable.";
      break;
    case WARN_No_Visual_Is_Matched_For_Screen:
      msg = "No visual is matched for screen.";
      break;
    case WARN_The_Window_Mode_Is_Unacceptable:
      msg = "The window mode is unacceptable.";
      break;
    case WARN_Insufficient_Memory_To_Allocate:
      msg = "Insufficient memory to allocate.";
      break;
    default:
      msg = "Unknown error.";
  }
  fprintf(stdout, "[  WARNING  ]: %s\n", msg);
  return Warn_Code;
}


Now the case statements only have the absolutely necessary code. It's the most basic kind of refactoring: extracting values to variables and filling them in a template later. Another variant would be extracting common logic to a helper function. I also added a default case that was missing: it's always good to have.

Also, instead of fprintf(stdout, ...) why not use simply printf(...) ?

Finally, as @black pointed out, "What's the point to return what you get passed?" Better change the method signature to return void. Even better, return the message instead of printing, so that the rest of your code can do whatever it needs with it, for example:

const char * XB3DS_Get_Warn_Message (int Warn_Code) {
    const char * msg;
    // the switch ...
    return msg;
}

void XB3DS_Handle_Warning (int Warn_Code) {
    const char * msg = XB3DS_Get_Warn_Message(Warn_Code);
    fprintf(stdout, "[  WARNING  ]: %s\n", msg);
}


Change the rest of your code to call XB3DS_Handle_Warning instead of your current XB3DS_Warn_Code. Right now you just want to print to stdout. But maybe someday you will want to log to a file, or show as part of a GUI. Then you won't need to touch XB3DS_Get_Warn_Message anymore, but change the simpler XB3DS_Handle_Warning helper.

Code Snippets

int XB3DS_Warn_Code (int Warn_Code)
{
  const char * msg;
  switch(Warn_Code)
  {
    case WARN_Failed_To_Connect_To_Display:
      msg = "Failed to connect to display.";
      break;
    case WARN_Width_Or_Height_Is_Unacceptable:
      msg = "Width or height is unacceptable.";
      break;
    case WARN_No_Visual_Is_Matched_For_Screen:
      msg = "No visual is matched for screen.";
      break;
    case WARN_The_Window_Mode_Is_Unacceptable:
      msg = "The window mode is unacceptable.";
      break;
    case WARN_Insufficient_Memory_To_Allocate:
      msg = "Insufficient memory to allocate.";
      break;
    default:
      msg = "Unknown error.";
  }
  fprintf(stdout, "[  WARNING  ]: %s\n", msg);
  return Warn_Code;
}
const char * XB3DS_Get_Warn_Message (int Warn_Code) {
    const char * msg;
    // the switch ...
    return msg;
}

void XB3DS_Handle_Warning (int Warn_Code) {
    const char * msg = XB3DS_Get_Warn_Message(Warn_Code);
    fprintf(stdout, "[  WARNING  ]: %s\n", msg);
}

Context

StackExchange Code Review Q#62631, answer score: 4

Revisions (0)

No revisions yet.