4
\$\begingroup\$

I'm trying to teach myself C. I've managed to make this program I call HappyToYou.c (inspired by this video):

#include <stdio.h>
//#include <stdlib.h> //malloc would need this.
#include <string.h>
#include <err.h>

void sing(char *start, char *end){
    printf("%s%s.\n", start, end);
}

int main(int argc, char *argv[]){
    if(argc!=3) errx(1, "Give a holiday and a name as arguments.");

    char *happy="Happy ";
    char start[strlen(happy)+strlen(argv[1])+1];
    strcpy(start, happy);
    strcat(start, argv[1]);
    char *to=" to you";


    char *dear=" dear, ";
    char person[strlen(dear)+strlen(argv[2])+1];
    strcpy(person, dear);
    strcat(person, argv[2]);

    sing(start, to);
    sing(start, to);
    sing(start, person);
    sing(start, to);

    return 0;
}

I think it is free from bugs (as far as I know). I'm pretty sure it can be trimmed down but I'm not sure how.

\$\endgroup\$
5
\$\begingroup\$

The str* functions can take string literals as a second argument. For example:

strcpy(start, "Happy ");

Also, one can use sprintf to create a string (you need to have enough memory in it before you use this):

sprintf(out, "Happy %s, dear %s!", holiday, person);

But it's much easier and more readable to just print the whole thing:

#include <stdio.h>
#include <error.h>

int main(int argc, char *argv[]) {
    if (argc != 3)
        errx(1, "Give a holiday and a name as arguments.");

    printf("Happy %1$s to you.\n\
Happy %1$s to you.\n\
Happy %1$s, dear %2$s!\n\
Happy %1$s to you.\n", argv[1], argv[2]);

    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Cool +1. What are those %1$s and %2$s? I've never seen them. Are those standard C? \$\endgroup\$ – Spikatrix Oct 4 '15 at 6:12
  • 1
    \$\begingroup\$ @CoolGuy: not standard C, but they're in POSIX printf \$\endgroup\$ – Mat Oct 4 '15 at 6:49
  • \$\begingroup\$ @Mat Ah. Thanks. I also found this Stack Overflow question mentioning it. Also, GCC gives warning: ISO C does not support %n$ operand number formats [-Wformat=] with -pedantic. \$\endgroup\$ – Spikatrix Oct 4 '15 at 7:30
  • \$\begingroup\$ I'm surprised a Core Review answer which such formatting problems and the omission of optional braces has received so many upvotes. \$\endgroup\$ – nhgrif Oct 4 '15 at 12:35
6
\$\begingroup\$

There're some bugs in the code. For example, if the user provides more inputs than needed, which the program could safely ignore (or warn about), it'll still exit with an error and an incorrect message.

The comparison should be updated to: if (argc < 3)

Although I think the first answer is ok, I think the readability of the code counts, and proper indentation makes a huge difference. You should develop this habit from the start and I'd find the following minor modifications slightly more preferable, IMHO:

#include <stdio.h>
#include <error.h>

int main(int argc, char *argv[]) {
    /* notice the logical operator update here; prevents bogus error
       message if user provides more than 2 inputs */
    if (argc < 3)
        errx(1, "Give a holiday and a name as arguments.");

    printf(
        "Happy %1$s to you.\n"
        "Happy %1$s to you.\n"
        "Happy %1$s, dear %2$s!\n"
        "Happy %1$s to you.\n",
        argv[1],
        argv[2]
    );

    return 0;
}

Also, I'd recommend that you get in the habit of writing user-visible messages using user-friendly terminology, especially when error conditions are detected.

For example, instead of saying "Give a holiday and name as arguments", which relies on programmer terminology (e.g. non-programmers are unlikely to know what "arguments" are), try to be user-friendly. Consider if the program instead said the following during the error and helped the user correct it:

Some inputs appear to be missing. Please enter the holiday and your name.
Usage: HappyToYou <holiday> <name>
Example: HappyToYou Birthday Bob

Few things are more frustrating than error messages that are uninformative or wrong because the former doesn't help you resolve the issue and the latter sends the user down the wrong path in trying to find a solution.

\$\endgroup\$
  • \$\begingroup\$ The \ character is not required. \$\endgroup\$ – Spikatrix Oct 4 '15 at 7:28
  • \$\begingroup\$ @CoolGuy: True in this case. Removed. \$\endgroup\$ – code_dredd Oct 4 '15 at 7:31
  • \$\begingroup\$ You're right about the readability. I forgot that "..." "..." concatenates just like what I have used. \$\endgroup\$ – Vedran Šego Oct 4 '15 at 8:43

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