gpop1:
it compiles and does what its asked to do but is there a better way
If it works, and it makes sense to you, then it is a correct solution. While I don't see anything intrinsically wrong with your solution, the one thing that stands out to me about it is its lack of self-documentation.
What I mean by that is you have a function, with various parameters that you pass - but only by reading the code can you tell what the function does. I can't simply look at the function definition and tell what it does from the names of the parameters, nor do I see any comments that tell me that, either. Secondly, even if there were comments, and the variables expressed differently - looking at the calls to the function in the code doesn't give me any clue as to how it functions.
Why is this important? Well - maintainability for the most part. Should you hand this code to someone else, or should you come back to it after a period of time has passed - the more readable and understandable you make it, the easier it is for someone else or your future self to understand it.
So - let's try an iterative approach to making it more readable - here's the original function:
int limit(int a, int b, int c, int d, int e) {
if (a <= b) {
a = d;
}
if (a >= c) {
a = e;
}
return (a);
}
So - let's first make it more readable by improving the names of the variables, plus adding some commenting:
/*
* Apply upper and lower limits to a given value
*
* @param value integer value to check
* @param min integer minimum value can be before limiting
* @param max integer maximum value can be before limiting
* @param lower integer value to set for lower limit
* @param upper integer value to set for upper limit
*
* @return int
*/
int limit(int value, int min, int max, int lower, int upper) {
if (value <= min) {
value = lower;
}
if (value >= max) {
value = upper;
}
return (value);
}
As you can see, by adding a few comments, and changing some variable names, it becomes much more readable.
The comments in the comment block before the function declaration are formatted for Doxygen (http://www.stack.nl/~dimitri/doxygen/), which is almost a defacto standard now for documenting code (if you do all of your code like this, you can run a doxygen translation program over the source code and get an instant set of API documentation for what your code does - which is very, very handy).
The variables are all named in a manner to indicate their use - through the combination of both (and, had the function been more complex, inline comments in the code as well), the code become clearer. You may also notice that I added some spacing to the code; used properly (and consistently I might add!) - these three minor updates can do wonders for the future maintenance of a code base.
More can be done, of course. One thing I noticed was that your code set the limits based on checks using "less-than-or-equal" and "greater-than-or-equal". I am not sure this is completely correct; usually, when you say "this is the maximum value" and "this is the minimum value" to be accepted, you are saying that anything under or over that will cause a change - not that the value for the min/max is included in that check. To me, that would mean not including the "equals" part of the check:
/*
* Apply upper and lower limits to a given value
*
* @param value integer value to check
* @param min integer minimum value can be before limiting
* @param max integer maximum value can be before limiting
* @param lower integer value to set for lower limit
* @param upper integer value to set for upper limit
*
* @return int
*/
int limit(int value, int min, int max, int lower, int upper) {
if (value < min) {
value = lower;
}
if (value > max) {
value = upper;
}
return (value);
}
You could also make the code more efficient by returning from the check immediately should one of the checks pass - because if the one check passes, the other should fail - so returning early is better than performing both checks:
/*
* Apply upper and lower limits to a given value
*
* @param value integer value to check
* @param min integer minimum value can be before limiting
* @param max integer maximum value can be before limiting
* @param lower integer value to set for lower limit
* @param upper integer value to set for upper limit
*
* @return int
*/
int limit(int value, int min, int max, int lower, int upper) {
if (value < min) {
return lower;
}
if (value > max) {
return upper;
}
return value;
}
If you didn't need to be able to set a different upper or lower value after a bounds check, you could just use the upper and lower value as your returned value:
/*
* Apply minimum and maximum limits to a given value
*
* @param value integer value to check
* @param min integer minimum to limit value to
* @param max integer maximum to limit value to
*
* @return int
*/
int limit(int value, int min, int max) {
if (value < min) {
return min;
}
if (value > max) {
return max;
}
return value;
}
You could also break out the min/max checks into their own functions (now we are getting into the territory of creating a "bounds checking function library" - aka "minmax"):
/*
* Apply minimum limits to a given value
*
* @param value integer value to check
* @param min integer minimum to limit value to
*
* @return int
*/
int min(int value, int min) {
if (value < min) {
return min;
}
return value;
}
/*
* Apply maximum limits to a given value
*
* @param value integer value to check
* @param max integer maximum to limit value to
*
* @return int
*/
int max(int value, int max) {
if (value > max) {
return max;
}
return value;
}
/*
* Apply minimum and maximum limits to a given value
*
* @param value integer value to check
* @param min integer minimum to limit value to
* @param max integer maximum to limit value to
*
* @return int
*/
int minmax(int value, int min, int max) {
return max(min(value, min), max);
}
Note that this example - while providing a couple of extra functions (if the complete min/max check isn't needed) - does suffer from multiple function calls (with no instant guard-statement return), and is thus a bit less efficient than prior examples. That said, given the simplistic nature of the code, the trade-off is negligible for most use-cases, IMHO.
You could take this further - and turn it into a minmax class; properly doing so would let you set your parameters once in the constructor, then you could simply call the minmax() method with the singular value to check and return; the code would become much cleaner and more readable (at the expense of the added slight complexity of class and object notation - which can be an alien thing at first if you aren't familiar with it; most of the libraries the Arduino uses, though, are object oriented - so it is something you might want to consider if you haven't yet done so).