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: mimicking a cell phone keypad messaging.

Here is the program, with goto statements. I know it is an 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 can I come to a realization that X program is better compared to Y?

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

share|improve this question

3 Answers 3

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

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

Terminating when getchar() encounters \n is poor practice. If the program encounters EOF before Newline, then it will enter an infinite loop.

Considering that you only support lowercase, it would be nice if you folded the input to lowercase.

That is one long, uninspired while-loop, full of cut-and-paste code. I recommend a completely different approach. Defining a print_t9() function makes the code more meaningful and reusable.

#include <ctype.h>
#include <stdbool.h>
#include <stdio.h>
#include <string.h>

/**
 * Determines the key and the number of repetitions of that key to type
 * character c in T9.  If c is a supported character, returns true.
 * If c is unsupported, returns false, and the contents of *key and *reps
 * are undefined.
 */
static _Bool t9_lookup(char c, int *key, int *reps) {
    static const char t9_table[][5] = {
                " ",            /*   0   */
        "",     "abc", "def",   /* 1 2 3 */
        "ghi",  "jkl", "mno",   /* 4 5 6 */
        "pqrs", "tuv", "wxyz"   /* 7 8 9 */
    };

    c = tolower(c);
    for (*key = 0; *key < sizeof(t9_table); (*key)++) {
        char *p;
        if ((p = strchr(t9_table[*key], c))) {
            if (reps) {
                *reps = p - t9_table[*key] + 1;
            }
            return true;
        }
    }
    return false;
}

/**
 * If c is one of the supported characters, prints the T9 representation and
 * returns true.  Otherwise, returns false.  If the T9 representation uses the
 * same key as the previous character, a space is printed as a separator.
 */
_Bool print_t9(FILE *output, char c, char prev_c) {
    int key, reps, prev_key;
    if (!t9_lookup(c, &key, &reps)) return false;

    if (t9_lookup(prev_c, &prev_key, NULL) && key == prev_key) {
        fprintf(output, " ");
    }
    while (reps--) {
        fprintf(output, "%d", key);
    }
    return true;
}

int main() {
    for (char prev_c = '\0', c; EOF != (c = getchar()); prev_c = c) {
        print_t9(stdout, c, prev_c) || putchar(c);
    }
    return 0;
}

You should probably support digits as well, to be typed with one additional keypress. All it would take to support that enhancement is a simple change:

    static const char t9_table[][6] = {
                 " 0",
        "1",     "abc2", "def3",
        "ghi4",  "jkl5", "mno6",
        "pqrs7", "tuv8", "wxyz9"
    };
share|improve this answer

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.