Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

There are a lot of good answers here, but most of them address your algorithm. I want to make the (incorrect) assumption that your algorithm is fine and focus on some readability issues your code has.

int lucky(long long int num)
{
    while(num>0)
    {
        if(!(num%10==1 || num%10==7 || num%10==9))
            return 0;
        num = num/10;
    }
    return 1;
}

First, let's talk just about the function signature:

int lucky(long long int num)

What does this method do? I know there's not a default boolean type in C, but there are plenty of good ways to introduce onethere are plenty of good ways to introduce one. We should do this in order to make it clear that our method returns a true or false value only. And with that said, functions that return boolean type values read a little bit better with an is prefix. And finally, since I personally have such a hard time remembering how big things like long long int actually are, I also like to create type definitions here.

typedef long long int int64_t;

I also don't see a particularly good reason to abbreviate the parameter. So now we're looking at a signature which actually looks more like this:

bool isLucky(int64_t number);

Now it's clear. isLucky takes a 64-bit integer called number and returns a bool which should indicate whether or not that number is actually lucky or not.

As a side note, we may want to only accept unsigned integers. In which case our type definition and function signature become:

typedef long long unsigned uint64_t;
bool isLucky(uint64_t number);

Now, let's look at the function body.

while(num>0)
{
    if(!(num%10==1 || num%10==7 || num%10==9))
        return 0;
    num = num/10;
}
return 1;

We've used a while loop, but the value we're using for our conditional is also being updated within the method body. Any time this is happening, I prefer to use a for loop:

for(; number > 0; number /= 10) {
    if(!(number%10==1 || number%10==7 || number%10==9)) {
        return false;
    }
}
return true;

Using the for loop, it's more clear what's happening to the number on each iteration of the loop. The importance of this increases as the complexity of the loop's body increases. With a for loop, we can generally see exactly what's going to happen within a single line.

Importantly, you'll also notice that I've added braces around your if statement. Omitting braces around if statements (or anywhere) should never be considered optional (no matter what the compiler thinks) because it leads to bugs.

There are a lot of good answers here, but most of them address your algorithm. I want to make the (incorrect) assumption that your algorithm is fine and focus on some readability issues your code has.

int lucky(long long int num)
{
    while(num>0)
    {
        if(!(num%10==1 || num%10==7 || num%10==9))
            return 0;
        num = num/10;
    }
    return 1;
}

First, let's talk just about the function signature:

int lucky(long long int num)

What does this method do? I know there's not a default boolean type in C, but there are plenty of good ways to introduce one. We should do this in order to make it clear that our method returns a true or false value only. And with that said, functions that return boolean type values read a little bit better with an is prefix. And finally, since I personally have such a hard time remembering how big things like long long int actually are, I also like to create type definitions here.

typedef long long int int64_t;

I also don't see a particularly good reason to abbreviate the parameter. So now we're looking at a signature which actually looks more like this:

bool isLucky(int64_t number);

Now it's clear. isLucky takes a 64-bit integer called number and returns a bool which should indicate whether or not that number is actually lucky or not.

As a side note, we may want to only accept unsigned integers. In which case our type definition and function signature become:

typedef long long unsigned uint64_t;
bool isLucky(uint64_t number);

Now, let's look at the function body.

while(num>0)
{
    if(!(num%10==1 || num%10==7 || num%10==9))
        return 0;
    num = num/10;
}
return 1;

We've used a while loop, but the value we're using for our conditional is also being updated within the method body. Any time this is happening, I prefer to use a for loop:

for(; number > 0; number /= 10) {
    if(!(number%10==1 || number%10==7 || number%10==9)) {
        return false;
    }
}
return true;

Using the for loop, it's more clear what's happening to the number on each iteration of the loop. The importance of this increases as the complexity of the loop's body increases. With a for loop, we can generally see exactly what's going to happen within a single line.

Importantly, you'll also notice that I've added braces around your if statement. Omitting braces around if statements (or anywhere) should never be considered optional (no matter what the compiler thinks) because it leads to bugs.

There are a lot of good answers here, but most of them address your algorithm. I want to make the (incorrect) assumption that your algorithm is fine and focus on some readability issues your code has.

int lucky(long long int num)
{
    while(num>0)
    {
        if(!(num%10==1 || num%10==7 || num%10==9))
            return 0;
        num = num/10;
    }
    return 1;
}

First, let's talk just about the function signature:

int lucky(long long int num)

What does this method do? I know there's not a default boolean type in C, but there are plenty of good ways to introduce one. We should do this in order to make it clear that our method returns a true or false value only. And with that said, functions that return boolean type values read a little bit better with an is prefix. And finally, since I personally have such a hard time remembering how big things like long long int actually are, I also like to create type definitions here.

typedef long long int int64_t;

I also don't see a particularly good reason to abbreviate the parameter. So now we're looking at a signature which actually looks more like this:

bool isLucky(int64_t number);

Now it's clear. isLucky takes a 64-bit integer called number and returns a bool which should indicate whether or not that number is actually lucky or not.

As a side note, we may want to only accept unsigned integers. In which case our type definition and function signature become:

typedef long long unsigned uint64_t;
bool isLucky(uint64_t number);

Now, let's look at the function body.

while(num>0)
{
    if(!(num%10==1 || num%10==7 || num%10==9))
        return 0;
    num = num/10;
}
return 1;

We've used a while loop, but the value we're using for our conditional is also being updated within the method body. Any time this is happening, I prefer to use a for loop:

for(; number > 0; number /= 10) {
    if(!(number%10==1 || number%10==7 || number%10==9)) {
        return false;
    }
}
return true;

Using the for loop, it's more clear what's happening to the number on each iteration of the loop. The importance of this increases as the complexity of the loop's body increases. With a for loop, we can generally see exactly what's going to happen within a single line.

Importantly, you'll also notice that I've added braces around your if statement. Omitting braces around if statements (or anywhere) should never be considered optional (no matter what the compiler thinks) because it leads to bugs.

deleted 11 characters in body
Source Link
nhgrif
  • 25.3k
  • 3
  • 63
  • 129

There are a lot of good answers here, but most of them address your algorithm. I want to make the (apparently incorrectincorrect) assumption that your algorithm is fine and focus on some readability issues your code has.

int lucky(long long int num)
{
    while(num>0)
    {
        if(!(num%10==1 || num%10==7 || num%10==9))
            return 0;
        num = num/10;
    }
    return 1;
}

First, let's talk just about the function signature:

int lucky(long long int num)

What does this method do? I know there's not a default boolean type in C, but there are plenty of good ways to introduce one. We should do this in order to make it clear that our method returns a true or false value only. And with that said, functions that return boolean type values read a little bit better with an is prefix. And finally, since I personally have such a hard time remembering how big things like long long int actually are, I also like to create type definitions here.

typedef long long int int64_t;

I also don't see a particularly good reason to abbreviate the parameter. So now we're looking at a signature which actually looks more like this:

bool isLucky(int64_t number);

Now it's clear. isLucky takes a 64-bit integer called number and returns a bool which should indicate whether or not that number is actually lucky or not.

As a side note, we may want to only accept unsigned integers. In which case our type definition and function signature become:

typedef long long unsigned uint64_t;
bool isLucky(uint64_t number);

Now, let's look at the function body.

while(num>0)
{
    if(!(num%10==1 || num%10==7 || num%10==9))
        return 0;
    num = num/10;
}
return 1;

We've used a while loop, but the value we're using for our conditional is also being updated within the method body. Any time this is happening, I prefer to use a for loop:

for(; number > 0; number /= 10) {
    if(!(number%10==1 || number%10==7 || number%10==9)) {
        return false;
    }
}
return true;

Using the for loop, it's more clear what's happening to the number on each iteration of the loop. The importance of this increases as the complexity of the loop's body increases. With a for loop, we can generally see exactly what's going to happen within a single line.

Importantly, you'll also notice that I've added braces around your if statement. Omitting braces around if statements (or anywhere) should never be considered optional (no matter what the compiler thinks) because it leads to bugs.

There are a lot of good answers here, but most of them address your algorithm. I want to make the (apparently incorrect) assumption that your algorithm is fine and focus on some readability issues your code has.

int lucky(long long int num)
{
    while(num>0)
    {
        if(!(num%10==1 || num%10==7 || num%10==9))
            return 0;
        num = num/10;
    }
    return 1;
}

First, let's talk just about the function signature:

int lucky(long long int num)

What does this method do? I know there's not a default boolean type in C, but there are plenty of good ways to introduce one. We should do this in order to make it clear that our method returns a true or false value only. And with that said, functions that return boolean type values read a little bit better with an is prefix. And finally, since I personally have such a hard time remembering how big things like long long int actually are, I also like to create type definitions here.

typedef long long int int64_t;

I also don't see a particularly good reason to abbreviate the parameter. So now we're looking at a signature which actually looks more like this:

bool isLucky(int64_t number);

Now it's clear. isLucky takes a 64-bit integer called number and returns a bool which should indicate whether or not that number is actually lucky or not.

As a side note, we may want to only accept unsigned integers. In which case our type definition and function signature become:

typedef long long unsigned uint64_t;
bool isLucky(uint64_t number);

Now, let's look at the function body.

while(num>0)
{
    if(!(num%10==1 || num%10==7 || num%10==9))
        return 0;
    num = num/10;
}
return 1;

We've used a while loop, but the value we're using for our conditional is also being updated within the method body. Any time this is happening, I prefer to use a for loop:

for(; number > 0; number /= 10) {
    if(!(number%10==1 || number%10==7 || number%10==9)) {
        return false;
    }
}
return true;

Using the for loop, it's more clear what's happening to the number on each iteration of the loop. The importance of this increases as the complexity of the loop's body increases. With a for loop, we can generally see exactly what's going to happen within a single line.

Importantly, you'll also notice that I've added braces around your if statement. Omitting braces around if statements (or anywhere) should never be considered optional (no matter what the compiler thinks) because it leads to bugs.

There are a lot of good answers here, but most of them address your algorithm. I want to make the (incorrect) assumption that your algorithm is fine and focus on some readability issues your code has.

int lucky(long long int num)
{
    while(num>0)
    {
        if(!(num%10==1 || num%10==7 || num%10==9))
            return 0;
        num = num/10;
    }
    return 1;
}

First, let's talk just about the function signature:

int lucky(long long int num)

What does this method do? I know there's not a default boolean type in C, but there are plenty of good ways to introduce one. We should do this in order to make it clear that our method returns a true or false value only. And with that said, functions that return boolean type values read a little bit better with an is prefix. And finally, since I personally have such a hard time remembering how big things like long long int actually are, I also like to create type definitions here.

typedef long long int int64_t;

I also don't see a particularly good reason to abbreviate the parameter. So now we're looking at a signature which actually looks more like this:

bool isLucky(int64_t number);

Now it's clear. isLucky takes a 64-bit integer called number and returns a bool which should indicate whether or not that number is actually lucky or not.

As a side note, we may want to only accept unsigned integers. In which case our type definition and function signature become:

typedef long long unsigned uint64_t;
bool isLucky(uint64_t number);

Now, let's look at the function body.

while(num>0)
{
    if(!(num%10==1 || num%10==7 || num%10==9))
        return 0;
    num = num/10;
}
return 1;

We've used a while loop, but the value we're using for our conditional is also being updated within the method body. Any time this is happening, I prefer to use a for loop:

for(; number > 0; number /= 10) {
    if(!(number%10==1 || number%10==7 || number%10==9)) {
        return false;
    }
}
return true;

Using the for loop, it's more clear what's happening to the number on each iteration of the loop. The importance of this increases as the complexity of the loop's body increases. With a for loop, we can generally see exactly what's going to happen within a single line.

Importantly, you'll also notice that I've added braces around your if statement. Omitting braces around if statements (or anywhere) should never be considered optional (no matter what the compiler thinks) because it leads to bugs.

Source Link
nhgrif
  • 25.3k
  • 3
  • 63
  • 129

There are a lot of good answers here, but most of them address your algorithm. I want to make the (apparently incorrect) assumption that your algorithm is fine and focus on some readability issues your code has.

int lucky(long long int num)
{
    while(num>0)
    {
        if(!(num%10==1 || num%10==7 || num%10==9))
            return 0;
        num = num/10;
    }
    return 1;
}

First, let's talk just about the function signature:

int lucky(long long int num)

What does this method do? I know there's not a default boolean type in C, but there are plenty of good ways to introduce one. We should do this in order to make it clear that our method returns a true or false value only. And with that said, functions that return boolean type values read a little bit better with an is prefix. And finally, since I personally have such a hard time remembering how big things like long long int actually are, I also like to create type definitions here.

typedef long long int int64_t;

I also don't see a particularly good reason to abbreviate the parameter. So now we're looking at a signature which actually looks more like this:

bool isLucky(int64_t number);

Now it's clear. isLucky takes a 64-bit integer called number and returns a bool which should indicate whether or not that number is actually lucky or not.

As a side note, we may want to only accept unsigned integers. In which case our type definition and function signature become:

typedef long long unsigned uint64_t;
bool isLucky(uint64_t number);

Now, let's look at the function body.

while(num>0)
{
    if(!(num%10==1 || num%10==7 || num%10==9))
        return 0;
    num = num/10;
}
return 1;

We've used a while loop, but the value we're using for our conditional is also being updated within the method body. Any time this is happening, I prefer to use a for loop:

for(; number > 0; number /= 10) {
    if(!(number%10==1 || number%10==7 || number%10==9)) {
        return false;
    }
}
return true;

Using the for loop, it's more clear what's happening to the number on each iteration of the loop. The importance of this increases as the complexity of the loop's body increases. With a for loop, we can generally see exactly what's going to happen within a single line.

Importantly, you'll also notice that I've added braces around your if statement. Omitting braces around if statements (or anywhere) should never be considered optional (no matter what the compiler thinks) because it leads to bugs.