Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My task is:

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

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

Is this a decent code?

share|improve this question
    
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
add comment

1 Answer

up vote 3 down vote accepted

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

share|improve this answer
    
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
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.