Solved

refactoring code

Posted on 2010-08-16
14
555 Views
Last Modified: 2013-12-06
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
Comment
Question by:Vlearns
  • 7
  • 3
  • 3
  • +1
14 Comments
 
LVL 53

Expert Comment

by:Infinity08
ID: 33449010
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
 

Author Comment

by:Vlearns
ID: 33449111
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
 
LVL 40

Expert Comment

by:evilrix
ID: 33449178
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
 
LVL 22

Expert Comment

by:ambience
ID: 33449221
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
 
LVL 53

Expert Comment

by:Infinity08
ID: 33449235
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
 

Author Comment

by:Vlearns
ID: 33449989
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
 
LVL 40

Expert Comment

by:evilrix
ID: 33450021
>> 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
IT, Stop Being Called Into Every Meeting

Highfive is so simple that setting up every meeting room takes just minutes and every employee will be able to start or join a call from any room with ease. Never be called into a meeting just to get it started again. This is how video conferencing should work!

 

Author Comment

by:Vlearns
ID: 33450060
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
 
LVL 40

Expert Comment

by:evilrix
ID: 33450115
>> 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
 

Author Comment

by:Vlearns
ID: 33450213
(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
 

Author Comment

by:Vlearns
ID: 33450335
this is how the header files define the constants
#define NGX_MAIL_PARSE_INVALID_COMMAND  20
#define NGX_MAIL_PARSE_ARGS_COMMAND  21
0
 

Author Comment

by:Vlearns
ID: 33450434
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
 

Author Comment

by:Vlearns
ID: 33451000
any updates?
0
 
LVL 53

Accepted Solution

by:
Infinity08 earned 500 total points
ID: 33451977
>> 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

Featured Post

Threat Intelligence Starter Resources

Integrating threat intelligence can be challenging, and not all companies are ready. These resources can help you build awareness and prepare for defense.

Join & Write a Comment

Many modern programming languages support the concept of a property -- a class member that combines characteristics of both a data member and a method.  These are sometimes called "smart fields" because you can add logic that is applied automaticall…
Why Shell Scripting? Shell scripting is a powerful method of accessing UNIX systems and it is very flexible. Shell scripts are required when we want to execute a sequence of commands in Unix flavored operating systems. “Shell” is the command line i…
Video by: Grant
The goal of this video is to provide viewers with basic examples to understand and use nested-loops in the C programming language.
The viewer will learn additional member functions of the vector class. Specifically, the capacity and swap member functions will be introduced.

746 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

13 Experts available now in Live!

Get 1:1 Help Now