Can someone help me fine-tune my **else if** statement in this code?

Question

I am trying to write a code that checks the user entered password against a Password Format e.g. "L$$222ee" (first letter uppercase, followed by two special characters, followed by three digits and finally two lowercase letters). My code is working fine; I just want to refine the else if statement. Here is the code:

#include<stdio.h>
#include<string.h>
#include<ctype.h>
//format required: L$$222ee
int main(){
    char pass[8];
    printf("Enter password:\n");
    gets(pass);
    if(strlen(pass)<8){
        printf("Password is too short...!!!\n");
    }
    else if(isupper(pass[0]) && ((!isalpha(pass[1]) && !isalpha(pass[2])) && !isdigit(pass[1]) && !isdigit(pass[2])) 
    && (isdigit(pass[3]) && isdigit(pass[4]) && isdigit(pass[5])) && (isalpha(pass[6]) 
    && isalpha(pass[7]))){
        printf("Password is compliant");
    }
    else{
        printf("Error: password is not compliant with the format!!!\n");
    }
    return 0;
}

Answer 1

Since all these ctype.h functions have the same format int func (int), you can take advantage of that to create a look-up table corresponding to all checks that need to be done. The advantage of a look-up table is that it's much easier to maintain than a fairly unreadable chain of &&.

First create a function typedef identical to the ctype functions: typedef int ctype_func_t (int);

Then make a struct to be used in the look-up table. Store string index, the true or false condition of the result, and the function itself:

typedef struct
{
  size_t         index;
  bool           cond;
  ctype_func_t*  check_func;
} check_t;

A look-up table can then be created as:

const check_t fmt_check [] =
{
  {0, true,  isupper },
  {1, false, isalpha },
  {2, false, isalpha },
  {1, false, isdigit },
  {2, false, isdigit }, 
  {3, true,  isdigit },
  {4, true,  isdigit },
  {5, true,  isdigit },
  {6, true,  isalpha },
  {7, true,  isalpha },
};

The order isn't important, add or remove format requirements as you please. The complete function for this would look like:

int ispwcorrect (const char pw[8])
{
  typedef int ctype_func_t (int);
  typedef struct
  {
    size_t         index;
    bool           cond;
    ctype_func_t*  check_func;
  } check_t;

  const check_t fmt_check [] =
  {
    {0, true,  isupper },
    {1, false, isalpha },
    {2, false, isalpha },
    {1, false, isdigit },
    {2, false, isdigit }, 
    {3, true,  isdigit },
    {4, true,  isdigit },
    {5, true,  isdigit },
    {6, true,  isalpha },
    {7, true,  isalpha },
  };
  
  for(size_t i=0; i<sizeof fmt_check/sizeof *fmt_check; i++)
  {
    unsigned char ch = pw[ fmt_check[i].index ];
    
    if((bool)fmt_check[i].check_func(ch) != fmt_check[i].cond)
    {
      return false;
    }
  }  
  return true;
}

This should be fairly self-explanatory, it calls ctype functions through a function pointer in a loop with a specific string index character as parameter. Then checks against a true/false condition. The check stops as soon as there is a mismatch.

The cast to bool after calling each ctype function is necessary, since they return zero or non-negative int. So comparing a bool against such a non-zero int won't work, since == will promote the bool parameter to an int.

This should vastly outperform regex, but will be slightly slower than the raw if-else.

---

Unrelated to your question, you also have a severe bug here: char pass[8];. There is no room for the null terminator, so you can't store "L$$222ee" inside pass.

Answer 2

Alternatively, on Unix systems, you can use the regex.h module, which supports the POSIX basic regular expression syntax. In this case, the example will look like this:

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

//format required: L$$222ee
char pattern[] = "[[:upper:]][^[:alnum:]]\\{2\\}[[:digit:]]\\{3\\}[[:lower:]]\\{2\\}";
int main(){
    char pass[8];
    printf("Enter password:\n");
    gets(pass);

    regex_t regex;
    int ret;
    char msgbuf[100];

    if(strlen(pass)<8){
        printf("Password is too short...!!!\n");
        return 0;
    }

    // Compile regular expression
    ret = regcomp(&regex, pattern, 0);
    if (ret) {
        fprintf(stderr, "Could not compile regex\n");
        return 0;
    }
    /* Execute regular expression */
    ret = regexec(&regex, pass, 0, NULL, 0);
    if (!ret) {
        printf("Password is compliant");
    }
    else if (ret == REG_NOMATCH) {
        printf("Error: password is not compliant with the format!!!\n");
    }
    else {
        regerror(ret, &regex, msgbuf, sizeof(msgbuf));
        fprintf(stderr, "Regex match failed: %s\n", msgbuf);
    }

    return 0;
}

Answer 3

Let me help you, not with giving you an answer, but showing you how you can make that "else-if" clause more readable, this is what you have:

else if(isupper(pass[0]) && ((!isalpha(pass[1]) && !isalpha(pass[2])) && !isdigit(pass[1]) && !isdigit(pass[2])) 
    && (isdigit(pass[3]) && isdigit(pass[4]) && isdigit(pass[5])) && (isalpha(pass[6]) 
    && isalpha(pass[7])))

You can "re-write" this (just using some indentation, newlines and comments), like this:

else if( isupper(pass[0]) &&                             // 0   : Must be upper 
         ( ( !isalpha(pass[1]) && !isalpha(pass[2])) &&  // 1,2 : both should not be alpha
           !isdigit(pass[1]) &&                          // 1   : should not be digit
           !isdigit(pass[2])                             // 2   : should not be digit
           ) && 
         ( isdigit(pass[3]) &&                           // 3   : Must be digit
           isdigit(pass[4]) &&                           // 4   : Must be digit
           isdigit(pass[5])                              // 5   : Must be digit
         ) && 
         ( isalpha(pass[6]) &&                           // 6   : Must be alpha
           isalpha(pass[7])                              // 7   : Must be alpha
         )
       )

It takes some extra writing and editing, but it makes the whole thing more readable, comprehensible and as a result, easily maintainable (seen the fact that rules about passwords are rapidly changing, maintainability of such functions is quite important).

By the way, just by looking at this, you easily realise that you can re-write the part about pass[1] and pass[2]:

Currently:

        ( ( !isalpha(pass[1]) && !isalpha(pass[2])) &&  // 1,2 : both should not be alpha
           !isdigit(pass[1]) &&                          // 1   : should not be digit
           !isdigit(pass[2])                             // 2   : should not be digit
           ) && 

Proposal:

          ( !isalpha(pass[1]) && !isalpha(pass[2]) ) &&  // 1,2 : both should not be alpha
          ( !isdigit(pass[1]) && !isdigit(pass[2]) ) &&  // 1,2 : both should not be digit