0

I am trying to solve the substitution problem on CS50 course. After I finished with it, I tried to compile but it does not work correctly. It always returns the ./sustitiuion key part even though I give true key. Can you see what is wrong?

The part while I check the key seems incorrect, and I am nearly sure that there shouldn't be problem because I checked on the Internet for the same part and it is nearly the same.

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

int main(int argc, string argv[])
{
    if (argc != 2)
    {
        printf("Usage: ./substituion key \n");
        return 1;
    }
    //validate the key!

    string key = argv[1];

    for (int i = 0; i < strlen(key); i++)
    {
        if (!isalpha(key[i]))
        {
            printf("Usage: ./substitution key\n");
        }
    }

    for (int j = 0; j < strlen(key); j++)
    {
        for (int k = 0; k < strlen(key); k++)
        {
            if (toupper(key[j]) == toupper(key[k]))
            {
                printf("Usage: ./substitution key \n");
                return 1;
            }
        }
    }

    if (strlen(argv[1]) != 26)
    {
        printf("Usage: ./substituion key \n");
        return 1;
    }

Why are this course's problems so hard?

chqrlie
  • 131,814
  • 10
  • 121
  • 189
learner_engineer7
  • 11
  • 1
    Welcome to Stack Overflow. Please read [the help pages](http://stackoverflow.com/help), take the SO [tour], and read [ask]. Also please read [how to write the "perfect" question](https://codeblog.jonskeet.uk/2010/08/29/writing-the-perfect-question/), especially its [checklist](https://codeblog.jonskeet.uk/2012/11/24/stack-overflow-question-checklist/). – Some programmer dude Jul 22 '23 at 10:02
  • As for your problem, it might be a good time to learn how to [*debug*](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/) your code. Knowing how to debug your programs, including using a [*debugger*](https://stackoverflow.com/questions/25385173/what-is-a-debugger-and-how-can-it-help-me-diagnose-problems) to step though the code line by line while monitoring variables and their values, is a crucial skill for anyone serious about programming. – Some programmer dude Jul 22 '23 at 10:03
  • Have you tried running your code line-by-line in a debugger while monitoring the control flow and the values of all variables, in order to determine in which line your program stops behaving as intended? [In week 2 of CS50, you were taught how to use a debugger.](https://video.cs50.io/XmYnsO7iSI8?screen=5YGV1hcM_MY&start=2294&end=3777). Week 2 also has a few short videos on debugging. – Andreas Wenzel Jul 22 '23 at 10:10
  • `"why this course's problems are hard?"` -- Because you selected to do a hard exercise. You were given [several exercises to choose from](https://cs50.harvard.edu/x/2023/psets/2/). Some are marked "if feeling less comfortable", others are marked "if feeling more comfortable". You chose an exercise that is marked "if feeling more comfortable", i.e. "hard". – Andreas Wenzel Jul 22 '23 at 10:13
  • 1
    Make your three identical error messages slightly different, so you know *which* one is failing. Then when you look why, can you see that `if(toupper(key[j]) == toupper(key[k]))` should be `if(j != k && toupper(key[j]) == toupper(key[k]))`? Obviously you'll get an apparent duplication if it's the same position. – Weather Vane Jul 22 '23 at 10:39
  • ... better is to rewrite the inner loop as `for(int k = j + 1; k < strlen(key); k++)`. – Weather Vane Jul 22 '23 at 10:45
  • Questions seeking debugging help should generally provide a [mre] of the problem, which includes the exact input required to reproduce the problem. Without the proper input, it may be impossible for other people to reproduce the problem. – Andreas Wenzel Jul 22 '23 at 10:53

1 Answers1

1

The problem is in the code that tests for duplicate letters: you should only check if the letters are different if the index j and k are different. An efficient approach suggested by @chux is to iterate k from j + 1 to the length of the string.

Here is a modified loop:

    for (int j = 0; j < strlen(key); j++)
    {
        for (int k = j + 1; k < strlen(key); k++)
        {
            if (toupper(key[j]) == toupper(key[k]))
            {
                printf("Usage: ./substitution key \n");
                return 1;
            }
        }
    }

More generally, you should compute the string length once and store it into a variable to avoid computing it repeatedly at each loop iteration. The compiler might be able to optimize these calls to the strlen function, but it is bad style in C to use j < strlen(key) as a loop test.

Here is a modified version:

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

int main(int argc, string argv[]) {
    if (argc != 2) {
        fprintf(stderr, "Usage: ./substitution key\n");
        return 1;
    }
    //validate the key!

    string key = argv[1];
    int len = strlen(key);

    if (len != 26) {
        fprintf(stderr, "substitution: key must be 26 letters exactly\n");
        return 1;
    }
    for (int i = 0; i < len; i++) {
        if (!isalpha((unsigned char)key[i])) {
            fprintf(stderr, "substitution: key must be letters only\n");
            return 1;
        }
    }

    for (int j = 0; j < len; j++) {
        for (int k = j + 1; k < len; k++) {
            if (toupper((unsigned char)key[j]) == toupper((unsigned char)key[k])) {
                fprintf(stderr, "substitution: duplicate letter '%c'\n", key[j]);
                return 1;
            }
        }
    }
chqrlie
  • 131,814
  • 10
  • 121
  • 189
  • 1
    Note that `for (int j = 0; j < len; j++) { for (int k = 0; k < len; k++) {` iterates `len*len` times. Only about `len*len/2` needed with `for (int k = j + 1; k < len; k++) {` and then `j != k` test no longer needed. – chux - Reinstate Monica Jul 22 '23 at 11:52
  • @chux-ReinstateMonica: absolutely! Answer amended. – chqrlie Jul 22 '23 at 13:04