2
\$\begingroup\$

I have this little function to validate e-mail. It's a little piece of code to search major mistakes in e-mail input and it's part of a bigger program I have to complete as a school assignment. Is there a better way to do it in C?

I'd like to do it in regex but if I have understood correctly, C under Windows doesn't support complex regex. And I use scanf to make testing this code simpler. Later, I replace it with another function that reads user input. Test is an array test[40] that is declared in main.

In addition there is a compiler warning about my for loop, that value computed is not used. It probably means *ch. I use it to end for loop when *ch reaches '\0'. I know I could use while loop. However, is there a solution to fix that or a workaround or a better solution using for loop? And I'd like to know, what would you do differently or what would you also check in addition to what I am checking in my code.

int test(char test[])
{
    int     i;
    char    *pos1, *pos2;
    char    *ch;

    puts("Enter email!");
    scanf("%39s", test);

    while (1) {
        for (i = 0, ch = test; *ch; *ch++) {
                if (*ch == '@') {
                    pos1 = ch;  // <-stores last @ address
                    i++;
                }
            }
            pos2 = ch;          // <-stores end address ("\0") of test

        /* If there is only one "@" */
        if (i == 1) {

            /* Pos1 - test <-a character must be before "@". Ch - pos1 <-Counts
             * backwards from the end of the string towards "@". A character
             * must be between "@" and "." */
            while (pos1 - test && ch - pos1 > 1) {

                /* From the end of string to "." has to be atleast 2 chars */
                if ((pos2 - ch) > 2 && *ch == '.') {
                    printf("pos2 - ch is %d and *ch is %c",pos2 - ch, *ch);
                    return 1;
                }
                ch--;
            }
        }
        puts("Email wrong! Enter again");
        scanf("%39s", test);
    } /* End while */

    return 1;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ A 'little function' might not be sufficient here. See the Wikipedia article 'Email address', section 'Validation and verification' (link) and RFC-s linked there. Some relevant examples and nice comments available in 'I Knew How To Validate An Email Address Until I Read The RFC' at 'You've Been Haacked'. \$\endgroup\$ – CiaPan Mar 25 '16 at 12:21
  • \$\begingroup\$ @CiaPan- Thank you. That's why it is a "little function". It seems, that validating e-mail requires a lot of checking. \$\endgroup\$ – Name Mar 25 '16 at 14:43
2
\$\begingroup\$

Formatting

for (i = 0, ch = test; *ch; *ch++) {
        if (*ch == '@') {
            pos1 = ch;  // <-stores last @ address
            i++;
        }
    }
    pos2 = ch;

You have an extraneous level of indentation in there that throws out that block.

int     i;
char    *pos1, *pos2;
char    *ch;

This is a weird indentation level, it doesn't match anything else and is unnecessary. It literally just complicates how you read it.


Naming

  • int test(char test[]):

Naming both your function and variables test is bad practice. How am I meant to tell as an outsider reading your code what it is that your code does.

/* Pos1 - test <-a character must be before "@". Ch - pos1 <-Counts
 * backwards from the end of the string towards "@". A character
 * must be between "@" and "." */

If you feel the need to explain your code with comment blocks like this, you can probably improve your naming.

Name your variables as to what they are. For Example: charactersAfterAt.


Duplicate Logic

scanf("%39s", test);
while(1){
    // ...
    scanf("%39s", test);
}

You can reduce duplicate logic by declaring the scan inside the loop at the top.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.