Check Abecedarian word

Question

An 'abecedarian' is a word whose letters appear in alphabetical order. Write a program which will determine whether the given word is abecedarian or not and print 1 if abecedarian, 0 if not.

Input : user enter a string or word.
Output: print `1` if word is Abecedarian else `0`.

Test Cases :

abc    1
beee   1
cuanf  0

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

void main()
 {

   char str[20];

printf("\n Enter a string");

scanf("%s",str);

int len = strlen(str);

int i,flag = 1;

for(i=0;i < len - 1;i++)
 {

if(str[i+1] >= str[i])

    continue;

else
{ 
       flag = 0 ;
   break;
}   
}

if(flag == 0)

printf("\n String is not Abecedarian ");

else

printf("\n Is abecedarian");

}

Answer 1

Formatting

The obvious wrong thing with your code is the indentation (I don't know if it is because of the copy on CodeReview.SE).

Also, the lines breaks seem to be used randomly. They should be used to divide the different logical parts of your function/program.

You could use more spaces to make your code easier to go through.

Finally (but this is a tiny detail), you have loads of trailing whitespace making things a bit more painful to edit. You can easily remove them automatically.

Activate warnings in your compiler

It is usually a good idea to enable all warnings in your compiler and to take them into account. In your case, gcc complains about the return value for main.

Once this is fixed, your code looks like :

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

int main()
{
        char str[20];
        printf("\n Enter a string");
        scanf("%s",str);
        int len = strlen(str);
        int i,flag = 1;
        for(i=0; i < len - 1; i++)
        {
                if (str[i+1] >= str[i])
                        continue;
                else
                {
                        flag = 0 ;
                        break;
                }
        }

        if(flag == 0)
                printf("\n String is not Abecedarian ");
        else
                printf("\n Is abecedarian");
        return 0;
}

Logic

Your code seems OK but there is a way to improve how the logic is written. For instance, you could get rid of the continue by rewriting the condition. The inside of the loop becomes :

            if (str[i+1] < str[i])
            {
                    flag = 0 ;
                    break;
            }

In the same kind of spirit, I'd inverse the conditions for the printing as I find it easier to read something like :

    if(abcdarian)
            printf("\n Is abecedarian");
    else
            printf("\n String is not Abecedarian ");

More style

It is usually a good idea to try to move your variables in the smallest possible scope. In your case, using c99, you can move i into the for loop : for(int i=0; i < len - 1; i++).

In the same kind of spirit, it could be a good idea to create a function to handle the logic currently in the loop. Then you'd be able to just call if (isAbecedarian(str)).

About scanf

You'll find online a few problems with scanf and details about what you should be doing like using fgets.

Answer 2

Definition of alphabetical order

What is your definition of "alphabetical order"? It looks like you just compare bytes by ASCII value? If so, all uppercase letters will be considered as coming before all lowercase letters. (For example, "Hello" would be abecedarian.)

If you're using byte values, I suggest using unsigned char rather than char.

If you're using a more human-friendly standard, then you should normalize all input to either uppercase or lowercase when making the comparison.

Efficiency

Avoid calling strlen() if possible. It wastefully scans a character byte-by-byte until it finds the '\0' terminator. In the grand scheme of things, a strlen() call is not a big deal, but here it is completely needless. You can just check for the '\0' terminator as you crawl along the string.

Separation of concerns

You should define a function

int is_abecedarian(const unsigned char *word)
{
        unsigned char a = '\0';
        while (*word != '\0')
        {
                if (*word < a)
                {
                        return 0;
                }
                a = *word++;
        }
        return 1;
}

This has multiple advantages:

• The function gives that self-contained chunk of code a name and a purpose.
• That smaller chunk of code is easier to understand.
• main() can focus on its job, which is to prompt for a word, and print the yes/no result.
• You have the flexibility to reuse the code by calling it in other ways. For example, you could call the is_abecedarian() routine many times in a unit test.

Buffer overflow

It's easy to overflow the 20-byte buffer that you set aside for the input, because you call scanf() unsafely. Your program can crash, and may even be vulnerable to a security hole. You can limit the size of the input by specifying a width in the format string. (It would be even easier to use fgets() as @Josay suggests.)

#include <stdio.h>

#define BUFSIZE 20
#define STRINGIFY(x) #x
#define TOSTRING(x) STRINGIFY(x)

int main()
{
        unsigned char buf[BUFSIZE + 1];
        scanf("%" TOSTRING(BUFSIZE) "s", buf);
        if (is_abecedarian(buf))
        {
                printf("%s is abecedarian\n", buf);
        }
        else
        {
                printf("%s is not abecedarian\n", buf);
        }
        return 0;
}

Miscellaneous

• #include <string.h> is superfluous.

• Never omit optional braces, or bad things will happen. Don't think that such mistakes won't happen to you, because that is exactly how accidents occur. Think of it this way: by writing that code without braces, you were a contributing factor to a future accident.

  If you don't like the way the braces take up too much vertical space, then abandon that brace style in favour of something more compact. Alternatively, put the single-statement body on the same line as the condition:

  if (*word < a) return 0;