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 to n!=0. Semantically, it's 100% equivalent, but it makes it a little bit clearer.
- Move declaration of
c (which I renamed to digit) 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) to sum += ((n%10)*(n%10)) and skip the variable digit 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. :)
