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.

Today I solved a question from Google Code Jam. A program that mimics a cell phone keypad messaging.

Here is the program, with goto statement, I know it is unstructured style to use goto, but I still did, because it improved speed.

#include <stdio.h>

int main()
{
    char c, c1 = NULL;
    int top;

    top:
    while((c = getchar()) != '\n')
    {
        if (c == ' ')printf("0");

        if(c == 'a' || c == 'b' || c == 'c')
        {
           if(c1 == 'a' || c1 == 'b' || c1 == 'c')
           printf(" ");
          if(c == 'a') printf("2");
          if(c == 'b') printf("22");
          if(c == 'c') printf("222");
           c1 = c;
            goto top;

        }


         if(c == 'd' || c == 'e' || c == 'f')
        {
          if(c1 == 'd' || c1 == 'e' || c1 == 'f')
           printf(" ");
          if(c == 'd') printf("3");
          if(c == 'e') printf("33");
          if(c == 'f') printf("333");
           c1 = c;
            goto top;


        }

         if(c == 'g' || c == 'h' || c == 'i')
        {
            if(c1 == 'g' || c1 == 'h' || c1 == 'i')
           printf(" ");

          if(c == 'g') printf("4");
          if(c == 'h') printf("44");
          if(c == 'i') printf("444");
           c1 = c;
            goto top;

        }



         if(c == 'j' || c == 'k' || c == 'l')
        {
            if(c1 == 'j' || c1 == 'k' || c1 == 'l')
           printf(" ");

          if(c == 'j') printf("5");
          if(c == 'k') printf("55");
          if(c == 'l') printf("555");
           c1 = c;
            goto top;


        }

         if(c == 'm' || c == 'n' || c == 'o')
        {
            if(c1 == 'm' || c1 == 'n' || c1 == 'o')
           printf(" ");

          if(c == 'm') printf("6");
          if(c == 'n') printf("66");
          if(c == 'o') printf("666");
           c1 = c;
            goto top;


        }

          if(c == 'p' || c == 'q' || c == 'r' || c == 's')
        {
            if(c1 == 'p' || c1 == 'q' || c1 == 'r' || c1 == 's')
           printf(" ");

          if(c == 'p') printf("7");
          if(c == 'q') printf("77");
          if(c == 'r') printf("777");
          if(c == 's') printf("7777");
           c1 = c;
            goto top;

        }

          if(c == 't' || c == 'u' || c == 'v')
        {
            if(c1 == 't' || c1 == 'u' || c1 == 'v')
           printf(" ");

          if(c == 't') printf("8");
          if(c == 'u') printf("88");
          if(c == 'v') printf("888");
           c1 = c;
           goto top;

        }

          if(c == 'w' || c == 'x' || c == 'y' || c == 'z')
        {
            if(c1 == 'w' || c1 == 'x' || c1 == 'y' || c1 == 'z')
           printf(" ");
          if(c == 'w') printf("9");
          if(c == 'x') printf("99");
          if(c == 'y') printf("999");
          if(c == 'z') printf("9999");
           c1 = c;
          goto top;

        }
    }
}

The above program without goto.

#include <stdio.h>

int main()
{
    char c, c1 = NULL;

    while((c = getchar()) != '\n')
    {
        if (c == ' ')printf("0");

        if(c == 'a' || c == 'b' || c == 'c')
        {
           if(c1 == 'a' || c1 == 'b' || c1 == 'c')
           printf(" ");
          if(c == 'a') printf("2");
          if(c == 'b') printf("22");
          if(c == 'c') printf("222");

        }


         if(c == 'd' || c == 'e' || c == 'f')
        {
          if(c1 == 'd' || c1 == 'e' || c1 == 'f')
           printf(" ");
          if(c == 'd') printf("3");
          if(c == 'e') printf("33");
          if(c == 'f') printf("333");


        }

         if(c == 'g' || c == 'h' || c == 'i')
        {
            if(c1 == 'g' || c1 == 'h' || c1 == 'i')
           printf(" ");

          if(c == 'g') printf("4");
          if(c == 'h') printf("44");
          if(c == 'i') printf("444");

        }



         if(c == 'j' || c == 'k' || c == 'l')
        {
            if(c1 == 'j' || c1 == 'k' || c1 == 'l')
           printf(" ");

          if(c == 'j') printf("5");
          if(c == 'k') printf("55");
          if(c == 'l') printf("555");


        }

         if(c == 'm' || c == 'n' || c == 'o')
        {
            if(c1 == 'm' || c1 == 'n' || c1 == 'o')
           printf(" ");

          if(c == 'm') printf("6");
          if(c == 'n') printf("66");
          if(c == 'o') printf("666");


        }

          if(c == 'p' || c == 'q' || c == 'r' || c == 's')
        {
            if(c1 == 'p' || c1 == 'q' || c1 == 'r' || c1 == 's')
           printf(" ");

          if(c == 'p') printf("7");
          if(c == 'q') printf("77");
          if(c == 'r') printf("777");
          if(c == 's') printf("7777");

        }

          if(c == 't' || c == 'u' || c == 'v')
        {
            if(c1 == 't' || c1 == 'u' || c1 == 'v')
           printf(" ");

          if(c == 't') printf("8");
          if(c == 'u') printf("88");
          if(c == 'v') printf("888");


        }

          if(c == 'w' || c == 'x' || c == 'y' || c == 'z')
        {
            if(c1 == 'w' || c1 == 'x' || c1 == 'y' || c1 == 'z')
           printf(" ");
          if(c == 'w') printf("9");
          if(c == 'x') printf("99");
          if(c == 'y') printf("999");
          if(c == 'z') printf("9999");


        }

        c1 = c;
    }
}

Which one is better?, How to come to a realization that X program is better compared to Y. Also how to overcome this addiction to writing code and testing it. Its my 4th month in programming and computer science and I am not studying any computer science. I am always writing programs and reading C books. What should be my next step after completing KN Kings "C programming- A modern approach".

By the way I did not use any Derived data typed and poiters because I did not find it useful here.

share|improve this question
add comment

2 Answers

I didn't write any C in the last few years but I think you could use continue instead of goto in the first version.

share|improve this answer
2  
continue looks correct –  acidzombie24 Jan 12 '13 at 19:51
add comment

Because your outer-layer tests are mutually exclusive (ie. only one can occur), you should use else rather than goto or continue. The else is used when the condition for which if tests is false:

    if (c == ' ') {
        printf("0");
    }
    else if (c == 'a' || c == 'b' || c == 'c')
    {
        if(c1 == 'a' || c1 == 'b' || c1 == 'c')
            printf(" ");
        if(c == 'a') printf("2");
        else if(c == 'b') printf("22");
        else if(c == 'c') printf("222");
        c1 = c;
    }
    else if(c == 'd' || c == 'e' || c == 'f')
    { ... }
    else if ...
etc

note also that c and c1 should be declared int and that c1 = NULL is wrong (as NULL is declared as (void *) 0; use c1 = '\0'.

Moreover, turn more warnings ON in your compiler.

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