How efficient is my code that determines if a password is valid or not?

Question

The requirements are that the password has to include an upper-case letter, an number, and the "$" sign. Everything works (except for when the user inputs a space for some reason, so help on that would be appreciated), but idk how efficient my code is. I'm new to C so, any advice? (Also is there any way to set a "maximum length" for my password other than just setting it to something really high or forcing the user to follow the limit?)

int main(void){

int maxlength = 15;
char password[maxlength];
int index = 0;
int x = 0;
int y = 0;
int z = 0;

printf("Enter Password: "); //Mike$4
scanf(" %s", password);

do{ // If index is strlen, then we checked every char of pw
    // Ex. Password is length 6, then index 0 - 5 will be checked

    if(index == strlen(password) && x>0 && y>0 && z>0){
        printf("Good password!");
        break;
    }
    if(index == strlen(password) && (x==0 || y==0 || z==0)){
        printf("BAD PASSWORD");
        break;
    }

    if(isupper(password[index]) ||  isdigit(password[index]) ||
       password[index] == '$'){

         if(isupper(password[index])){
                x++; index++;
                continue;}
         if(isdigit(password[index])){
                y++; index++;
                continue;}
         if(password[index] == '$'){
                z++; index++;
                continue;}
    }else{index++;
          continue;
          }
}while(index <= strlen(password));
return 0;}

Basically every time a requirement is hit I just noted it by incrementing x, y, or z, and if by the end of the password, they all have at least 1, then it's a good password.

Thanks!

I'm asking if there's a better way to code this, because my next CS course in school will mark based on efficiency as well so I wanna make sure I know what makes a C code efficient or not. And of course, how to deal with the space in the password.

For what it's worth, you might want to check out the latest [NIST guidance about password policies](https://en.wikipedia.org/wiki/Password_policy#NIST_guidelines). — Daniel Pryden, Dec 31 '18 at 21:12
It's tempting to post a more efficient version of the code, but the correct place to ask for such suggestions is the [code review site](https://codereview.stackexchange.com/). As for how to deal with the space character: on any disallowed character you can immediately print the "BAD" message, and `break` the loop. — user3386109, Dec 31 '18 at 21:15
How does your school define "efficiency?" Optimum performance is hardly a useful criteria if all you're doing is checking a single password. — Robert Harvey, Dec 31 '18 at 21:25
When you read the password with `scanf("%s", password)`, you are guaranteed that the reading skip any leading white space (including newlines), and will then stop at the first white space character — a blank, tab, or newline (or one of a few others) — after that. If you want to read a line, use `fgets()`, but remember that the string returned by `fgets()` will include the newline (in general, unless there isn't enough space for the whole line). — Jonathan Leffler, Dec 31 '18 at 21:37
For effective password entry checking, you may want to put the keyboard in non-canonical mode and respond to each keypress. That is the only way you can flag any unwanted character at the time it is entered and before the next key is pressed. See [Hide password input on terminal](https://stackoverflow.com/questions/6856635/hide-password-input-on-terminal) — David C. Rankin, Dec 31 '18 at 21:49
Notes: 1) The `" "` in `scanf(" %s", password);` serves no purpose. Consider `scanf("%s", password);`. 2) Add a width limit like `scanf("%14s", password);` — chux - Reinstate Monica, Dec 31 '18 at 23:30
`while(index <= strlen(password)` is inefficient unless the compiler recognizes the constancy of `password` in the loop. — chux - Reinstate Monica, Dec 31 '18 at 23:35
@DanielPryden: [but Mordac says...](https://dilbert.com/strip/1998-04-06) — Bob Jarvis - Слава Україні, Jan 01 '19 at 00:05
If you're examining each character in the buffer there's no need to use `strlen()`. Instead, use something like `char *ptr; ... for(ptr = password ; *ptr ; ptr++)`. Best of luck. — Bob Jarvis - Слава Україні, Jan 01 '19 at 00:14

chux - Reinstate Monica · Answer 1 · 2019-01-01T00:32:32.913

How efficient is my code ....

A common pitfall of coding is focusing on performance, yet the C code has undefined behavior (UB) in it. Fix UB first, then address efficiency.

Buffer overrun

scanf(" %s", password); provided no width limit. A 15-character, or more, password will invoke UB by attempting to write outside password[].

Negative char

Early in coding, one is not likely to encounter char with negative values, yet when they happen is...(int ch) functions are a problem. Here, ch must have a value in the unsigned char range or EOF, else UB strikes again.

// isupper(password[index])
isupper((unsigned char) password[index])

With UB out of the way, consider that scanf(" %s", password); can easily takes 100x the time of your while loop. So optimizing your code borders on micro-optimization.

Avoid O(n*n) code

Assume the password is length n and then to find the length of a strings obliges a walk through all n characters via strlen(). Also code is doing n loops calling strlen() each iteration. That is n*n time.

Many compilers today will see that password does not change in the loop and so the result of strlen(password) will be remembered, preventing n*n iterations.

do{
  if(index == strlen(password) && x>0 && y>0 && z>0){
  ...
} while(index <= strlen(password));

Still, it is just as simple to substitute non-iterative code or calculate the length, once, outside the loop.

size_t len = strlen(password);

do {
  // if(index == strlen(password) ...
  if(password[index] != '\0' ...
  // or 
  if(index == len ...

  // }while(index <= strlen(password));
  } while(index <= len);

With such short strings, the length being type int is fine, yet for general array code, including string, size_t is the right-size type for array indexing and lengths.

Using informative names like upper_count rather than x makes for better code.

Sample untested alternative code. Code specifically uses fgetc() versus other input means as a step toward security - there are many more steps.

#include <ctype.h>
#include <stdbool.h>
#include <stdio.h>
#define PWD_N 6

int main(void) {
  size_t maxlength = 15;
  char password[maxlength + 1]; // add 1 for \0
  bool upper = false;
  bool digit = false;
  bool dollar = false;
  bool length_ok = true;

  printf("Enter Password: "); //Mike$4
  fflush(stdout);

  size_t i = 0;
  for (;;) {
    int ch = fgetc(stdin);
    if (ch == '\n' || ch == EOF) {
      break;
    }
    if (i < sizeof password - 1) {
      password[i++] = ch;
      upper |= isupper(ch);
      digit |= isdigit(ch);
      dollar |= ch == '$';
    } else {
      length_ok = false; // too long
    }
  }
  password[i] = '\0';

  if (i == PWD_N && length_ok && upper && digit && dollar) {
    printf("Good password!\n");
  } else {
    printf("BAD PASSWORD\n");
  }
  return 0;
}

How efficient is my code that determines if a password is valid or not?

1 Answers1