3
\$\begingroup\$

Please critique to help me learn:

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

\$\endgroup\$
  • \$\begingroup\$ "essentially when I repeatedly call this function I want the out to me 1,2,3,1,2,3,1,2,3" That function will only have that output if you start by passing in 0. If you pass 1, it will return 2. Is that what you intend? \$\endgroup\$ – Nicol Bolas Jan 12 '17 at 5:24
  • \$\begingroup\$ yes, it doesn't matter where in the pattern it starts, just as long as the pattern is maintained. \$\endgroup\$ – user1919249 Jan 12 '17 at 5:25
  • \$\begingroup\$ Are you interested in the 'external variable' value, or in the returned value only? I mean: does it matter what values are stored in *number (provided the return values follow the pattern)? \$\endgroup\$ – CiaPan Jan 12 '17 at 7:31
  • \$\begingroup\$ The external value just holds where the program is at in the sequence. That's it's only purpose. \$\endgroup\$ – user1919249 Jan 12 '17 at 7:53
  • \$\begingroup\$ @user1919249 Please add some context how the external variable is declared and how that function is called. \$\endgroup\$ – πάντα ῥεῖ Jan 12 '17 at 8:30
5
\$\begingroup\$

Style issue: Formatting and braces 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 reading

With 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)

\$\endgroup\$
  • 1
    \$\begingroup\$ Smart pointers existed in C++03, too. They're not as good, but they were there, and you should still use them in lieu of raw pointers. I mention this because your answer seemed to imply that you need C++11 to use smart pointers, and that isn't true. (Also, nitpick: it called the modulo operator, not "module".) \$\endgroup\$ – Cody Gray Jan 12 '17 at 10:48
1
\$\begingroup\$

Naming is hard.

What would you expect to be the output of the following code snippet?

int c = 1;
printf("%d\n", getNextInt(&c));

I, for one, find it unintuitive that the output is 1 rather than 2. The "post-increment" behaviour conflicts with the "next" behaviour implied by the function name. Furthermore, I wouldn't include "get" in the function name: "get" implies that you are retrieving something that already exists, and that you are doing it without any side effects. Neither of those implications is true.

My suggestion would be

int cycle123(int *counter) {
    …
}
\$\endgroup\$
  • \$\begingroup\$ Everything you mentioned made A LOT sense. You are totally right. Thank you for your input. \$\endgroup\$ – user1919249 Jan 18 '17 at 2:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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