Detecting a combination of characters from input

Question

Write a program that reads input up to # and reports the number of times that the sequence ei occurs.

Is this a decent code?

#include <stdio.h>
int main(void)
{
 int index = 0, combinationTimes = 0, total = 0;
 char userInput;
 char wordChar[index];
 printf("please enter your input:\n");
 while ((userInput = getchar()) != '#')
 {
 if (userInput == '\n')
 continue;
 wordChar[index] = userInput;
 index++;
 total++;
 }
 for (index = 1; index < total; index++)
 {
 if (wordChar[index] == 'i')
 {
 if (wordChar[--index] == 'e')
 {
 combinationTimes++;
 ++index;
 }
 }
 }
 printf("number of combination is: %d", combinationTimes);
 return 0;
}

• Daniel Kats: Posted my original comment as answer because spaces in comments are stupid.
• MNY: can you say why it dosen't work with this input? i tried it and i see that i have an problem there @BlackSheep
• Daniel Kats: Edited original answer to reflect suggested changes. What I suggest there is a good start, but you can definitely improve on the provided code as well.
• Daniel Kats: If you are using the code at the bottom of the answer for anything, I have revised it to fix an edge case I missed last night because it was late.

Answer 1

Here are some inputs on command line which break your code:

dan@albatross $ gcc -Wall f.c -o f
dan@albatross $ ./f
please enter your input:
bfiqwb23b r9pu3h2ru23r
9aisdbfuiasdf
adsf#asdf
#
#
#
 adsfadsfasdf
34324!
^C

I assume your program is supposed to terminate at the first #, but notice here that it does not. I have to kill the program to stop it. The shows there is a problem with one of your loops. By seeing the manipulation of the index variable in the second loop, I think it's the problem.

Code Review

First: you do not need to have the line index++ inside the loop. Use the single variable total instead. This is a minor thing.

Instead of having a nested if-statement, use an and conditional. And stop decreasing the index variable - that's what causing all those problems. Try this:

for (index = 1; index < total; index++)
 { 
 if (wordChar[index] == 'i' && wordChar[index - 1] == 'e')
 combinationTimes++;
 } 

However, that is still inefficient code, because you should be processing the input as it is given, not processing it after the last hash. Notice that you are doing length_of_input work at the end, when you could be doing constant work at each step, which is computationally less noticeable to a user. In addition, the array has length_of_input memory overhead, which seems unnecessary here. Consider writing something like this instead:

#define FALSE 0
#define TRUE 1
int main(void)
{
 int combinationTimes = 0;
 char userInput;
 short int last_e = FALSE;
 printf("please enter your input:\n");
 while ((userInput = getchar()) != '#')
 { 
 if (userInput == 'e')
 { 
 last_e = TRUE;
 } 
 else if (userInput == 'i' && last_e) 
 {
 combinationTimes++;
 last_e = FALSE;
 }
 else
 {
 //this is important, otherwise combinations like 'ite' are counted
 last_e = FALSE;
 }
 } 
 printf("Number of combos: %d\n", combinationTimes);
 return 0;
}

Here are some key features:

• I removed your array, so I have no significant memory overhead. Each char is processed and discarded, one at a time

• I remove your variables total and index

• I process the event you care about inside the first for loop, as you read the character

• I create boolean variables with compiler flags

This code also has a lot of room for improvement, but that's left as an exercise...

• Daniel Kats: Sorry, it's my first answer. I edited the original answer to make it less... mean... :(
• MNY: hah, good thing you did so. C is the first language im learning and im only learning it for 2 weeks, so you could guess positive feedbacks are more appreciate around here ;) @BlackSheep