5
\$\begingroup\$

Challenge

Swap the numbers surrounding the words in sentences.

Specifications

  1. The first argument is a path to a file.
  2. The file contains multiple lines.
  3. Each line is a test case represented by a sentence.
  4. Each sentence begins and ends with a number.
  5. Print out the sentence obtained by swapping the numbers surrounding each word.

Constraints

  1. The suffix and the prefix of each word may be equal.
  2. Sentences are from 1 to 17 words long.
  3. The number of test cases is 40.
  4. All characters are ASCII.
  5. The numbers are single digit positive intgers 0-9.

Sample Input

4Always0 5look8 4on9 7the2 4bright8 9side7 3of8 5life5
5Nobody5 7expects3 5the4 6Spanish4 9inquisition0

Sample Output

0Always4 8look5 9on4 2the7 8bright4 7side9 8of3 5life5
5Nobody5 3expects7 4the5 4Spanish6 0inquisition9

Source

My solution

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

#define LINE_BUFFER 256

int fileExists(char *filename) {
    FILE *file = fopen(filename, "r");
    if (file != NULL) {
        fclose(file);
    }
    return file != NULL;
}

void swapNumbers(char line[LINE_BUFFER]) {
    int temp = 0;
    int position = 0;
    int swapped = 0;

    for (int i = 0; i < LINE_BUFFER; i++) {
        if (isdigit(line[i])) {
            if (!swapped) {
                position = i;
                temp = line[i];
                swapped = 1;
            } else {
                line[position] = line[i];
                line[i] = temp;
                swapped = 0;
            }
        }
    }
}

int main(int argc, char *args[]) {
    if (argc < 2) {
        puts("File path not provided.");
        return 1;
    }

    if (argc > 2) {
        puts("Excessive arguments, only the first will be considered.");
    }

    if (!fileExists(args[1])) {
        puts("Could not access file / file not found.");
        return 1;
    }

    FILE *file = fopen(args[1], "r");
    char line[LINE_BUFFER];

    while (fgets(line, LINE_BUFFER, file)) {
        swapNumbers(line);
        printf("%s", line);
    }

    fclose(file);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Hmmmm fclose(file); } return file != NULL; uses the pointer value file after the fclose(). I suspect this is generally OK, but smells of potential UB: using a pointer after it is no longer valid. \$\endgroup\$ – chux Sep 5 '16 at 3:57
6
\$\begingroup\$
    if (!fileExists(args[1])) {
        puts("Could not access file / file not found.");
        return 1;
    }

    FILE *file = fopen(args[1], "r");

This seems more complicated than necessary.

    FILE *file = fopen(args[1], "r");
    if (NULL == file) {
        puts("Could not access file / file not found.");
        return 1;
    }

This has the exact same effect as fileExists but it leaves you with an open file handle. The other method tested that it could open a file and then assumed that the second open worked. So it's actually slightly less safe, as well as more code.

You might argue that it would be extremely rare for the first open to succeed and the second one to fail. I agree with you. But rare is not never. Thus, "slightly" less safe.

    int temp = 0;
    int position = 0;
    int swapped = 0;

    for (int i = 0; i < LINE_BUFFER; i++) {
        if (isdigit(line[i])) {
            if (!swapped) {
                position = i;
                temp = line[i];
                swapped = 1;
            } else {
                line[position] = line[i];
                line[i] = temp;
                swapped = 0;
            }
        }
    }

I don't like using negative test conditions. I'd normally fix this by switching the if and else blocks. In this case though, the current order makes more sense.

My second thought was that the name was wrong. So I was going to change the name to needs_swapped or similar. But then I realized that if I changed the name, the current form would be correct. The name swapped is actually backwards. When swapped is true, then you haven't swapped but need to do so. So my third thought was to simply swap the meaning.

    int temp = 0;
    int position = 0;
    int swapped = 1;

    for (int i = 0; i < LINE_BUFFER; i++) {
        if (isdigit(line[i])) {
            if (swapped) {
                position = i;
                temp = line[i];
                swapped = 0;
            } else {
                line[position] = line[i];
                line[i] = temp;
                swapped = 1;
            }
        }
    }

Now I have the else as the negative of the condition, which I like. And we've also made it so that when swapped is 1 (true in C parlance), we've just swapped. When swapped is 0 (false in C), we're ready to swap.

But I actually think that there is a more idiomatic way to do this in C. You are saving an index (position), a value (temp), and a Boolean (swapped). But you actually only need to save the location. Consider

    char *location = NULL;
    for (int i = 0; i < LINE_BUFFER; i++) {
        if (isdigit(line[i])) {
            if (NULL == location) {
                location = &line[i];
            } else {
                int temp = *location;
                *location = line[i];
                line[i] = temp;
                location = NULL;
            }
        }
    }

or less explicitly (and slightly out of order)

    char *location = NULL;
    for (int i = 0; i < LINE_BUFFER; i++) {
        if (isdigit(line[i])) {
            if (location) {
                int temp = *location;
                *location = line[i];
                line[i] = temp;
                location = NULL;
            } else {
                location = line + i;
            }
        }
    }

Either version reduces three variables at function scope down to one at function scope and one at the most local block scope.

It's up to you which version you find more readable. I also switched between line + i and &line[i]. They do the same thing, so it's pretty much a readability difference.

And I like this better, as rather than using a Boolean to track whether or not we need to swap, we track based on whether we know where to swap. Less parallel logic increases robustness.

Twice now I've used the notation NULL == variable (where variable is any declared variable like location or file). The reason for this is that in C, if (variable = NULL) { is valid and will compile and run. It creates unreachable code, as NULL will always be false. So modern compilers often warn on this. But it's valid grammatically. If you instead always write if (VALUE == variable) { (where VALUE is any constant like NULL or 3), then a typo leads to if (VALUE = variable) { which is not valid and won't compile.

Like I said, most modern compilers will warn on this. So it's not as important as it once was. But if you start editing C code, you are likely to see this idiom used. This is why.

\$\endgroup\$
3
\$\begingroup\$

Bug: potentially accessing line[i] pass the null character. Instead, stop on the null character.

// for (int i = 0; i < LINE_BUFFER; i++) {
for (int i = 0; i < line[i]; i++) {
  if (isdigit(line[i])) {

#define LINE_BUFFER 256 is not necessarily sufficient as there is no limit on "word" length. To handle requirements, a new approach is needed.

Pedantic: Using isdigit() with negative values, aside from EOF, is UB. Not a concern here given "All characters are ASCII.".

// if (isdigit(line[i])) {
if (isdigit((unsigned char) line[i])) {
\$\endgroup\$

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.