0

I'm having different results with check50 with the same code. The code works normally if I type. Sometimes all the results show correct

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

int main(int argc, string argv[])
{

const int N = 26;
string alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
char letters[N];

if (argc == 2)
{
    if (strlen(argv[1]) == 26)
    {

        int count = 0;
        for (int i = 0, len = strlen(argv[1]); i < len; i++)
        {
            
            
            argv[1][i] = toupper(argv[1][i]);
            if (!isalpha(argv[1][i]))
            {
                
                printf("Key must contain only letters.\n");
                return 1;
                
            }
            
            for (int j = 0; j < N; j++)
            {
                if (argv[1][i] == letters[j])
                {
                    
                    printf("Key must not have repeated letters\n");
                    return 1;
                    
                }
            }
        
            letters[i] = argv[1][i];
            
            
        }
        
        string p = get_string("plaintext: ");
        int l = strlen(p);
        char c[l + 1];
        
        for (int i = 0, len = strlen(p); i < len; i++)
        {
            if (isupper(p[i]))
            {

                for (int j = 0; j < 26; j++)
                {
                    if (p[i] == alphabet[j])
                    {
                        c[i] = argv[1][j];
                        break;
                    }
                
                }
        
            }
        
            else if (islower(p[i]))
            {

                for (int j = 0; j < 26; j++)
                {
                    if (p[i] == tolower(alphabet[j]))
                    {
                        c[i] = tolower(argv[1][j]);
                        break;
                    }
                
                }
        
            }
        
            else
            {
            
                c[i] = p[i];
            
            }
        }
        
        c[l] = '\0';
        printf("ciphertext: %s\n", c);

    }

    else if (strlen(argv[1]) != 26)
    {

        printf("Key must contain 26 characters.\n");
        return 1;

    }

}

else
{

    printf("Usage: ./substitution key\n");
    return 1;

}

}
3
2

It's a subtle problem,so subtle that I wasn't able to find it. However, I believe that I understand where it lies.

There's an old saying in programming, "KISS, Keep It Simple, Stupid!" I remind myself of this on just about every project I work on. It means that when the code gets too complex, its more likely to have bugs in it. I think that's what's at work here.

This code intermixes the tasks of validating the program parameters, getting user input and producing the encoded result. It's too convoluted and complex.

Code should, whenever possible, do one thing at a time and one thing only. In this case, it should validate the input parameter at the beginning, and nothing else. Then, after that task is complete, get the user input. Finally, encode the text. These three steps should be totally independent of each other. They should not overlap and should not be part of if or else statements related to other steps.

It's my opinion that wherever this bug lies, it's related to the complexity of the code and intermix of the various steps involved. My suggestion is to restructure the code.

I also suspect an additional problem with this line:

if (argv[1][i] == letters[j])

[EDIT: This actually is the problem. As is the case with most uninitialized vars, it leads to unpredictable results. Sometimes this test evaluates true, other times false. check50 is flagging intermittent errors.]

On the first pass through this loop, letters[0] has yet to be initialized. It may be causing some of the issues.

Programming tip: else statements aren't always required. If the code in an if statement code block will terminate a program, an else statement is probably not needed. The basic test is this. When a true if statement terminates a program, and a false if statement means that the program continues, then an else statement is not required. Just continue with whatever code needs to follow.

If anyone else figures out exactly what's happening with this one, please post it. I'm just not seeing it today.

2
  • 1
    Exactly what you said. letters is not initialized, so doing a comparison is undefined behaviour. The memory is 'garbage' so sometimes it actually finds a match and ends the program, which check50 then reports as output being empty.
    – curiouskiwi
    Jan 31 '21 at 4:10
  • thank you very much
    – Marcelo
    Jan 31 '21 at 23:16

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .