0

I am trying to obtain the first two strings from the first line of stdin using the following code.

#include <stdio.h>
#include <string.h> // for memset

int main(void) {

    #define MAX_LINE_LEN 20

    char heightWidth[MAX_LINE_LEN]; // allocate 50 chars to heightWidth
    memset(heightWidth, 0, MAX_LINE_LEN);

    fgets(heightWidth, MAX_LINE_LEN, stdin); // first line stored in heightWidth

    char str1[MAX_LINE_LEN];
    char str2[MAX_LINE_LEN];
    memset(str1, 0, MAX_LINE_LEN);
    memset(str1, 0, MAX_LINE_LEN);

    int index = 0; // stores the current char index of str array
    int strNumber = 1;
    char currChar;
    for (int i = 0; i < MAX_LINE_LEN; i++) {
        if (strNumber > 2) { // if we've read two strings, break
            break;
        }
        currChar = heightWidth[i]; // asssign current char to currChar
        if (currChar == ' ') { // if current character is a space, continue
            strNumber++; // increment strNumber
            index = 0; // reset the index
            continue;
        }
        // otherwise add it to one of our arrays
        if (strNumber == 1) {
            str1[index] = currChar;
        } else {
            str2[index] = currChar;
        }
        index++; // increment index
    }
    puts(str1);
    puts(str2);
    return 0;
}

However, when I enter three space separated strings or more, I sometimes get garbage values appended to the second string. 

asdf 234 sdf // user input
asdf // first string printed is ok
234�� // second string has garbage appended

I initially though this was because the memory allocated to those arrays still had their previous values in them (hence my use of memset to "clear" them) but adding memset didn't seem to fix my issue.

What is the problem here and how can I edit my code to obtain two strings that are space separated?

ptk
  • 6,835
  • 14
  • 45
  • 91
  • 1
    filling only ONE of two arrays and then incrementing a shared `index` looks a bit fishy to me, sure this is what you want to do? –  Apr 04 '18 at 10:09
  • I guess I used the same index since I thought I could reset it once the first was filled and didn't think it was too big of an issue. Are you saying that using a separate index would make it more readable and less confusing? – ptk Apr 04 '18 at 10:17
  • I just say your code will leave `0` bytes in your `str1` and `str2`. –  Apr 04 '18 at 10:18
  • 1
    you don't initialise `str2`, but `str1` twice. – too honest for this site Apr 04 '18 at 10:22

2 Answers2

4

You have a typo and instead of zeroing the second string you're zeroing the first one twice.

SomeWittyUsername
  • 18,025
  • 3
  • 42
  • 85
1

Ditch the memset calls (one of them has a typo as you are setting the same array twice), and write

char str1[MAX_LINE_LEN] = {0};

&c. instead. That will zero-initialise all the elements in the array. This is superior to using memset:

  1. It reduces the possibility for bugs such as the one you have.
  2. str1 and str2 are never in an uninitialised state.
  3. You don't have to repeat the length of the array or conjure an expression (e.g. sizeof idioms) to impute it.

Reference for (3): https://en.wikipedia.org/wiki/Don%27t_repeat_yourself

Bathsheba
  • 231,907
  • 34
  • 361
  • 483