# Detecting combination of characters from input

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

My code is:

#include <stdio.h>

int main(void)

{
int index = 0, combinationTimes = 0, total = 0;
char userInput;
char wordChar[index];

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;
}


Is this a decent code?

-
Posted my original comment as answer because spaces in comments are stupid. –  BlackSheep Jan 29 '13 at 2:21
can you say why it dosen't work with this input? i tried it and i see that i have an problem there @BlackSheep –  Nir Jan 29 '13 at 2:40
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. –  BlackSheep Jan 29 '13 at 3:31
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. –  BlackSheep Jan 29 '13 at 13:24

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

dan@albatross  $gcc -Wall f.c -o f dan@albatross$ ./f
bfiqwb23b r9pu3h2ru23r
9aisdbfuiasdf
#
#
#
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;

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...

-
thank you for the detailed answer @BlackSheep –  Nir Jan 29 '13 at 10:59
Sorry, it's my first answer. I edited the original answer to make it less... mean... :( –  BlackSheep Jan 29 '13 at 13:19
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 –  Nir Jan 29 '13 at 13:30