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

Mapping enum to function - request / response library encapsulation

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

Problem

I'm writing code for an embedded device - specifically, a camera. The idea is different camera manufacturers can implement the code so it properly works with their camera.

Here, I'm encapsulating the request/response library we're using, because it's complicated to deal with, and we want to use ease of implementation as a selling point.
Some of the requests require a response or parameters. Others, however, act like your TV would: you press a button, that sends a command code, and that triggers an action. No return code needed, no parameters. I'm gonna have a lot of functions like that.

Relevant types provided by the library we're using:

typedef unsigned char bool;
typedef unsigned char uint8_t;


My enum definition...

typedef enum {
    //auth - 1xx
    //move - 2xx
    //snapshot - 3xx
    //stream - 4xx
    //zoom - 5xx
    //infrared - 6xx
    ZOOM_CAPABLE = 500,
    ZOOM_IN = 501,
    ZOOM_OUT = 502,
    ZOOM_STOP = 503,
    INFRARED_CAPABLE = 600,
    INFRARED_ON = 601,
    INFRARED_OFF = 602,
    WRITE_DEMO = 1000,
    READ_DEMO = 1001
} application_requestID;


These values are shared by some javascript client that communicates with my embedded device. I have the authority to change the values and the javascript client. I'm developing a new system, so I don't need to adhere to any previously defined values.

Currently it's ZOOM_IN, ZOOM_OUT, ZOOM_STOP, INFRARED_ON, INFRARED_OFF that are functions that require no input arguments and no output arguments. As you can see in the comments, I got more stuff planned.

The functions look like this:

bool canZoom();//ZOOM_CAPABLE
void startZoomIn();//ZOOM_IN
void startZoomOut();//ZOOM_OUT
void stopZoom();//ZOOM_STOP


And (as a demo), I have implemented them like this:

```
static bool zoomingIn = false;
static bool zoomingOut = false;
static const uint8_t ZOOM_CAP = 200;
static uint8_t currentZoom = 100;

bool canZoom(){
return true;
}
void startZoomIn(){
zoomingIn = true;

Solution

ANSI C

Don't use // comments but /**/.

Design away invalid states

It probably will never happen and is only in your mockup code but your design allows for the state

zoomingIn = true;
zoomingOut = true;


These two are mutually exclusive so you might consider to use a tristate (enum) like:

typedef enum {
    ZOOMING_IN,
    ZOOMING_STOPPED,
    ZOOMING_OUT,
} zooming_state;


And having the state managed in one variable zooming (or something with a better name then).

Switch to the rescue

Why don't you use a switch for the commandmapping?

switch(applicationRequest->queryId) {
case WRITE_DEMO:
    if (!unabto_query_write_uint8(writeBuffer, getDemoValue())){
        return AER_REQ_RSP_TOO_LARGE;
    }
    return AER_REQ_RESPONSE_READY;
case READ_DEMO: {
    uint8_t x;
    if (!unabto_query_read_uint8(readBuffer, &x)){
        return AER_REQ_TOO_SMALL;
    }
    setDemoValue(x);
    return AER_REQ_RESPONSE_READY;
}
case ZOOM_CAPABLE:
    if (!unabto_query_write_uint8(readBuffer, canZoom())){
        return AER_REQ_RSP_TOO_LARGE;
    }
    return AER_REQ_RESPONSE_READY;
case ZOOM_IN:
    startZoomIn();
    return AER_REQ_RESPONSE_READY;
case ZOOM_OUT:
    startZoomOut();
    return AER_REQ_RESPONSE_READY;
case ZOOM_STOP:
    stopZoom();
    return AER_REQ_RESPONSE_READY;
default:
    return AER_REQ_NO_QUERY_ID;
}


That looks much cleaner to me and it can handle the sparse indices as well (and with automatic compileroptimization!).

Code Snippets

zoomingIn = true;
zoomingOut = true;
typedef enum {
    ZOOMING_IN,
    ZOOMING_STOPPED,
    ZOOMING_OUT,
} zooming_state;
switch(applicationRequest->queryId) {
case WRITE_DEMO:
    if (!unabto_query_write_uint8(writeBuffer, getDemoValue())){
        return AER_REQ_RSP_TOO_LARGE;
    }
    return AER_REQ_RESPONSE_READY;
case READ_DEMO: {
    uint8_t x;
    if (!unabto_query_read_uint8(readBuffer, &x)){
        return AER_REQ_TOO_SMALL;
    }
    setDemoValue(x);
    return AER_REQ_RESPONSE_READY;
}
case ZOOM_CAPABLE:
    if (!unabto_query_write_uint8(readBuffer, canZoom())){
        return AER_REQ_RSP_TOO_LARGE;
    }
    return AER_REQ_RESPONSE_READY;
case ZOOM_IN:
    startZoomIn();
    return AER_REQ_RESPONSE_READY;
case ZOOM_OUT:
    startZoomOut();
    return AER_REQ_RESPONSE_READY;
case ZOOM_STOP:
    stopZoom();
    return AER_REQ_RESPONSE_READY;
default:
    return AER_REQ_NO_QUERY_ID;
}

Context

StackExchange Code Review Q#57765, answer score: 7

Revisions (0)

No revisions yet.