patterncppMinor
Counting from 1 to 3
Viewed 0 times
fromcountingstackoverflow
Problem
Please critique to help me learn:
I want a function that will increment an external number by one, when it goes over than 3, then reset it to 1.
essentially when I repeatedly call this function I want the out to me 1,2,3,1,2,3,1,2,3
int getNextInt(int* number)
{
int value = *number;
(*number)++;
if (*number > 3)*number = 1;
return value;
}I want a function that will increment an external number by one, when it goes over than 3, then reset it to 1.
essentially when I repeatedly call this function I want the out to me 1,2,3,1,2,3,1,2,3
Solution
Style issue: Formatting and braces
Even if the language allows you to leave out the braces after an
Design issue: out parameters Output parameters tend to obscure the information flow in your program. They are not required in any way, you can do everything fine without them. Failing to keep track of all the funny little things a function might modify as a side effect is a constant source of bugs. Many people thus prefer to not use them at all and rather return a compound of all the things the function calculates and distribute them to the data structures after the function has returned. If this becomes hard you may have an overall design issue. (There are still valid reasons for using out-parameters e.g. in performance and memory critical low-level applications.)
Legacy code: Raw pointers Using plain raw pointers is only fun for very small and quick hacks, that will never see any real use. Become friends with the C++11 (and above) smart pointers. Seriously, they will save you a lot of trouble.
Last but not least
Is in your case (continuous counting with clamped upper and lower value) equal to
With max = 3 and min = 1 you get
Using the modulo operator is an elegant approach for such "around the clock" counting.
Edit: The modulo operator is now named correctly. (Credits to @Cody Gray)
Even if the language allows you to leave out the braces after an
if, do not omit them. People tend to expand their code and forget to put them in when required. This creates nasty, hard to find bugs and the compiler will not complain.Design issue: out parameters Output parameters tend to obscure the information flow in your program. They are not required in any way, you can do everything fine without them. Failing to keep track of all the funny little things a function might modify as a side effect is a constant source of bugs. Many people thus prefer to not use them at all and rather return a compound of all the things the function calculates and distribute them to the data structures after the function has returned. If this becomes hard you may have an overall design issue. (There are still valid reasons for using out-parameters e.g. in performance and memory critical low-level applications.)
Legacy code: Raw pointers Using plain raw pointers is only fun for very small and quick hacks, that will never see any real use. Become friends with the C++11 (and above) smart pointers. Seriously, they will save you a lot of trouble.
Last but not least
if(foo > max){
foo = min;
}Is in your case (continuous counting with clamped upper and lower value) equal to
foo = (foo % (max + 1 - min)) + min; // parenthesis for ease of readingWith max = 3 and min = 1 you get
foo = (foo % 3) + 1;Using the modulo operator is an elegant approach for such "around the clock" counting.
Edit: The modulo operator is now named correctly. (Credits to @Cody Gray)
Code Snippets
if(foo > max){
foo = min;
}foo = (foo % (max + 1 - min)) + min; // parenthesis for ease of readingfoo = (foo % 3) + 1;Context
StackExchange Code Review Q#152405, answer score: 5
Revisions (0)
No revisions yet.