Strange char appears in the output

Question

I'm trying to obfuscate word which is stored in string and my code sometimes works and sometimes doesn't. Here is my code:

// main function
int main(int argc, string argv[])
{
    string k, plaintext;
    int size, i = 0, key = 0;
    k = argv[1];
    size = strlen(k);
    if (argc < 2 || !isNummeric(k, size) || k < 0)
        return 1;
    else
    plaintext = GetString();
    size = strlen(plaintext);
    char ciphertext[size];
    key = atoi(k);
    while(i < size)
    {
        if (isalpha(plaintext[i]))
        {
            encipher(key, i, &ciphertext[i], plaintext);
        }
        else
        {
            ciphertext[i] = plaintext[i];
        }
        i++;
    }
    printf("%s\n",ciphertext);
}

A key is received from the user to shift each letter and I need to check whether the key is numeric value or not so I made isNummeric function to do that

bool isNummeric(string k, int size)
{
    int c=0;
    for(int i=0; i<size; i++)
    {
        if(!isdigit(k[i]))
        c++;
    }
    if(c==0)
    return true;
    return false;
}

Now to encipher I made function to shift each letter:

void encipher(int k, int i, char *pt, string plaintext)
{
    int p, c;
    if(islower(plaintext[i]))
    {
        p=plaintext[i]-'a';
        c=(p+k)%26;
        *pt=c+97;
    }
    else
    {
        p=plaintext[i]-'A';
        c=(p+k)%26;
        *pt=c+65;
    }
}

here is my output

First observation, you reference `argv[1]` before you check `argc`. What is `string`? Please show the [Minimal, Complete, and Verifiable example](http://stackoverflow.com/help/mcve) which shows what you have tried. — Weather Vane, Jul 29 '16 at 17:03
`char ciphertext[size];` is one too short, and is unterminated. Changing it to `char ciphertext[size+1] = {0};` should help. — Weather Vane, Jul 29 '16 at 17:05
For example, suppose that the secret key, k, is 13 and that the plaintext, p, is "Be sure to drink your Ovaltine!" Let’s encrypt that p with that k in order to get the ciphertext, c, by rotating each of the letters in p by 13 places, whereby: Be sure to drink your Ovaltine! becomes: Or fher gb qevax lbhe Binygvar! but some times it prints something like in the image i uploaded — Ahmed Nabil, Jul 29 '16 at 17:12
thank you i made it but why char ciphertext[size]; is too short i can't understand your point and do you mean by unterminated — Ahmed Nabil, Jul 29 '16 at 17:28
C strings end with a `'\0'` nul termimator, as even the most basic reference book will tell you. A `char` array need not do so, so there is no problem encyphering each element. But you are sending `ciphertext` to `printf` which relies on the nul terminator. The `strlen` function you used, does not include that terminator. Therefore you need to define the array one element longer, to accommodate the nul terminator. You could explicitly write this terminator after coding the array, but I dealt with it by initilialising the whole array to `0` which is the same as `'\0'` or `nul`. — Weather Vane, Jul 29 '16 at 17:34
@WeatherVane `ciphertext[size+1] = {0};` You can not write the initializers to VLA. — BLUEPIXY, Jul 29 '16 at 23:21
@BLUEPIXY ok thanks, that shows I don't use them :). Then OP should use `ciphertext[size] = '\0';` to terminate the string. — Weather Vane, Jul 29 '16 at 23:46
@WeatherVane or `ciphertext[i] = 0;` after while-loop. (Typing is less :) — BLUEPIXY, Jul 29 '16 at 23:48
'Tis curious that you subtract `'a'` but add `97`; similarly that you subtract `'A'` but add `65`. You should be self-consistent and using the character constants is better style. — Jonathan Leffler, Jul 30 '16 at 02:20
Your `isNummeric()` function — which would be more conventionally called `isNumeric()` with a single 'm', but this doesn't affect the operation — could perfectly well simply return `false` when it detects the first non-digit, and simply return `true` if all the characters are digits. The `else` after the `if (…)` condition in `main()` affects just one line; the `else` should really be eliminated altogether since everything after the `return 1;` will be executed when the `main()` didn't exit/return. — Jonathan Leffler, Jul 30 '16 at 02:24
Note that `string` is `typedef char *string;` in `"cs50.h"` (from [CS50](https://manual.cs50.net/library/)). Unfortunately, people doing the CS50 course don't routinely recognize that they need to identify this on SO — but the compilation command line from `make` does give it away. — Jonathan Leffler, Jul 30 '16 at 02:30

score 0 · Accepted Answer · answered Jul 30 '16 at 16:15

Assembling multiple comments from the question into fixed code yields the following code which seems to work:

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

static bool isNumeric(string k, int size)
{
    for (int i = 0; i < size; i++)
    {
        if (!isdigit((unsigned char)k[i]))
            return false;
    }
    return true;
}

/* There are too many arguments to this function - but it works */
static void encipher(int k, int i, char *pt, string plaintext)
{
    int p, c;
    if (islower((unsigned char)plaintext[i]))
    {
        p = plaintext[i] - 'a';
        c = (p + k) % 26;
        *pt = c + 'a';
    }
    else
    {
        p = plaintext[i] - 'A';
        c = (p + k) % 26;
        *pt = c + 'A';
    }
}

int main(int argc, string argv[])
{
    string k = argv[1];
    if (argc < 2 || !isNumeric(k, strlen(k)))
        return 1;
    string plaintext = GetString();
    int size = strlen(plaintext);
    char ciphertext[size + 1];
    int key = atoi(k);
    for (int i = 0; i < size; i++)
    {
        if (isalpha(plaintext[i]))
        {
            encipher(key, i, &ciphertext[i], plaintext);
        }
        else
        {
            ciphertext[i] = plaintext[i];
        }
    }
    ciphertext[size] = '\0';
    printf("%s\n", ciphertext);
}

The program was called csr13, and gives the following outputs:

$ csr13 4
The Quick Brown Fox Jumped Over The Lazy Dog
Xli Uymgo Fvsar Jsb Nyqtih Sziv Xli Pedc Hsk
$ csr13 22
Xli Uymgo Fvsar Jsb Nyqtih Sziv Xli Pedc Hsk
The Quick Brown Fox Jumped Over The Lazy Dog
$

A better design for the encipher function would pass the single character plus the 'key' offset and would return the encrypted character:

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

static bool isNumeric(string k, int size)
{
    for (int i = 0; i < size; i++)
    {
        if (!isdigit((unsigned char)k[i]))
            return false;
    }
    return true;
}

static int encipher(int k, int c)
{
    assert(isalpha(c));
    if (islower(c))
        return (c - 'a' + k) % 26 + 'a';
    else
        return (c - 'A' + k) % 26 + 'A';
}

int main(int argc, string argv[])
{
    string k = argv[1];
    if (argc < 2 || !isNumeric(k, strlen(k)))
        return 1;
    string plaintext = GetString();
    int size = strlen(plaintext);
    char ciphertext[size + 1];
    int key = atoi(k);
    for (int i = 0; i < size; i++)
    {
        if (isalpha(plaintext[i]))
            ciphertext[i] = encipher(key, plaintext[i]);
        else
            ciphertext[i] = plaintext[i];
    }
    ciphertext[size] = '\0';
    printf("%s\n", ciphertext);
}

Strange char appears in the output

1 Answers1