I decided the task in codewars.com which called "Detect Pangram" (A pangram is a sentence that contains every single letter of the alphabet at least once. For example, the sentence "The quick brown fox jumps over the lazy dog" is a pangram, because it uses the letters A-Z at least once (case is irrelevant). Given a string, detect whether or not it is a pangram. Return True if it is, False if not. Ignore numbers and punctuation.), so I wrote that code:

main.c

#include <stdio.h>
#include <stdlib.h>

int is_pangram(char *str)
{
    int j = 0;                                  // start counting the number of characters in a string 
    while (str[j])
    {
        j  ;
    }                                           // end counting
    for (int i = 0; i < j; i  )                 // checking all characters in this string on a capital letter
    {
        if (str[i] >= 65 && str[i] <= 90)       // if this character is a capital letter (in the ASCII from 65 to 90),
                                                // we set it to low letter
        {
            str[i] = (int)str[i]   32;
        }
    }
    char alphabet[26];                          // creating an empty array    
    int count_add_to_alph = 0;                  // set a counter for successfull setting a letter to the array 
    int counter_char = 0;                       // set a counter for array of characters
    while (str[counter_char])                   // start checking each character 
    {
        if (str[counter_char] >= 'a' && str[counter_char] <= 'z' && str[counter_char] != alphabet[str[counter_char] - 'a'])
        {                                       // if the number of ASCII of the character is a low letter 
                                                // and this letter is not set in alphabet we set this letter 
                                                // into an array alphabet
            alphabet[str[counter_char] - 'a'] = str[counter_char];
            count_add_to_alph  ;
        }
        counter_char  ;
    }                                           // end checking
    for (int i = 0; i < 26; i  )                // setting each character to 0
    {
        alphabet[i] = 0;
    }

    if (count_add_to_alph == 26)                // if all letters of english alphabet were added to the alphabet array,
                                                // we return true, else false
    {
        return count_add_to_alph;
    }
    else
    {
        return 0;
    }
}

I started testing that in codewars and it returned me that:

error

Test Results:
Example_Tests
should_pass_all_the_tests_provided
Test Crashed
Caught unexpected signal: SIGSEGV (11). Invalid memory access.
Completed in 0.0000ms

CodePudding user response：

You are modifying the input string. This is "okay" but hazardous. It would segfault if called using a string constant [as the OS/implementation would probably put the constant into readonly memory]:

is_pangram("The quick brown fox jumps over the lazy dog");

The code is well commented.

But, don't do comments that just mimic the code. Good comments should show intent.

Personally, I try to avoid making "sidebar" comments. That is, I prefer a "full line" comment above the line to keep the line length under 80 chars.

The algorithm is [way] too complex. And, therefore, more error prone.

No need to scan the string to get the length and then another loop that uses the length to stop the scan. Better to iterate through the string using pointers and stop when the current character is 0 [which you do in the first loop].

Don't use "hardwired" decimal constants instead of character literals (e.g.) You're using 65 instead of 'A', etc.

You're using four loops. The entire function can be done in a single pass through the data with just one loop.

I had to refactor the code considerably. Here's the annotated code:

#include <stdio.h>
#include <stdlib.h>

int
is_pangram(const char *str)
{
    // list of characters seen
    char already_seen[26] = { 0 };

    // number of _unique_ characters seen
    int count_add_to_alph = 0;

    // loop through all input string chars
    for (int chr = *str  ;  chr != 0;  chr = *str  ) {
        int idx = -1;

        // if char is alpha, get index into "alphabet" array
        // NOTE: it _might_ be better to use (e.g.) isalpha/islower/isupper,
        // et. al. but you used char ranges in original code, so I'll continue
        // that
        do {
            // lowercase char
            if ((chr >= 'a') && (chr <= 'z')) {
                idx = chr - 'a';
                break;
            }

            // uppercase char
            if ((chr >= 'A') && (chr <= 'Z')) {
                idx = chr - 'A';
                break;
            }
        } while (0);

        // non-alpha char -- ignore
        if (idx < 0)
            continue;

        // increase _unique_ alpha count
        // NOTES:
        // (1) (! foo) is always exactly 0 or 1
        // (2) thus, (! already_seen[idx] returns 1 on the first time and 0
        //     thereafter
        count_add_to_alph  = (! already_seen[idx]);

        // mark alpha char as already seen (i.e. no longer unique)
        already_seen[idx] = 1;
    }

    // we must see all 26 letters
    return (count_add_to_alph == 26);
}