Simplify and reduce the amount of code without using macros
Sometimes there are situations when the repetition of simple code blocks is inevitable. To illustrate with this example code:
Note: This code is for illustration purposes only, the actual code is much larger and more complex. It may also contain errors, but this is not the point of this question.
switch(cmd) {
case CMD_BLOCK_READ:
if(current_user != key) {
ERROR("Access violation - invalid key!");
res = CR_ACCESS_DENIED;
break;
}
if(current_state < STATE_BUSY) {
WARN("Command %s is not allowed in this state!", cmd_name[cmd]);
res = CR_NOT_PERMITTED;
break;
}
if(ioctl(fd, HPI_CTL_BR) != 0) {
WARN("Handshake failed (%s). Aborted!", strerror(errno));
res = CR_TIME_OUT;
goto post_resp;
}
if(block_read(id) != 0) {
ERROR("Failed to read %d block (%s)! Aborted!", id, strerror(errno));
res = CR_FAIL;
goto send_nop;
}
res = CR_SUCCESS;
break;
case CMD_BLOCK_WRITE:
if(current_user != key) {
ERROR("Access violation - invalid key!");
res = CR_ACCESS_DENIED;
break;
}
if(current_state < STATE_BUSY) {
WARN("Command %s is not allowed in this state!", cmd_name[cmd]);
res = CR_NOT_PERMITTED;
break;
}
if(ioctl(fd, HPI_CTL_BR) != 0) {
WARN("Handshake failed (%s). Aborted!", strerror(errno));
res = CR_TIME_OUT;
goto post_resp;
}
if(block_write(id) != 0) {
ERROR("Failed to write %d block - %s. Command aborted!", id, strerror(errno));
res = CR_FAIL;
goto send_nop;
}
res = CR_SUCCESS;
break;
case CMD_REQ_START:
if(current_state < STATE_READY) {
WARN("Command %s is not allowed in this state!", cmd_name[cmd]);
res = CR_NOT_PERMITTED;
break;
}
state = STATE_BUSY;
if(ioctl(fd, HPI_CTL_BR) != 0) {
WARN("Handshake failed (%s). Aborted!", strerror(errno));
res = CR_TIME_OUT;
goto send_nop;
}
if(block_read(id) != 0) {
ERROR("Failed to read %d block (%s)! Aborted!", id, strerror(errno));
res = CR_FAIL;
goto post_resp;
}
res = CR_SUCCESS;
break;
}
/* The remaining 28 or so similar commands */
}
As you can see, due to minor differences and widespread use of break / goto statements, it is not possible to use functions or inline strings. I usually define some macros:
/* NOTE: DO NOT USE these macros outside of Big Switch */
#define CHECK_KEY(_key) \
if(current_user != (_key)) \
{ \
ERROR("Access violation!"); \
res = CR_ACCESS_DENIED; \
break; \
}
#define CHECK_STATE(_state) \
if(current_state < _state) \
{ \
WARN("Command %s is not allowed in this state!", cmd_name[cmd]); \
res = CR_NOT_PERMITTED; \
break; \
}
#define HANDSHAKE(_fail) \
if(ioctl(fd, CTL_BR) != 0) \
{ \
WARN("Handshake failed (%s). Aborted!", strerror(errno)); \
res = CR_TIME_OUT; \
goto _fail; \
}
#define BLOCK_READ(_id, _fail) \
if(block_read((int)(_id))!= 0) \
{ \
ERROR("Failed to read %d block (%s)! Aborted!", (int)_id, strerror(errno)); \
res = CR_FAIL; \
goto _fail; \
}
#define BLOCK_WRITE(_id, _fail) \
if(block_write((int)(_id)) != 0) \
{ \
ERROR("Failed to write %d block - %s. Aborted!", (int)_id, strerror(errno)); \
res = CR_FAIL; \
goto _fail; \
}
.. and write the same code using them. The code becomes much smaller and (possibly) more readable:
switch(cmd)
{
case CMD_BLOCK_READ:
CHECK_KEY(key);
CHECK_STATE(STATE_BUSY);
HANDSHAKE(post_resp);
BLOCK_READ(id, send_nop);
res = CR_SUCCESS;
break;
case CMD_BLOCK_WRITE:
CHECK_KEY(key);
CHECK_STATE(STATE_BUSY);
HANDSHAKE(post_resp);
BLOCK_WRITE(id, send_nop);
res = CR_SUCCESS;
break;
case CMD_REQ_START:
{
CHECK_STATE(STATE_READY);
state = STATE_BUSY;
HANDSHAKE(send_nop);
BLOCK_READ(id, post_resp);
res = CR_SUCCESS;
break;
}
/* The remaining 28 or so similar commands */
<..>
The code looks more like some kind of scripting language than good old C, and it's really ugly, but I'm willing to sacrifice that for readability.
The question is : how do you deal with situations like this? What are more elegant solutions and best practices?
PS I admit that in general macros and goto statements are signs of bad design, so there is no need to cry about how evil they are or how bad my programming style is.
source to share
I will not argue that the Python source code is an example of organization, but it contains (IMHO) a good example of macros used to simplify a complex piece of code.
The main Python loop implements a bytecode based virtual machine. It contains a huge switching case with one package for each Python opcode. The dispatch for the opcode looks like this:
case STORE_ATTR:
w = GETITEM(names, oparg);
v = TOP();
u = SECOND();
STACKADJ(-2);
err = PyObject_SetAttr(v, w, u); /* v.w = u */
Py_DECREF(v);
Py_DECREF(u);
if (err == 0) continue;
break;
where TOP
, SECOND
and STACKADJ
all are defined as macros working on the stack object. Some macros have an alternative #define
used to support debugging. All opcodes are written this way, and it helps to make the implementation of each opcode much clearer by expressing logic in this miniature scripting language.
In my opinion, careful, reasonable and limited use of macros can improve the readability of the code and make the logic clearer. In your case, where macros hide some small but non-trivial functionality, it can be useful to have macros to standardize the implementation and ensure you don't have multiple copies of the same code snippets to update.
source to share
In situations like this, I usually consider whether cases can be reasonably described with data that is then processed in one common block of code. Of course, this cannot always be done, but often it is possible.
In your case, this could lead to the following:
#define IO_NOOP 0
#define IO_READ 1
#define IO_WRITE 2
struct cmd_desc {
int check_key; /* non-zero to do a check */
int check_state;
int new_state;
void* handshake_fail;
int io_dir;
void* io_fail;
};
const struct cmd_desc cmd_desc_list[] = {
{ 1, STATE_BUSY, -1, &&post_resp, IO_READ, &&send_nop }, /* CMD_BLOCK_READ */
{ 1, STATE_BUSY, -1, &&post_resp, IO_WRITE, &&send_nop }, /* CMD_BLOCK_WRITE */
{ 0, STATE_READY, STATE_BUSY, &&send_nop, IO_READ, &&post_rep } /* CMD_REQ_START */
};
const struct cmd_desc* cmd_desc = cmds[cmd];
if(cmd_desc->check_key) {
if(current_user != key) {
ERROR("Access violation - invalid key!");
return CR_ACCESS_DENIED;
}
}
if(cmd_desc->check_state != -1) {
if(current_state check_state) {
WARN("Command %s is not allowed in this state!", cmd_name[cmd]);
return CR_NOT_PERMITTED;
}
}
if(cmd_desc->new_state != -1)
state = cmd_desc->new_state;
switch(cmd_desc->io_dir) {
case IO_READ:
if(block_read(id) != 0) {
ERROR("Failed to read %d block (%s)! Aborted!", id, strerror(errno));
res = CR_FAIL;
goto *cmd_desc->io_fail;
}
break;
case IO_WRITE:
if(block_write(id) != 0) {
ERROR("Failed to write %d block (%s)! Aborted!", id, strerror(errno));
res = CR_FAIL;
goto *cmd_desc->io_fail;
}
break;
case IO_NOOP:
break;
}
res = CR_SUCCESS;
Remarks I've used the gcc extension "Labels as Values" for goto labels ( http://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html ). In the C standard, you can use function pointers, but that will require some code refactoring and I don't have enough information to do this.
source to share
With the code you posted, there is no reason why you couldn't use the functions. This will be the Extract Function refactoring template. To handle gotos, just leave them in your main function and call them or not based on the result of the function.
http://www.refactoring.com/catalog/extractMethod.html
Also, you really messed things up by using variables in macros that weren't passed. This means that you cannot use them easily, and they are arguably worse than writing it all for a long time. If you have passed everything used by the macro, then this is more useful. Then you end up with a duck input style encoding that can be used effectively.
Also, you are using C, so you should not "avoid" macros. They are incredibly useful primarily for code generation. (ie structure and concatenation) Many C ++ and some others say that "macros are evil". This is C, macros are not evil.
source to share