Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

For one of my university assignments, we were asked to take an unsigned int, reverse its digits, find the absolute difference between the original and reversed, then reverse that, and you receive the "Magic Number".

$$ R(\ \lvert R(A) - A\rvert\ )$$

where \$A\$ is a positive integer and \$R(n)\$ means reverse.

We were also asked to take multiple inputs at once and keep track of the largest absolute difference between all inputs.

#include <stdio.h>

// Reverse an unsigned integer
unsigned int reverseInteger(unsigned int input) {

    unsigned int output = 0;

    while (input != 0) { // Terminate when input is 0
        output = output * 10 + input % 10; // Can't use =* here, order of operations!
        input /= 10;
    }

    return output;
}

// Find the absolute differnece between a reversed number, then reverse the absolute difference
unsigned int generateMagicNumber(unsigned int input) {

    /* Find Absolute Difference */
    if (input < reverseInteger(input)) {
        input = reverseInteger(input); // If the result will be negative, reverse the number before finding the difference
        input -= reverseInteger(input);
    }
    else {
        input -= reverseInteger(input);
    }

    largestAbsoluteDifference(input);

    return reverseInteger(input); // Reverse absolute difference to find the magic number

}

// Find the largest of all values passed to this function
unsigned int largestAbsoluteDifference(unsigned int input) {

    static unsigned int keep = 0; // Must be static!

    return (input > keep) ? (keep = input) : keep; // If the input is greater than the current, store the input. Else, keep the current.

}

int main() {

    int inputs;
    printf("Enter number of inputs: ");
    scanf("%d", &inputs);
    printf("Enter space separated: ");

    for (int i = 0; i < inputs; i++) {
        unsigned int num;
        scanf("%u", &num);

        if (i == 0) { printf("\nOutput: "); } // Append label only on first iteration

        printf("%u ", generateMagicNumber(num)); // Generate and print the magic number
    }

    printf("\nLargest Absolute Difference: %u\n", largestAbsoluteDifference(0)); // Must pass 0 into largestAbsoluteDifference

}

Example Input / Output

Enter number of inputs: 5
Enter space separated: 23 512 454 94 100

Output: 9 792 0 54 99
Largest Absolute Difference: 297

Notes / Concerns

  • The way I wrote the largestAbsoluteDifference seems slimy me to (having to pass 0 to get the result).
  • I've got in the habit of calling functions inside a printf instead of storing the value in a variable and then printing the variable (see lines 56, 59). Is this okay?
  • In generateMagicNumber I had to make sure finding the difference wouldn't result in a negative number (which you can't store in an unsigned int). Is my method the best way to find the absolute value like that?
share|improve this question
3  
Is R intended to be bijective? As implemented reverseInteger(90) yields 9, which when reversed again yields 9. If bijectivity is required, then you might have to operate on strings... – Matthieu M. Jan 28 at 17:32
    
@MatthieuM. No. As it works is how it should. Good thing to point out, though! – Insane Jan 29 at 3:43
up vote 8 down vote accepted

You write unsigned int enough times that it's probably worthwhile to typedef it, in caseunsigned int turns out to be too small. (Keep in mind that it is only guaranteed to go up to 65535.)

typedef unsigned int number;
#define NUMBER_FMT "%u"

The reverseInteger() function is fine. I'd rename it for clarity, then remove the superfluous comments.

/**
 * Reverse the digits of a number
 */
number reverseDigits(number input) {
    number output = 0;

    while (input != 0) {
        output = output * 10 + input % 10;
        input /= 10;
    }

    return output;
}

The code in generateMagicNumber() was really hard to follow. The call reverseInteger(input) appears five times in that function. One codepath has four of those calls, and the other has three. Not only is that suboptimal1, it's also confusing because the input refers to a different number each time even though they all look the same. Please avoid repurposing variables like that.

/**
 * The reverse of the absolute difference between a number and its reverse
 */
number magic(number input) {
    number rev = reverseDigits(input);
    number absDiffWithReverse = (rev > input) ? rev - input : input - rev;
    return reverseDigits(absDiffWithReverse);
}

The ugly part of the problem is the requirement to keep track of the largest \$\lvert R(A) - A \rvert\$. I see four unpalatable options:

  1. As you have done, insert code inside generateMagicNumber() to capture the intermediate computational result.

    The disadvantages are that you are doing this as a side-effect, and using static (or global) state. That kind of statefulness would become a maintenance headache on larger projects. You also had to use a funny hack of calling largestAbsoluteDifference(0) to read out the maximum.

  2. Instead of defining generateMagicNumber(), define absDiffWithReverse() to calculate \$\lvert R(A) - A \rvert\$. Then, let main() be responsible for tracking the maximum of those.

    To get the magic numbers, main() would need to finish the job by reversing them. It's a bit dissatisfying that, for a program whose primary purpose is to compute "magic numbers", there is no magic-number-computing function.

  3. Define a function to compute both the magic number and the \$\lvert R(A) - A \rvert\$ intermediate result.

    Since C only allows one return value, you could either return a struct with two numbers or pass back one of the values as an out-parameter. I find both techniques aesthetically unpleasing, especially since those two results can be trivially derived from each other.

  4. Define a function to compute the magic number. To get the \$\lvert R(A) - A \rvert\$, reverse the magic number again.

    As @MatthieuM has pointed out, reversing the digits twice could change the result, due to the possibility of trailing or leading zeroes.

I think I would prefer option 3:

number magic(number input, number *absDiffWithReverse) {
    number rev = reverseDigits(input);
    *absDiffWithReverse = (rev > input) ? rev - input : input - rev;
    return reverseDigits(*absDiffWithReverse);
}

number max(number a, number b) {
    return a > b ? a : b;
}

int main() {
    int inputs;
    printf("Enter number of inputs: ");
    scanf("%d", &inputs);
    printf("Enter space separated: ");

    number maxDiffRev = 0;
    for (int i = 0; i < inputs; i++) {
        number n, nMagic, absDiffRev;
        scanf(NUMBER_FMT, &n);
        // Print label only on first iteration
        if (i == 0) { printf("\nOutput: "); }

        nMagic = magic(n, *absDiffRev);
        printf(NUMBER_FMT " ", nMagic);
        maxDiffRev = max(maxDiffRev, *absDiffRev);
    }

    printf("\nLargest Absolute Difference: " NUMBER_FMT "\n", maxDiffRev);
}

To address your specific concerns:

  1. Yes, calling largestAbsoluteDifference(0) to read the maximum is slimy. Ideally, static state should be avoided altogether.
  2. Calling functions inside printf() calls is fine if they are pure functions with no side-effects. It's not as good a practice if the function being called has side-effects. I wouldn't worry about it in your case. What I wrote above, with a variable assignment inside a printf() call, is probably far more controversial.
  3. Your technique is arithmetically correct, but not recommended due to the input variable shifting its meaning.

1 Not that performance really matters here. What I'm pointing out is silliness more than a serious efficiency problem.

share|improve this answer

Using static local variables like that is not a good idea for a few reasons. First there is no way to reset it for the next run. It's also not thread safe.

Instead pull that out into the main function:

unsigned int largest = 0;

for (int i = 0; i < inputs; i++) {
    unsigned int num;
    scanf("%u", &num);

    if (i == 0) { printf("\nOutput: "); } // Append label only on first iteration
    unsigned int magicNumber = generateMagicNumber(num)
    printf("%u ", magicNumber ); // Generate and print the magic number

    if(magicNumber > largest) largest = magicNumber;
}

printf("\nLargest Absolute Difference: %u\n", largest); 

For generating the magic number you can call reverseInteger just once and then store the input:

unsigned int generateMagicNumber(unsigned int input) {

    unsigned int reverse = reverseInteger(input);
    /* Find Absolute Difference */
    if (input < reverse) {
        input = reverse - input;
    }
    else {
        input -= reverse;
    }

    return reverseInteger(input); // Reverse absolute difference to find the magic number

}
share|improve this answer
1  
I think you oversimplified it: largest must consider the absolute difference (and not the magic number) - see the answer of @200_success – oliverpool Jan 28 at 12:15
    
Think the correct term is reentrant, not threadsafe, if you add locks to a code that read*/writes to statics or global's it is thread safe, but not reeantrant. *read from constants is always safe of course. – fhtuft Jan 28 at 13:31
    
@fhtuft non-reentrant in a non-recursive context generally means not thread safe – ratchet freak Jan 28 at 13:39

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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