Validating last digit of CNIC always returns invalid input?

Question

So, I've been trying to validate CNIC because Pakistan requires male CNIC's to end in odd number and female CNIC's to end in even number. CNIC is stored as a string in a structure containing information about a bank user. The problem is when i apply the logic, it doesn't seem to work as intended and always prints the invalid CNIC prompt even when it is correctly input. Here is the relevant code:

    struct account//The structure used to store the records
{
  int  mm,dd,yyyy,Deposit;
  long long int accountnum;
  char name[50];
  char address[50];
  char Account[50];
  char CNIC[50];
  char Num[50];
  char gender;
} s[100];

void ValidCNIC(int j) {
    int i = 0, check = 0;
    char ch;
    printf("\n \tCNIC: ");
    fgets(s[j].CNIC,sizeof(s[j].CNIC),stdin);
    fflush(stdin);
    while(i < strlen(s[j].CNIC)-1) {
            ch = s[j].CNIC[i];
            if(ch >= '0' && ch <= '9') {
                    i++;
            }
            else{
                    printf(" \tThe CNIC can contain numbers only!\n");
                    fordelay(1000000000);
                    ValidCNIC(j);

            }
    }
    if (strnlen(s[j].CNIC,sizeof(s[j].CNIC)) < 14)
    {
            printf(" \tThe CNIC is too short\n \tPlease reenter\n");
            fordelay(1000000000);
            ValidCNIC(j);
    }
    else if (strnlen(s[j].CNIC,sizeof(s[j].CNIC)) > 14) {
            printf(" \tThe CNIC is too long\n \tPlease reenter\n");
            fordelay(1000000000);
            ValidCNIC(j);
    }
    int len = strnlen(s[j].CNIC,sizeof(s[j].CNIC));
    if((s[j].gender == 'm') && ((s[j].CNIC[len] !='1')||(s[j].CNIC[len] !='3')||(s[j].CNIC[len] !='5')||(s[j].CNIC[len] !='7')||(s[j].CNIC[len] !='9')))
       {
           printf("invalid CNIC, male CNIC must always end in an odd number");
           ValidCNIC(j);
       }
    else if((s[j].gender == 'w') && ((s[j].CNIC[len] !='0')||(s[j].CNIC[len] !='2')||(s[j].CNIC[len] !='4')||(s[j].CNIC[len] !='6')||(s[j].CNIC[len] !='8')))
    {
           printf("Invalid CNIC, female CNIC must end in an even number");
           ValidCNIC(j);
    }

`s[j].CNIC[len]` after `int len = strnlen(s[j].CNIC,sizeof(s[j].CNIC));` will always be zero unless the string is too long because strings in C are terminated by a null-character. — MikeCAT, Jun 17 '21 at 17:37
Also note that `fflush(stdin);` [invokes *undefined behavior*](https://stackoverflow.com/questions/2979209/using-fflushstdin). — MikeCAT, Jun 17 '21 at 17:40
Instead of `if(ch >= 48 && ch <= 57)` you probably meant `if(ch >= '0' && ch <= '9')`, which is better written `if(isdigit(ch))`. — William Pursell, Jun 17 '21 at 17:50

William Pursell · Answer 1 · 2021-06-17T19:35:22.317

You ought not take input in your validation function; just validate. Something like:

#include <stdio.h>
#include <ctype.h>
int
isValidCNIC(const char *cnic, char gender)
{
        size_t len = 0;
        const char *s = cnic;
        char c;
        if( gender != 'm' && gender != 'f' ){
                fprintf(stderr, "invalid gender: %c\n", gender);
                return 0;
        }
        while( *cnic ){
                len += 1;
                if( ! isdigit(c = *cnic++) ){
                        fprintf(stderr, "%s: invalid character %c\n", s, c);
                        return 0;
                }
        }
        if( len != 14 ){
                fprintf(stderr, "%s: too %s\n", s, len < 14 ? "short" : "long");
                return 0;
        }
        if( ((c - '0') % 2) != ( gender == 'm' ) ){
                fprintf(stderr, "%s: invalid parity\n", s);
                return 0;
        }
        return 1;
}

int
main(void)
{
        struct test { char *cnic; int gender; } t[] = {
                { "34576271345678", 'f' },
                { "34576271345677", 'm' },
                { "34576271345678", 'm' },
                { "3457627134678", 'm' },
                { "345762713456788", 'm' },
                { "3457a271345788", 'k' },
                { NULL, 0 }
        };
        for( struct test *p = t; p->cnic; p++ ){
                if( isValidCNIC( p->cnic, p->gender) ){
                        printf("valid: %s\n", p->cnic);
                }
        }
}

But note that strspn does most of the work that you are trying to do, and you could refactor to:

#include <stdio.h>
#include <string.h>
#include <ctype.h>
int
isValidCNIC(const char *cnic, char gender)
{
        char err[256] = "";
        size_t len = strspn(cnic, "0123456789");
        const char *s = cnic;

        if( gender != 'm' && gender != 'f' ){
                strcat(err, "invalid gender, ");
        }
        if( cnic[len] ){
                strcat(err, "invalid character, ");
        }
        if( len != 14 ){
                strcat(err, len < 14 ? "too short, " : " too long, ");
        }
        if( ((cnic[len - 1] - '0') % 2) != ( gender == 'm' ) ){
                strcat(err, "invalid parity, ");
        }
        if( *err ){
                err[strlen(err) - 2] = '\0';
                fprintf(stderr, "%s, %c: %s\n", s, gender, err);
        }
        return !*err;
}

int
main(void)
{
        struct test { char *cnic; int gender; } t[] = {
                { "34576271345678", 'f' },
                { "34576271345677", 'm' },
                { "34576271345678", 'm' },
                { "3457627134678", 'm' },
                { "345762713456788", 'm' },
                { "3457a271345788", 'k' },
                { NULL, 0 }
        };
        for( struct test *p = t; p->cnic; p++ ){
                if( isValidCNIC( p->cnic, p->gender) ){
                        printf("valid: %s\n", p->cnic);
                }
        }
}

Craig Estey · Answer 2 · 2021-06-17T20:09:29.450

Use '0' and '9' instead of 48 and 57.

Do not recursively call ValidCNIC from within itself. This can lead to infinite recursion

Add an (e.g.) outer that checks an error flag/value and loops if there is an error

Using s[j].CNIC[i] everywhere is cumbersome. Better to set (e.g.) char *cnic = s[j].CNIC; and use that.

No need to call strlen in the loop. Just call it once and save the value.

Your even/odd check can be simplified

Here's a refactored version. It is annotated. It compiles but I've not tested it:

#include <stdio.h>
#include <string.h>

//The structure used to store the records
struct account {
    int mm, dd, yyyy, Deposit;
    long long int accountnum;
    char name[50];
    char address[50];
    char Account[50];
    char CNIC[50];
    char Num[50];
    char gender;
} s[100];

#define ERRSET(_fmt) \
    { \
        err = _fmt; \
        break; \
    }

void
ValidCNIC(int j)
{
    int i;
    char ch;
    const char *err;

    while (1) {
        printf("\n \tCNIC: ");

        struct account *acct = &s[j];
        fgets(acct->CNIC, sizeof(acct->CNIC), stdin);
        char *cnic = acct->CNIC;

        // strip the newline
        cnic[strcspn(cnic,"\n")] = 0;

        size_t cnlen = strlen(cnic);

        err = NULL;

        do {
            // ensure CNIC has only digits
            for (i = 0;  i < cnlen;  ++i) {
                ch = cnic[i];
                if ((ch < '0') || (ch > '9'))
                    ERRSET(" \tThe CNIC can contain numbers only!\n");
            }
            if (err != NULL)
                break;

            // ensure CNIC length is correct
            if (cnlen < 14)
                ERRSET(" \tThe CNIC is too short\n \tPlease reenter\n");
            if (cnlen > 14)
                ERRSET(" \tThe CNIC is too long\n \tPlease reenter\n");

            int isodd = cnic[cnlen - 1];
            isodd -= '0';
            isodd %= 2;

            // ensure even/odd of CNIC matches gender
            switch (acct->gender) {
            case 'm':
                if (! isodd)
                    ERRSET("invalid CNIC, male CNIC must always end in an odd number");
                break;
            case 'w':
                if (isodd)
                    ERRSET("Invalid CNIC, female CNIC must end in an even number");
                break;
            }
        } while (0);

        // no errors
        if (err == NULL)
            break;

        // show the error
        printf("%s",err);
    }
}

UPDATE:

It still doesn't seem to work either, keeps giving the same prompt of Invalid CNIC. – Bas_Anar

Oops, my bad ... I forgot to remove the newline after the fgets. I've edited the example code above.

Btw what does that preprocessor directive do? – Bas_Anar

The macro arg is _fmt. The macro just does:

err = _fmt;
break;

where it is used.

So, it sets an error string and breaks out of the containing loop.

At the bottom, if err is NULL, there is no error and we're done.

Otherwise, it prints the error string and the outer loop reprompts the user.

Here's an example of the expanded macro text:

// ensure CNIC length is correct
if (cnlen < 14) {
    err = " \tThe CNIC is too short\n \tPlease reenter\n";
    break;
}

It still doesn't seem to work either, keeps giving the same prompt of Invalid CNIC. — Bas_Anar, Jun 17 '21 at 19:03

score 0 · Accepted Answer · answered Jun 17 '21 at 18:43

Recursion does not seem well suited to this task. Consider a loop until the input is ok. strspn can be used to test the input for digits. index will be the first character that is not a digit. Following fgets and all digits, that character should be a newline ( '\n'). Use modulus ( '% 2') to determine if the last digit is odd or even. This is not exactly what is needed but should give ideas that can be adapted.

#include <stdio.h>
#include <string.h>

#define LENGTH 50

int main ( void) {
    char CNIC[LENGTH] = "";
    int ok = 0;
    int gender = 'm';

    while ( ! ok) {
        printf ( "enter CNIC\n");
        fgets ( CNIC, sizeof CNIC, stdin);
        size_t index = strspn ( CNIC, "0123456789");
        if ( 14 <= index && '\n' == CNIC[index]) {
            int mod = CNIC[index - 1] % 2;
            if ( 'm' == gender) {
                if ( mod ) {//odd
                    ok = 1;
                }
                else {
                    printf ( "male CNIC must be odd\n");
                }
            }
            if ( 'w' == gender) {
                if ( ! mod ) {//even
                    ok = 1;
                }
                else {
                    printf ( "female CNIC must be odd\n");
                }
            }
        }
        else {
            printf ( "enter digits only, at least 14. try again\n");
        }
    }

    return 0;
}

Thanks this worked! However what does strspn do here? – Bas_Anar Jun 17 '21 at 19:23 — Bas_Anar, Jun 17 '21 at 19:23

Validating last digit of CNIC always returns invalid input?

3 Answers3