current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite

I have to read a int8_t from a buffer, read a uint8_t from the camera's current settings, add them together, keep new value within the bounds 0 to 100 and write the new value to the camera's current settings. This seems quite easy to me, and yet here I have a rather large bit of code.

The brace style is company-mandated.

Is there a way to remove some of the complexity from this code?

Ranges:
plusZoomPercentage: full range of int8_t.
currentZoomPercentage and getZoomPercentage(): 0 to 100, 0 and 100 included.
newZoomPercentage: To be between 0 and 100, 0 and 100 included.

Header file:

/*Zooms to the provided percentage of maximum zoom level.*/
void zoomToPercentage(uint8_t percentage);

/*Retrieves the current targeted percentage of maximum zoom level (if the camera is currently at 50% zoom, but is zooming towards 60%, return 60.)*/
uint8_t getZoomPercentage();

Calling implementation:

int8_t plusZoomPercentage;
if (!unabto_query_read_int8(readBuffer, &plusZoomPercentage))
{
    return AER_REQ_TOO_SMALL;
}
if (plusZoomPercentage == 0) 
{ 
    return AER_REQ_RESPONSE_READY;
}
uint8_t currentZoomPercentage = getZoomPercentage();
uint8_t newZoomPercentage = currentZoomPercentage + plusZoomPercentage;
if (plusZoomPercentage > 0)
{
    /*Check for overflow or otherwise going out of bounds*/
    if (newZoomPercentage > 100 || newZoomPercentage < currentZoomPercentage)
    {
        zoomToPercentage(100);
    }
    else 
    {
        zoomToPercentage(newZoomPercentage);
    }
}
else
{
    if (newZoomPercentage > currentZoomPercentage)
    {
        zoomToPercentage(0);
    }
    else
    {
        zoomToPercentage(newZoomPercentage);
    }
}
return AER_REQ_RESPONSE_READY;
share|improve this question
    
do the input values have any bounds? Like the camera setting and buffer never getting above 100 themselves? –  ratchet freak Mar 6 at 10:48
    
@ratchetfreak Moved the ranges and other related info to the question so people don't have to dig in the comments. –  Pimgd Mar 6 at 10:59

3 Answers 3

up vote 6 down vote accepted

It will be a bit shorter and simpler without the duplicated else blocks:

if (plusZoomPercentage > 0)
{
    /*Check for overflow or otherwise going out of bounds*/
    if (newZoomPercentage > 100 || newZoomPercentage < currentZoomPercentage)
    {
        newZoomPercentage = 100;
    }
}
else if (newZoomPercentage > currentZoomPercentage)
{
    newZoomPercentage = 0;
}
zoomToPercentage(newZoomPercentage);
return AER_REQ_RESPONSE_READY;
share|improve this answer
1  
Even after looking at it several times I have nothing else to say than "yep, that's true. That'll work." –  Pimgd Mar 6 at 11:16
up vote 2 down vote

Sometimes, instead of bounding the result, you should bound the difference. Min and Max are your friend.... Consider the following:

if (plusZoomPercentage != 0)
{
    uint8_t currentZoomPercentage = getCurrentZoom();
    // cannot zoom to less than 0
    plusZoomPercentage = max(plusZoomPercentage, - currentZoomPercentage);
    // cannot zoom to more than 100
    plusZoomPercentage = min(plusZoomPercentage, 100 - currentZoomPercentage);

    zoomToPercentage(currentZoomPercentage + plusZoomPercentage);
}
return AER_REQ_RESPONSE_READY;

Since your current zoom is consistently bound to 0 and 100, you can rely on bounding the difference only.

See this running in ideone.

share|improve this answer
    
Where are max and min coming from? –  Pimgd Mar 6 at 11:59
    
I see, you've defined them yourself... –  Pimgd Mar 6 at 12:05
up vote 1 down vote

By changing the types of currentZoomPercentage and newZoomPercentage to int16_t, you don't have to worry about certain types of overflows:

int16_t newZoomPercentage = getZoomPercentage() + plusZoomPercentage;

if (newZoomPercentage > 100)
    newZoomPercentage = 100;
else if (newZoomPercentage < 0)
    newZoomPercentage = 0;
zoomToPercentage(newZoomPercentage);
share|improve this answer
    
This has to work on an embedded device, where I'd like to keep my datatypes small. Use of an int16_t for newZoomPercentage could make the code smaller, indeed. –  Pimgd Mar 7 at 20:12
1  
Ok I see. What size registers do you have on your cpu? You should be able to use the same size int as your register size with no additional wasted code size or storage. –  JS1 Mar 7 at 20:16

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.