0

I think I just need a good set of eyes to look at this and tell me where I went wrong. It works for "Meet me at the park at eleven am" but fails a few of the check 50s. I believe it has something to do with me not handling the uppercase letters in the command line argument properly but i'm at a loss. I also fail the hax0r check and I have no idea why as to me it looks as it should return 1 If I give anything but an alpha character. Forgive my sloppy code and lack of notes I realize I need to start doing that and will come next pset. Thanks in advance.

https://sandbox.cs50.net/checks/214ff9f61ee5482a96eff56144254b29

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


int main(int argc, string argv[])
{
int keyy = 0;
int counter = 0;

if (argc != 2)
{
    printf("try again\n");
    return 1;
}
    string k = argv[1]; // grabs keyword from second command line argument
    int f = strlen(k); 

    for (int j = 0; j < f; j++) //looping through keyword to make sure its only alpha
    {
        if(!isalpha(k[j]))        
        {
            printf("try again\n");
            return 1;
        }
    }

    string s = GetString(); //gets plaintext string from user

    for (int i = 0, n = strlen(s); i < n; i++) // loops through plaintext until its all encrypted
    {
        keyy = k[counter % f]; // make sure the keyword loops
        if (isalpha(s[i]))
        {
            if (isupper(k[keyy]))//checks keyword letter to see if upper
            {
                int key = keyy - 'A';

                if (isupper(s[i]))
                {
                    int c = s[i] - 'A';
                    int letter = (c + key) % 26;
                    int letterr = letter + 'A';
                    printf("%c", letterr);
                }
                else
                {
                    int c = s[i] - 'a';
                    int lletter = (c + key) % 26;
                    int lletterr = lletter + 'a';
                    printf("%c", lletterr);
                }
            }
            else if (islower(k[keyy]))
            {

                int key = keyy - 'a';

                if (isupper(s[i]))
                {
                    int c = s[i] - 'A';
                    int letter = (c + key) % 26;
                    int letterr = letter + 'A';
                    printf("%c", letterr);
                }
                else
                {
                    int c = s[i] - 'a';
                    int lletter = (c + key) % 26;
                    int lletterr = lletter + 'a';
                    printf("%c", lletterr);
                }
            }
        counter++;
        }

        else
        {
            printf("%c", (char)s[i]);
        }
    }
    printf("\n");
    return 0;
}
5
  • You should try converting your key to lower cases in the upper for loop, that is what is causing errors when your key has uppercase letters. Remember the scope for any given variable is the function on which it is declared. – dLopez Aug 2 '16 at 2:06
  • Actually scope is not the problem here but rather the fact that j is only changed inside the first for loop, meaning each time you call tolower(k[j]); the value for j is always the same. – dLopez Aug 2 '16 at 2:30
  • How can I fix that? I was under the impressions that the second for loop was inside the first one therefore j would change always. Im having a hard time grasping whats happening. Thanks for your help! – Taylor Adam Aug 2 '16 at 2:58
  • Sloppy code is the bane of all programmers. You need to develop better code writing skills; it just can't be stressed enough. It will help both you and others understand what in the empire of dirt your code is attempting to do! – NullityNull Aug 2 '16 at 3:28
  • Appreciate the criticism and I completely agree, I'm very new and have realised now I need to start adding comments and using more descriptive names, because even I get lost looking at my own code. – Taylor Adam Aug 2 '16 at 3:52
3

Firstly there are two compilation errors, that will prevent your code from even compiling (making me wonder whether you submitted a different version of your program to the check50).

  1. The first one is the parentheses missing from this line in your second for loop

    if isalpha(s[i])
    

    I added them in your posted code to make it easier to find.

  2. Inside your second for loop, j is undefined, which means it's out of scope. What that means is that your code doesn't have access to this variable any more, and that's because the scope of a variable is between it's first parent curly braces.

    for (int j = 0, f = strlen(k); j < f; j++)  // in this line j is declared and comes into scope
    {
        if(!isalpha(s[j]))
        {
            printf("try again\n");
            return 1;
        }
    }
    // at this line j goes out of scope, you can't use it anymore
    

If you don't fix these problems we can't help you find the problem with your code, because we don't know how you intended to use j. Should it restart from 0 or continue with the last value? Fix them and ask again. Till then, happy bug hunting. :)


Edit after change in question

Your problem know, stems from the fact that you have used badly named variables (keyy, k and key).

Take a look at that:

string k = argv[1];
// ...
keyy = k[counter % f];
// ...
if (isupper(k[keyy]))
// ...
  • k is the crypto-key used to encrypt the plaintext, and it's a string
  • keyy is assigned a char from the crypto-key (even if stored as an int, we can safely say it can be converted back to a char)
  • and then you check for isupper(k[keyy]), but keyy is a char, not an index for the string k.

And that's probably the reason your code gives wrong output.

5
  • 1
    Thank you, I edited my post with my current code, I basically fixed a few small things and realised I needed separate if/else loops for lower and uppercase letters in the keyword. Also, thank you for those edits and the explanation on why J was not working how I thought it should! It only fails BARFOO and BaRFoo now in check 50. I feel like it may have something to do with the counter? I honestly don't know – Taylor Adam Aug 2 '16 at 7:07
  • 1
    ChrisG - Interesting thing about j. I looked at the original code and it did compile because the closing brace in the for( j ) loop was missing/misplaced, turning them into nested for loops. But your point is entirely correct. ;-) – Cliff B Aug 2 '16 at 7:17
  • I should have known better, but that was the code I saw and I judged (maybe incorrectly) based on it. – ChrisG Aug 2 '16 at 7:25
  • ChrisG, you were exactly right to analyze the latest version. well done. ;-) – Cliff B Aug 2 '16 at 8:39
  • Thank you so much @ChrisG, I wouldn't have figured it out without you're help. I changed isupper(k[keyy]) to isupper(k[counter % f]) and it passes all check50s now :)! – Taylor Adam Aug 2 '16 at 17:09

You must log in to answer this question.

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