Armstrong number program in C returns wrong value

Question

I am writing a program to see if a user entered number is Armstrong or not, here is my code:

#include <stdio.h>
#include <stdlib.h>
#include <math.h>

int main(){

int x = 0;

printf("Enter a natural number: ");
scanf("%d", &x);
int ans = x;

// Digit Counter
int counter = 0;          //Variable for number of digits in the user entered number
int b = x;                //For each time number can be divided by 10 and isnt 0
for (int i = 1; i <= x; i++){ // Then counter variable is incremented by 1
   b /= 10;
   if (b != 0){
     counter += 1;
   }            
}
++counter;

//Digit Counter
int sum = 0;
// Digit Finder
int D;
for (int j = 1; j <= x; j++){
   D = x % 10;               //Shows remainder of number (last digit) when divided by 10
   sum += pow(D, counter);   //Raises Digit found by counter and adds to sum
   printf("%d\n", sum);
   x /= 10;                  // Divides user entered number by 10 to get   rid of digit found
}



if (sum == ans){
   printf("%d is a Armstrong number! :)", ans);
}else 
   printf("%d is not an Armstrong number :(", ans);
   //Digit Finder

   return 0;
}

My problem is that the program works fine apart from one point, when the program is given a Armstrong number which does not start with 1 then it behaves normally and indicates if it is an Armstrong number or not, but when i input a Armstrong number which start with 1 then it will print out the Armstrong number but -1.

For example: If i input something such as 371 which is an Armstrong number it will show that it is an Armstrong number. However if i input 1634 it will output 1633 which is 1 less than 1634.

How can i fix this problem?, also by the way could someone comment on my code and tell me if it seems good and professional/efficient because i am a beginner in C and would like someone else's opinion on my code.

Answer 1

The condition in this loop

for (int i = 1; i <= x; i++){ // Then counter variable is incremented by 1
   b /= 10;
   if (b != 0){
     counter += 1;
   }            
}

does not make sense because there will be numerous redundant iterations of the loop.

For example if x is equal to 153 that is contains only 3 digits the loop will iterate exactly 153 times.

Also additional increment of the variable counter after the loop

++counter;

makes the code logically inconsistent.

Instead of the loop you could write at least the following way

int counter = 0;
int b = x;

do
{
    ++counter;
} while ( b /= 10 );

This loop iterates exactly the number of times equal to the number of digits in a given number.

In this loop

for (int j = 1; j <= x; j++){
   D = x % 10;               //Shows remainder of number (last digit) when divided by 10
   sum += pow(D, counter);   //Raises Digit found by counter and adds to sum
   printf("%d\n", sum);
   x /= 10;                  // Divides user entered number by 10 to get   rid of digit found
}

it seems you did not take into account that the variable x is decreased inside the body of the loop

   x /= 10;                  // Divides user entered number by 10 to get   rid of digit found

So the loop can interrupt its iterations too early. In any case the condition of the loop again does not make great sense the same way as the condition of the first loop and only adds a bug.

The type of used variables that store a given number should be unsigned integer type. Otherwise the user can enter a negative number.

You could write a separate function that checks whether a given number is an Armstrong number.

Here you are.

#include <stdio.h>

int is_armstrong( unsigned int x )
{
    const unsigned int Base = 10;
    
    size_t n = 0;

    unsigned int tmp = x;

    do 
    { 
        ++n; 
    } while ( tmp /= Base );
    
    unsigned int sum = 0;
    
    tmp = x;
    do
    {
        unsigned int digit = tmp % Base;
        unsigned int power = digit;
        
        for ( size_t i = 1; i < n; i++ ) power *= digit;
        
        sum += power;
    } while ( ( tmp /= Base ) != 0 && !( x < sum ) );
    
    return tmp == 0 && x == sum;
}

int main(void) 
{
    unsigned int a[] = 
    {
        0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 153, 370, 371, 407, 
        1634, 8208, 9474, 54748, 92727, 93084, 548834
    };
    const size_t N = sizeof( a ) / sizeof( *a );
    
    for ( size_t i = 0; i < N; i++ )
    {
        printf( "%u is %san Armstrong number.\n", a[i], is_armstrong( a[i] ) ? "": "not " );
    }

    return 0;
}

The program output is

0 is an Armstrong number.
1 is an Armstrong number.
2 is an Armstrong number.
3 is an Armstrong number.
4 is an Armstrong number.
5 is an Armstrong number.
6 is an Armstrong number.
7 is an Armstrong number.
8 is an Armstrong number.
9 is an Armstrong number.
153 is an Armstrong number.
370 is an Armstrong number.
371 is an Armstrong number.
407 is an Armstrong number.
1634 is an Armstrong number.
8208 is an Armstrong number.
9474 is an Armstrong number.
54748 is an Armstrong number.
92727 is an Armstrong number.
93084 is an Armstrong number.
548834 is an Armstrong number.

Answer 2

Let's take this and add the ability to handle multiple numeric bases while we're at it. Why? BECAUSE WE CAN!!!! :-)

#include <stdio.h>
#include <math.h>

double log_base(int b, double n)
  {
  return log(n) / log((double)b);
  }

int is_armstrong_number(int b,  /* base */
                        int n)
  {
  int num_digits = trunc(log_base(b, (double)n)) + 1;
  int sum = 0;
  int remainder = n;
  
  while(remainder > 0)
    {
    sum = sum + pow(remainder % b, num_digits);
    remainder = (int) (remainder / b);
    }

  return sum == n;
  }

int main()
  {
  printf("All the following are valid Armstrong numbers\n");

  printf("  407 base 10 - result = %d\n", is_armstrong_number(10, 407));
  printf("  0xEA1 base 16 - result = %d\n", is_armstrong_number(16, 0xEA1));
  printf("  371 base 10 - result = %d\n", is_armstrong_number(10, 371));
  printf("  1634 base 10 - result = %d\n", is_armstrong_number(10, 1634));
  printf("  0463 base 8 - result = %d\n", is_armstrong_number(8, 0463));
  
  printf("All the following are NOT valid Armstrong numbers\n");
  
  printf("  123 base 10 - result = %d\n", is_armstrong_number(10, 123));
  printf("  0x2446 base 16 - result = %d\n", is_armstrong_number(16, 0x2446));
  printf("  022222 base 8 - result = %d\n", is_armstrong_number(8, 022222));
  }

At the start of is_armstrong_number we compute the number of digits directly instead of looping through the number. We then loop through the digits of n in base b, summing up the value of the digit raised to the number of digits in the number, for the given numeric base. Once the remainder hits zero we know there are no more digits to compute and we return a flag indicating if the given number is an Armstrong number in the given base.

Answer 3

How can I fix this problem.

You know the number of iterations you want to make once you have calculated the digit count. So instead of looping till you reach the value of x:

for (int j = 1; j <= x; j++){

use the digit counter instead:

for (int j = 1; j <= counter; j++) {

---

also by the way could someone comment on my code and tell me if it seems good and professional/efficient because i am a beginner in C and would like someone else's opinion on my code.

There's a number of things you can do to improve your code.

1. First and foremost, any piece of code should be properly indented and formatted. Right now your code has no indenting, which makes it more difficult to read and it just looks ugly in general. So, always indent your code properly. Use an IDE or a good text editor, it will help you.

2. Be consistent in your code style. If you are writing

if (some_cond) {
 ...
}
else
   //do this

It is not consistent. Wrap the else in braces as well.

3. Always check the return value of a function you use, especially for scanf. It will save you from many bugs in the future.

if (scanf("%d", &x) == 1)
    //...all OK...
else
   // ...EOF or conversion failure...
   exit(EXIT_FAILURE);

4. Your first for loop will iterate x times uselessly. You can stop when you know that you have hit 0:

for (int i = 1; i <= x; i++){ // Then counter variable is incremented by 1
   b /= 10;
   if (b == 0){
      break;
   }
   counter += 1;    
}

5. C has ++ operator. Use that instead of doing counter += 1

6. int D; you create this, but don't initialize it. Always initialize your variables as soon as possible

7. C has const qualifier keyword, which makes a value immutable. This makes your code more readable, as the reader can immediately tell that this value will not change. In your code, you can change ans variable and make it a const int because it never changes:

const int ans = x;

8. Use more descriptive names for your variables. ans, D don't tell me anything. Use proper names, so that the reader of your code can easily understand your code.

These are some of the things that in my opinion you should do and keep doing to improve your code and coding skills. I am sure there can be more things though. Keep your code readable and as simple as possible.

Answer 4

Please remove j++ from 2nd loop for (int j = 1; j <= x; j++)

I tried this:

void armstrong(int x)
{
    // count digits
    int counter = 0, temp = x, sum = 0;
    while(temp != 0)
    {
        temp = temp/10;
        ++counter; // Note: pre increment faster
    }
    // printf("count %d\n",counter);
    
    temp = x;
    while(temp != 0)
    {
        sum += pow(temp % 10, counter);
        temp = temp/10;
    }
    // printf("sum %d\n",sum);
    if(x == sum)
    {
        printf("Armstrong\n");
    }
    else
    {
        printf("No Armstrong\n");
    }
}

int main(){

    armstrong(371);
    armstrong(1634);

   return 0;
}