?
Solved

refactoring code

Posted on 2010-08-16
14
Medium Priority
?
562 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
[X]
Welcome to Experts Exchange

Add your voice to the tech community where 5M+ people just like you are talking about what matters.

  • Help others & share knowledge
  • Earn cash & points
  • Learn & ask questions
  • 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
Get 15 Days FREE Full-Featured Trial

Benefit from a mission critical IT monitoring with Monitis Premium or get it FREE for your entry level monitoring needs.
-Over 200,000 users
-More than 300,000 websites monitored
-Used in 197 countries
-Recommended by 98% of users

 
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
 

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 2000 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

Independent Software Vendors: We Want Your Opinion

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Introduction This article is a continuation of the C/C++ Visual Studio Express debugger series. Part 1 provided a quick start guide in using the debugger. Part 2 focused on additional topics in breakpoints. As your assignments become a little more …
This is a short and sweet, but (hopefully) to the point article. There seems to be some fundamental misunderstanding about the function prototype for the "main" function in C and C++, more specifically what type this function should return. I see so…
The goal of the video will be to teach the user the difference and consequence of passing data by value vs passing data by reference in C++. An example of passing data by value as well as an example of passing data by reference will be be given. Bot…
The viewer will learn how to pass data into a function in C++. This is one step further in using functions. Instead of only printing text onto the console, the function will be able to perform calculations with argumentents given by the user.
Suggested Courses
Course of the Month11 days, 11 hours left to enroll

752 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