Do I need to explicitly handle negative numbers or zero when summing squared digits?
Summarizing a discussion that's been percolating in the comments:
- There is no good reason to test in advance for
n == 0
. Thewhile(n)
test will handle that case perfectly. - It's likely your teacher is still used to earlier times, when the result of
%
with negative operands was differently defined. On some old systems (including, notably, early Unix on a PDP-11, where Dennis Ritchie originally developed C), the result ofa % b
was always in the range[0 .. b-1]
, meaning that -123 % 10 was 7. On such a system, the test in advance forn < 0
would be necessary.
But the second bullet applies only to earlier times. In the current versions of both the C and C++ standards, integer division is defined to truncate towards 0, so it turns out that n % 10
is guaranteed to give you the (possibly negative) last digit of n
even when n
is negative.
So the answer to the question "What is the meaning of while(n)
?" is "Exactly the same as while(n != 0)
", and the answer to "Will this code work properly for negative as well as positive n
?" is "Yes, under any modern, Standards-conforming compiler." The answer to the question "Then why did the instructor mark it down?" is probably that they're not aware of a significant language redefinition that happened to C in 1999 and to C++ in 2010 or so.
Your code is perfectly fine
You are absolutely correct and your teacher is wrong. There is absolutely no reason at all to add that extra complexity, since it does not affect the result at all. It even introduces a bug. (See below)
First, the separate check if n
is zero is obviously completely unnecessary and this is very easy to realize. To be honest, I actually question your teachers competence if he has objections about this. But I guess everybody can have a brain fart from time to time. However, I DO think that while(n)
should be changed to while(n != 0)
because it adds a little bit extra clarity without even costing an extra line. It's a minor thing though.
The second one is a bit more understandable, but he is still wrong.
This is what the C11 standard 6.5.5.p6 says:
If the quotient a/b is representable, the expression (a/b)*b + a%b shall equal a; otherwise, the behavior of both a/b and a%b is undefined.
The footnote says this:
This is often called "truncation toward zero".
Truncation toward zero means that the absolute value for a/b
is equal to the absolute value for (-a)/b
for all a
and b
, which in turn means that your code is perfectly fine.
Modulo is easy math, but may be counterintuitive
However, your teacher does have a point that you should be careful, because the fact that you're squaring the result is actually crucial here. Calculating a%b
according to above definition is easy math, but it might go against your intuition. For multiplication and division, the result is positive if the operands have equal sign. But when it comes to modulo, the result has the same sign as the first operand. The second operand does not affect the sign at all. For instance, 7%3==1
but (-7)%(-3)==(-1)
.
Here is a snippet demonstrating it:
$ cat > main.c
#include <stdio.h>
void f(int a, int b)
{
printf("a: %2d b: %2d a/b: %2d a\%b: %2d (a%b)^2: %2d (a/b)*b+a%b==a: %5s\n",
a, b ,a/b, a%b, (a%b)*(a%b), (a/b)*b+a%b == a ? "true" : "false");
}
int main(void)
{
int a=7, b=3;
f(a,b);
f(-a,b);
f(a,-b);
f(-a,-b);
}
$ gcc main.c -Wall -Wextra -pedantic -std=c99
$ ./a.out
a: 7 b: 3 a/b: 2 a%b: 1 (a%b)^2: 1 (a/b)*b+a%b==a: true
a: -7 b: 3 a/b: -2 a%b: -1 (a%b)^2: 1 (a/b)*b+a%b==a: true
a: 7 b: -3 a/b: -2 a%b: 1 (a%b)^2: 1 (a/b)*b+a%b==a: true
a: -7 b: -3 a/b: 2 a%b: -1 (a%b)^2: 1 (a/b)*b+a%b==a: true
So, ironically, your teacher proved his point by being wrong.
Your teacher's code is flawed
Yes, it actually is. If the input is INT_MIN
AND the architecture is two's complement AND the bit pattern where the sign bit is 1 and all value bits are 0 is NOT a trap value (using two's complement without trap values is very common) then your teacher's code will yield undefined behavior on the line n = n * (-1)
. Your code is - if ever so slightly - better than his. And considering introducing a small bug by making the code unnecessary complex and gaining absolutely zero value, I'd say that your code is MUCH better.
In other words, in compilations where INT_MIN = -32768 (even though the resulting function cannot receive an input that is < -32768 or > 32767), the valid input of -32768 causes undefined behavior, because the result of -(-32768i16) cannot be expressed as a 16-bit integer. (Actually, -32768 probably would not cause an incorrect result, because -(-32768i16) usually evaluates to -32768i16, and your program handles negative numbers correctly.) (SHRT_MIN could be -32768 or -32767, depending on the compiler.)
But your teacher explicitly stated that n
can be in the range [-10^7; 10^7]. A 16-bit integer is too small; you would have to use [at least] a 32-bit integer. Using int
might seem to make his code safe, except that int
is not necessarily a 32-bit integer. If you compile for a 16-bit architecture, both of your code snippets are flawed. But your code is still much better because this scenario reintroduces the bug with INT_MIN
mentioned above with his version. To avoid this, you can write long
instead of int
, which is a 32-bit integer on either architecture. A long
is guaranteed to be able to hold any value in the range [-2147483647; 2147483647]. C11 Standard 5.2.4.2.1 LONG_MIN
is often -2147483648
but the maximum (yes, maximum, it's a negative number) allowed value for LONG_MIN
is -2147483647
.
What changes would I make to your code?
Your code is fine as it is, so these are not really complaints. It's more like that if I really, really need to say anything about your code, there are some small things that could make it just a tiny bit clearer.
- The names of the variables could be a little bit better, but it is a short function that is easy to understand, so it's not a big deal.
- You could change the condition from
n
ton!=0
. Semantically, it's 100% equivalent, but it makes it a little bit clearer. - Move declaration of
c
(which I renamed todigit
) to inside the while loop since it's only used there. - Change argument type to
long
to ensure it can handle the whole input set.
int sum_of_digits_squared(long n)
{
long sum = 0;
while (n != 0) {
int digit = n % 10;
sum += (digit * digit);
n /= 10;
}
return sum;
}
Actually, this can be a little bit misleading because - as mentioned above - the variable digit
can get a negative value, but a digit is in itself never either positive or negative. There are a few ways around this, but this is REALLY nitpicking, and I would not care for such small details. Especially the separate function for last digit is taking it too far. Ironically, this is one of the things that your teachers code actually solves.
- Change
sum += (digit * digit)
tosum += ((n%10)*(n%10))
and skip the variabledigit
completely. - Change the sign of
digit
if negative. But I would strongly advice against making the code more complex just to make a variable name make sense. That's a VERY strong code smell. - Create a separate function that extracts the last digit.
int last_digit(long n) { int digit=n%10; if (digit>=0) return digit; else return -digit; }
This is useful if you want to use that function somewhere else. - Just name it
c
as you originally do. That variable name does not give any useful information, but on the other hand, it's not misleading either.
But to be honest, at this point you should move on to more important work. :)
I don't completely like either your version or your teacher's. Your teacher's version does the extra tests that you correctly point out are unnecessary. C's mod operator is not a proper mathematical mod: a negative number mod 10 will produce a negative result (proper mathematical modulus is always non-negative). But since you're squaring it anyway, no difference.
But this is far from obvious, so I would add to your code not the checks of your teacher, but a big comment that explains why it works. E.g.:
/* NOTE: This works for negative values, because the modulus gets squared */