7
\$\begingroup\$

I'm writing a C program to reserve seats in a movie theater. I am validating the selection of seats:

const int TOTAL_ROWS = 10;            // 1 to 26 -> 'A' to 'Z'
const int SEATS_PER_ROW = 5;          // 1 to 9
int seats[TOTAL_ROWS][SEATS_PER_ROW]; // reserved seats, reserved seat == 1
char codes[TOTAL_SEATS][2];           // selected seats, e.g. A1, ..., Z9
...
for (i = 0; i < required_seats; i++) {
    printf("Enter the code for the seat: %i: ", i + 1);
    scanf("%s", codes[i]);
    if (codes[i][0] >= 'A' && 
        codes[i][0] <= 'A' + TOTAL_ROWS - 1 && 
        codes[i][1] >= '1' && 
        codes[i][1] <= '1' + SEATS_PER_ROW - 1) {

        if (seats[codes[i][0] - 'A'][codes[i][1] - '1'] == 1) {
            printf("Reserved seat. Select other.\n");
            i--;
        }
        for (j = 0; j < i; j++) {
            if (codes[j][0] == codes[i][0] && codes[j][1] == codes[i][1]) {
                printf("Repeated selection. Select other.\n");
                i--;
                break;
            }
        }
    } else {

        printf("Invalid code for seat.\n");
        i--;
    }
}

This code works fine. However, how can I refactor the code for this loop? In my opinion, using i-- does not look professional.


* By the way, you may want to see the full code —originally, in Spanish.

\$\endgroup\$
3
  • \$\begingroup\$ this line: 'scanf("%s", codes[i]);' allows the user to input more than 2 characters. That will result in a buffer overflow = undefined behaviour and can lead to a seg fault event. suggest add max length modifier to the %s format specifier suggest using isdigit() on each byte before using those bytes in any calculations \$\endgroup\$ – user3629249 Mar 14 '15 at 20:39
  • 1
    \$\begingroup\$ this code would make it a real problem to find some 'not reserved' seats in a crowded theatre. suggest modifying to display available seats before asking user to select specific seats \$\endgroup\$ – user3629249 Mar 14 '15 at 20:45
  • \$\begingroup\$ Thanks for your thoughts. Actually this is a homework and a console's application. \$\endgroup\$ – Paul Vargas Mar 14 '15 at 21:09
6
\$\begingroup\$

I/O

You want to read 2 characters although you use scanf("%s", codes[i]) this can have the effect you are overwriting the end of the buffer when you read scanf("%s", codes[required_seats-1]); since there is always an \0 added to your two characters.

Either increase the buffer, change the format specifier or use an alternative method of reading from keyboard e.g. fgets() followed by sscanf() or simply read the characters one by one using fgetc(). The latter method has the advantage that you could prevent the user from entering invalid characters while he types.

You may also want to convert the characters to upper case just to be a bit more flexible towards the user (toupper())

structure

I think the code could do with some more functions to clarify the purpose.

E.g.

int isCodeValid(char* code)
{
  return code[0] >= 'A' && code[0] <= 'A' + TOTAL_ROWS - 1 && 
     code[1] >= '1' &&  code[1] <= '1' + SEATS_PER_ROW - 1;
}

int isRepeatedSelection(char* codes);
...

loop

What other suggested and as you mentioned already decrementing i in the for loop may be a bit confusing for the reader (well it was for me at least), having a do..while loop feels more natural here where the while condition is false until user enters correct code (or changes his mind and gives up).

\$\endgroup\$
4
\$\begingroup\$

For the refactoring, I suggest that you should wrap the logic of the for-loop into a do-while loop.

Something like this:

int inputOk = 0;
for (i = 0; i < required_seats; i++) {
    inputOk = 0;
    do {
       printf("Enter the code for the seat: %i: ", i + 1);
       scanf("%s", codes[i]);
       if (codes[i][0] >= 'A' && 
           codes[i][0] <= 'A' + TOTAL_ROWS - 1 && 
           codes[i][1] >= '1' && 
           codes[i][1] <= '1' + SEATS_PER_ROW - 1) {
           // ...
           // if there were no problems
           inputOk = 1;
       }
   } while (!inputOk)
 }

Another remark: since you are reading char's and not strings, I suggest using scanf("%c", codes[i]) (so replace %s with %c). Or you can also use getchar().

\$\endgroup\$
3
\$\begingroup\$
  1. Modifying the loop variable i inside the for loop generally falls under the category "Don't do this" because it makes it really hard to see what is going on and can easily lead to very hard to find bugs. Attilio's answer already shows how to refactor this. I just wanted to mention it explicitly because it's a bad habit you should dump sooner than later.

  2. Another comment mentioned that you read in a string with %s but the location you store it at can only hold two characters. So if the user types in a longer string then you open yourself up for all kinds of memory corruptions which will lead to very strange program behaviour. This can be fixed by limiting the length of the string to read: scanf("%2c", codes[i]) (you should use %c because %s will automatically add a terminating \0 byte).

\$\endgroup\$
3
+50
\$\begingroup\$

To answer the question you asked: You want to repeat asking the user for a seat number until he enters a valid seat number. In some languages (e.g. Pascal), there's actually a control-flow primitive called repeat ... until. In C, we spell it do ... while. Sample code follows, far below.


Some other nitpicks first:

const int TOTAL_ROWS = 10;            // 1 to 26 -> 'A' to 'Z'

In C++, this does what you want (although modern idiom would have you write constexpr instead of const). In C, this defines a const-qualified global variable; the idiomatic way to define actual compile-time constants (capable of being constant-propagated and so on) is to use macros or unnamed enums.

#define TOTAL_ROWS 10
// or
enum { TOTAL_ROWS = 10 };

The use of ALL_CAPS is traditionally reserved for macros; for a plain old const variable I would use total_rows, and for an enum I might use any of total_rows, TotalRows, or TOTAL_ROWS depending on the rest of the codebase.

int seats[TOTAL_ROWS][SEATS_PER_ROW];

This is not valid C, last time I checked. You'll have to make TOTAL_ROWS and SEATS_PER_ROW enums or macros, not variables, to get this to compile with a pedantic compiler. (Unless this is inside a function scope, in which case what you've got here is a C99 variable-length array.)

if (codes[i][0] >= 'A' && 
    codes[i][0] <= 'A' + TOTAL_ROWS - 1 && 
    codes[i][1] >= '1' && 
    codes[i][1] <= '1' + SEATS_PER_ROW - 1) {

C has a strong bias in favor of half-open ranges ("at least A but strictly less than B"). Also, it's easier to read range comparisons if you organize them in the usual mathematical way, e.g. 1 <= x < 10. C doesn't support "chaining" comparison operators in that way, but we can still pretend like it does, for the sake of readability:

if ('A' <= codes[i][0] && codes[i][0] < 'A' + TOTAL_ROWS && 
    '1' <= codes[i][1] && codes[i][1] < '1' + SEATS_PER_ROW) {

Put it all together and we get something like

enum { TOTAL_ROWS = 10, SEATS_PER_ROW = 5 };
bool seats_taken[TOTAL_ROWS][SEATS_PER_ROW]; // reserved seats
for (i = 0; i < required_seats; i++) {
    printf("Enter the code for seat #%d: ", i + 1);
    char row = '\0';
    char seat = '\0';
    bool selected_a_seat = false;
    do {
        if (scanf(" %c%c%*[^\n]", &row, &seat) < 2) {
            // it looks like we've run out of user input
            puts("Goodbye...");
            return;
        } else if (!('A' <= row && row < 'A' + TOTAL_ROWS &&
                     '1' <= seat && seat < '1' + SEATS_PER_ROW)) {
            puts("Invalid seat number; please select again");
        } else if (seats_taken[row-'A'][seat-'1']) {
            printf("Seat %c%c is already taken; please select again\n", row, seat);
        } else {
            seats_taken[row-'A'][seat-'1'] = true;
            selected_a_seat = true;
        }
    } while (!selected_a_seat);
}

The construct

    bool selected_a_seat = false;
    do { ... } while (!selected_a_seat);

can also be expressed as

    for (bool selected_a_seat = false; !selected_a_seat; ) { ... }

if you feel more comfortable with that.

\$\endgroup\$
1
  • \$\begingroup\$ +1 You have made a good analysis. :-) In fact, I use the following flags: -std=c99 -pedantic -Wall -Wshadow -Wpointer-arith -Wcast-qual -Wstrict-prototypes -Wmissing-prototypes -finput-charset=UTF-8 -fexec-charset=CP850. The teacher uses Windows. This last flag shows correctly áéíóúñ. :) \$\endgroup\$ – Paul Vargas May 12 '15 at 15:15
2
\$\begingroup\$

I cannot imagine where the value for required_seats is coming from. I think that a better solution would be to ask the user when he has finished inserting the seats. This way you would also get a better code, in my opinion.

This is a possible rewriting (explanation follows):

seat_count = 0;
char buffer[10];
for (;;) {
    printf("Enter code for seat n. %d (leave empty to finish):", seat_count + 1);
    fgets(buffer, sizeof(buffer), stdin);
    int row = buffer[0]-'A';
    int col = buffer[1]-'0';
    if (buffer[0] == '\n') {  // empty
        break;
    }
    if (row < 0 || row >= TOTAL_ROWS || col < 0 || col >= SEATS_PER_ROW) {
        printf("Invalid code for seat.\n");
        continue;
    }
    if (seats[row][col]) {
        printf("Seat is already occupied, retry.\n");
        continue;
    }
    seat[row][col] = 1;
    codes[seat_count][0] = buffer[0];
    codes[seat_count][1] = buffer[1];
    seat_count++;
}

Comments:

  • don't be afraid to use break; and continue; statements to handle exceptions. You can avoid unnecessary nesting of the code and keep the condition checks close to the action performed.

  • you are not enumerating items, you are counting the input by the user. So an empty for loop is more appropriate in my opinion.

  • notice how the use of the variables row and col renders the code more readable and compact.

  • be sensitive to buffer overflows: scanf("%s", ...) must be never used.

  • take into consideration the fact that a C-string of length 2 occupies 3 bytes since it is '\0'-terminated. So, if you want to store your values in two bytes (which is quite natural), you need a buffer to get the input from the user. The buffer could also be useful if you later want to add some kind of cleaning on the input.

\$\endgroup\$
1
  • \$\begingroup\$ I have added a link to the full code in the post (if you have any additional suggestions). \$\endgroup\$ – Paul Vargas May 7 '15 at 15:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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