• Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 576
  • Last Modified:

refactoring code

I have a piece of code that looks like this:


    case NGX_MAIL_PARSE_ARGS_COMMAND:
        s->state = 0;
        s->out.len = sizeof(imap_invalid_args_command) - 1;
        s->out.data = imap_invalid_args_command;
        s->mail_state = ngx_imap_start;
        break;

    case NGX_MAIL_PARSE_INVALID_COMMAND:
        s->state = 0;
        s->out.len = sizeof(imap_invalid_command) - 1;
        s->out.data = imap_invalid_command;
        s->mail_state = ngx_imap_start;
        break;
    }

Both the case structures do the exact same thing, ie they compute the size for s->out.len and assign the data to sout.data...

the only thing that changes is the message...in teh first case it is (imap_invalid_args_command and in teh second case it is  imap_invalid_command ...

how can i better write this code so as to avoid code duplication?

should i put the code in a function?
should i write a macro?
can i do something like

case 1:
case2:
same code



0
Vlearns
Asked:
Vlearns
  • 7
  • 3
  • 3
  • +1
1 Solution
 
Infinity08Commented:
How are imap_invalid_args_command and imap_invalid_command defined ?
Are all the case's in the switch like this, or are there case's that are completely different ?
0
 
VlearnsAuthor Commented:
heres how these are defined, ma[_invalid_command is defined in a similar way..
static u_char  imap_invalid_args_command[] = "BAD [CLIENTBUG] Invalid or missing arguments" CRLF;


here is teh code

http://lxr.evanmiller.org/http/source/mail/ngx_mail_imap_handler.c
see line 246, basically i am adding more error mesages
0
 
evilrixSenior Software Engineer (Avast)Commented:
Assuming the code for all cases is identical with the only difference being the name of the message you could use a simple macro.

Something like below...
#define HANDLE_CASE(imap_invalid) \
    s->state = 0; \
    s->out.len = sizeof(imap_invalid) - 1; \
    s->out.data = imap_invalid; \
    s->mail_state = ngx_imap_start; \

case NGX_MAIL_PARSE_ARGS_COMMAND:
        HANDLE_CASE(imap_invalid_args_command);
        break;

case NGX_MAIL_PARSE_INVALID_COMMAND:
        HANDLE_CASE(imap_invalid_command);
        break;

Open in new window

0
Cloud Class® Course: Ruby Fundamentals

This course will introduce you to Ruby, as well as teach you about classes, methods, variables, data structures, loops, enumerable methods, and finishing touches.

 
ambienceCommented:
You probably wouldnt save much by refactoring this little piece of code, but I believe this can be done like
    case NGX_MAIL_PARSE_ARGS_COMMAND:
    case NGX_MAIL_PARSE_INVALID_COMMAND:
        s->state = 0;
if(switch express == NGX_MAIL_PARSE_ARGS_COMMAND) {
        s->out.len = sizeof(imap_invalid_args_command) - 1;
        s->out.data = imap_invalid_args_command;
} else {
        s->out.len = sizeof(imap_invalid_command) - 1;
        s->out.data = imap_invalid_command;
}
        s->mail_state = ngx_imap_start;
        break;
    }

 But this may look syntacticaly less redundant, but the added if statement would actualy had better be avoided. A branch is costly operation compared to duplication of two benign assigment statements.
0
 
Infinity08Commented:
For just two case's, I wouldn't bother with trying to collapse them into one case. The changes required to make that work are generally relatively intrusive (which brings its own risk of introducing bugs).

If you really want to, here are some ways (off the top of my head) :

(a) use a macro that sets all the members of s, with one argument - the type of error message (imap_invalid_args_command, imap_invalid_command)

(b) have a static arrays of structs, using the error code (NGX_MAIL_PARSE_ARGS_COMMAND, NGX_MAIL_PARSE_INVALID_COMMAND) as index. Each struct could then contain the error message (u_char  imap_invalid_args_command[], u_char  imap_invalid_command[]). You don't need the switch then - you can use the error code as the index into the array, and get the right error message from it.
0
 
VlearnsAuthor Commented:
i want to structure this so that i can add more error messages...not necessarily just tw, and my primary concern is performance in terms of speed and scalability...adding a function has its overhead, so does a conditional statement....are macros really faster? what are your thoughts?

0
 
evilrixSenior Software Engineer (Avast)Commented:
>> are macros really faster? what are your thoughts?

Macros are expanded at compile time... so the question "are they really faster" is not really something that can be answered directly. It really depends what they expand too.
0
 
VlearnsAuthor Commented:
so its the same as having the code in 2 places?, only that its more readable?


>> are macros really faster? what are your thoughts?

Macros are expanded at compile time... so the question "are they really faster" is not really something that can be answered directly. It really depends what they expand too.
0
 
evilrixSenior Software Engineer (Avast)Commented:
>> so its the same as having the code in 2 places?, only that its more readable?

Exactly.

Macro's are nothing more than text substitution that is performed by the preprocessor *before* the compiler kicks in. Think of it as an elaborate search and replace before compilation.
0
 
VlearnsAuthor Commented:
(b) have a static arrays of structs, using the error code (NGX_MAIL_PARSE_ARGS_COMMAND, NGX_MAIL_PARSE_INVALID_COMMAND) as index. Each struct could then contain the error message (u_char  imap_invalid_args_command[], u_char  imap_invalid_command[]). You don't need the switch then - you can use the error code as the index into the array, and get the right error message from it.


could you please help elaborate this with some sample code?

thanks!
0
 
VlearnsAuthor Commented:
this is how the header files define the constants
#define NGX_MAIL_PARSE_INVALID_COMMAND  20
#define NGX_MAIL_PARSE_ARGS_COMMAND  21
0
 
VlearnsAuthor Commented:
i was thinking on the lines of

static u_char  imap_error_codes[] = {
   "BAD invalid command" CRLF,
   "BAD [CLIENTBUG] Invalid or missing arguments" CRLF,
};


when i  do imap_error_codesNGX_MAIL_PARSE_INVALID_COMMAND] , i want to return   "BAD invalid command" CRLF,

and when i do

imap_error_codesNGX_MAIL_PARSE_ARGS_COMMAND] i want to return  "BAD [CLIENTBUG] Invalid or missing arguments" CRLF,

how do i do it?

previously

this is how the header files define the constants
#define NGX_MAIL_PARSE_INVALID_COMMAND  20
#define NGX_MAIL_PARSE_ARGS_COMMAND  21
0
 
VlearnsAuthor Commented:
any updates?
0
 
Infinity08Commented:
>> could you please help elaborate this with some sample code?

Sure - something like below is what I had in mind. There are downsides and upsides to this technique, so you'll have to weigh them against what you're looking for.

Note that, because you're working with existing code, the code below is slightly more complex than it should need to be (the existing code is the reason for the need for the len field in the struct, and the need for NGX_MAIL_ERROR_BASE_INDEX).
typedef struct ErrorMsg {
    const u_char* msg;
    size_t len;
} ErrorMsg;

static ErrorMsg imap_error_msgs[] = {
    { "BAD [CLIENTBUG] Invalid or missing arguments\r\n", sizeof("BAD [CLIENTBUG] Invalid or missing arguments\r\n") - 1 },
    { "BAD invalid command\r\n", sizeof("BAD invalid command\r\n") - 1 }
};

#define NGX_MAIL_ERROR_BASE_INDEX 20
#define NGX_MAIL_PARSE_INVALID_COMMAND  NGX_MAIL_ERROR_BASE_INDEX + 0
#define NGX_MAIL_PARSE_ARGS_COMMAND     NGX_MAIL_ERROR_BASE_INDEX + 1


/* And then, when you need an error message */

s->state = 0;
s->out.len = imap_error_msgs[rc - NGX_MAIL_ERROR_BASE_INDEX].len;
s->out.data = imap_error_msgs[rc - NGX_MAIL_ERROR_BASE_INDEX].msg;
s->mail_state = ngx_imap_start;

Open in new window

0
Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.

Join & Write a Comment

Featured Post

Get your problem seen by more experts

Be seen. Boost your question’s priority for more expert views and faster solutions

  • 7
  • 3
  • 3
  • +1
Tackle projects and never again get stuck behind a technical roadblock.
Join Now