Re: Why won't this code work?

Discussion in 'C Programming' started by John Bode, Aug 20, 2003.

  1. John Bode

    John Bode Guest

    Eirik <> wrote in message news:<>...
    > <BADLY DESIGNED C CODE>
    >
    > #include <stdio.h>
    >
    > void combo(int *, int *, int *, int);
    > void div(int *, int *, float *);
    >
    > int main(int argc, char *argv[])
    > {
    > int n1;
    > int n2;
    > char type[100];
    > int result;
    > float dres;
    >
    > printf("Number 1.\n");
    > scanf("%d", &n1);
    >
    > printf("Number 2.\n");
    > scanf("%d", &n2);
    >
    > printf("Addition, subtraction, multiplication, division.\n");
    > gets(type);


    Replace that gets() call with this:

    fgets (type, sizeof type, stdin);
    if (strchr (type, '\n'))
    *(strchr (type, '\n')) = 0;

    Reason: if somebody's stupid enough to type more than 100 characters,
    gets() will happily store the extra characters to the memory following
    the array. This is called a buffer overrun, which can lead to crashes
    or weird behavior. It also creates a potential security hole; many
    worms such as the recent MSBlast worm exploit buffer overruns.
    fgets() allows you to specify the maximum size of the array (given by
    sizeof type); the function will read sizeof type - 1 characters from
    standard input and save them to type.

    Since fgets() stores the terminating newline to the array (if there's
    room), you'll need to remove it before doing any comparisons. The
    strchr() call returns a pointer to the '\n' character if it's present.
    If it is, we replace it with the nul terminator.

    >
    > if(type=="addition")


    The '==' operator cannot compare array contents. Instead, it compares
    the base address of type to the base address of each string literal.
    It's almost guaranteed that the string literals have different base
    addresses from type, so none of the tests succeed. To compare array
    contents, use strcmp() as below:

    if (strcmp (type, "addition") == 0)

    Repeat for each case.

    > {
    > combo(&n1, &n2, &result, 1);
    > printf("%d + %d = %d\n", n1, n2, result);
    > }
    > if(type=="subtraction")
    > {
    > combo(&n1, &n2, &result, 2);
    > printf("%d - %d = %d\n", n1, n2, result);
    > }
    > if(type=="multiplication")
    > {
    > combo(&n1, &n2, &result, 3);
    > printf("%d * %d = %d\n", n1, n2, result);
    > }
    > if(type=="division")
    > {
    > div(&n1, &n2, &dres);
    > printf("%d / %d = %f\n", n1, n2, dres);
    > }
    > return 0;
    > }
    >
    > void combo(int *nO, int *nT, int *R, int m)
    > {
    > if(m == 1)
    > {
    > *R = *nO + *nT;
    > }
    > if(m==2)
    > {
    > *R = *nO - *nT;
    > }
    > if(m==3)
    > {
    > *R = *nO * *nT;
    > }
    > }
    >
    > void div(int *nO, int *nT, float *R)
    > {
    > *R = (float) *nO / *nT;
    > }
    >
    > </BADLY DESIGNED C CODE>
    >
    > Explanation of silly variable names:
    > *R = Result
    > *nO = Number One
    > *nT = Number Two
    > m = Method
    > n1 = Number 1
    > n2 = Number 2
    > dres = Division Result
    >
    > Stupid function names:
    > combo = Combination of add, subtract and multiply
    > div = Division
    >
    > I've tried using scanf() instead of gets, but that doesn't work either.
    > It compiles with no errors or warnings whatsoever when I don't use -Wall.
    > The problem is that the program stops when it is supposed to ask me
    > if I want it to add, subtract, multiply or divide.
    > I have tried these approaches:
    >
    > 1 "char *type;"&"scanf("%s", &type);"
    > 2 "char type[100];"&"scanf("%s", &type);"
    > 3 "char type[100];"&"scanf("%c", &type);"
    > 4 "char *type;"&"gets(type);"
    > 5 "char type[100];"&"gets(type);"
    >
    > gcc calc.c -o calc -Wall produces the following output:
    >
    > 1 warning: char format, pointer arg (arg 2)
    > 2 warning: char format, different type arg (arg 2)
    > 3 warning: char format, different type arg (arg 2)
    > 4 : the 'gets' function is dangerous and should not be used.
    > 5 : the 'gets' function is dangerous and should not be used.
    >
    > I use Mandrake GNU/Linux 9.1 with gcc 3.3 and glibc 2.3.1.
    >
    > I hope this question is not to stupid. Flame me if you want at
    > . Replies are also welcome at the _VERY_ same
    > address!
    >
    > Eirik
     
    John Bode, Aug 20, 2003
    #1
    1. Advertising

  2. John Bode

    Eirik Guest

    > Replace that gets() call with this:
    >
    > fgets (type, sizeof type, stdin);
    > if (strchr (type, '\n'))
    > *(strchr (type, '\n')) = 0;
    >
    > Reason: if somebody's stupid enough to type more than 100 characters,
    > gets() will happily store the extra characters to the memory following
    > the array. This is called a buffer overrun, which can lead to crashes
    > or weird behavior. It also creates a potential security hole; many
    > worms such as the recent MSBlast worm exploit buffer overruns.
    > fgets() allows you to specify the maximum size of the array (given by
    > sizeof type); the function will read sizeof type - 1 characters from
    > standard input and save them to type.
    >
    > Since fgets() stores the terminating newline to the array (if there's
    > room), you'll need to remove it before doing any comparisons. The
    > strchr() call returns a pointer to the '\n' character if it's present.
    > If it is, we replace it with the nul terminator.
    >
    >>


    Thank you for explaining this in a way in which the human brain is able to understand. I replaced the 'if(x == 'x')' with 'if(strcmp(x, "x") == NULL)' and replaced gets with 'fgets(type, sizeof(type), stdin)', but the program stops at 'Addition**********'. How come?

    Here is the code:

    #include <stdio.h>

    void combo(float *, float *, float *, int);
    void div(float *, float *, float *);

    int main(int argc, char *argv[])
    {

    float tal1;
    float tal2;
    char type[100];
    float resultat;
    float dres;

    printf("Skriv inn tal 1.\n");
    scanf("%d", &tal1);

    printf("Skriv inn tal 2.\n");
    scanf("%d", &tal2);

    printf("1: Addisjon\n2: Subtraksjon\n3: Multiplikasjon\n4: Divisjon\n");
    fgets(type, sizeof(type), stdin);
    if(strchr(type, '\n'))
    {
    *(strchr(type, '\n')) == 0;
    }

    if(strcmp(type, "addition") == NULL)

    {

    combo(&tal1, &tal2, &resultat, 1);
    printf("%d + %d = %d\n", tal1, tal2, resultat);

    }

    if(strcmp(type, "subtraction") == NULL)

    {

    combo(&tal1, &tal2, &resultat, 2);
    printf("%d - %d = %d\n", tal1, tal2, resultat);

    }

    if(strcmp(type, "multiplication") == NULL)

    {

    combo(&tal1, &tal2, &resultat, 3);
    printf("%d * %d = %d\n", tal1, tal2, resultat);

    }

    if(strcmp(type, "division") == NULL)

    {

    div(&tal1, &tal2, &dres);
    printf("%d / %d = %f\n", tal1, tal2, dres);

    }

    return 0;

    }

    void combo(float *tE, float *tT, float *R, int m)

    {

    if(m == 1)
    {
    *R = *tE + *tT;
    }
    if(m==2)
    {
    *R = *tE - *tT;
    }
    if(m==3)
    {
    *R = *tE * *tT;
    }

    }

    void div(float *tE, float *tT, float *R)

    {

    *R = (float) *tE / *tT;

    }

    Thank you again for answering in a nice and polite manner.
    Eirik
     
    Eirik, Aug 21, 2003
    #2
    1. Advertising

  3. On Thu, 21 Aug 2003 22:17:15 +0200, Eirik <>
    wrote:
    <snip>
    > Thank you for explaining this in a way in which the human brain is able to understand. I replaced the 'if(x == 'x')' with 'if(strcmp(x, "x") == NULL)' and replaced gets with 'fgets(type, sizeof(type), stdin)', but the program stops at 'Addition**********'. How come?
    >

    Please don't use such absurdly long lines in postings.

    > Here is the code:
    >
    > #include <stdio.h>
    >
    > void combo(float *, float *, float *, int);
    > void div(float *, float *, float *);
    >
    > int main(int argc, char *argv[])


    Good signature for main. One bonus point.
    Although since you don't use command-line arguments, you could use
    int main (void) /* or just () equivalent except for recursive calls */

    > {
    >
    > float tal1;
    > float tal2;
    > char type[100];
    > float resultat;
    > float dres;
    >

    For future reference, it is usually better to use 'double' instead of
    'float' unless you have so much data the space used makes a
    difference. 'double' is (almost always) more precise (and wider
    ranged, though that's not likely an issue for you); in the dim past,
    when electronic dinosaurs ruled cyberspace, it was often slower, but
    on modern systems it is usually as fast and sometimes faster because
    it saves extra conversions.

    > printf("Skriv inn tal 1.\n");
    > scanf("%d", &tal1);
    >
    > printf("Skriv inn tal 2.\n");
    > scanf("%d", &tal2);
    >

    %d is wrong for float, use %f. (Or %lf for double.)

    Should check these for failure, in case user doesn't type correct
    input: if( scanf( etc ) != 1 ) { do something -- for a toy program
    like this, probably just print an error and exit }

    > printf("1: Addisjon\n2: Subtraksjon\n3: Multiplikasjon\n4: Divisjon\n");


    Aside: I think this prompt is confusing; you apparently don't want
    the user to enter the number, the name with a capital letter, or both,
    or indeed the name spelled as in the prompt, see below.

    > fgets(type, sizeof(type), stdin);


    Should check for error: if( fgets( etc ) == NULL ) { error }
    or just if( ! fgets( etc ) ) { error }

    FAQ 12.18 (except fgets rather than gets): the previous scanf %d
    (almost certainly) left a newline in the input stream, and that is all
    that is read by the fgets here. Mixing streamed scanf and
    line-oriented fgets correctly is difficult; best is to either input
    everything as lines, and then convert the numbers using sscanf or
    strtol etc.; or since you only want to input a single word here, not
    really a line, use scanf %99s. (And again, check for failure.)

    > if(strchr(type, '\n'))
    > {
    > *(strchr(type, '\n')) == 0;
    > }
    >

    You wanted assignment = not equality test ==.

    > if(strcmp(type, "addition") == NULL)
    >

    Compare to 0, not NULL. It is implementation dependent whether NULL
    has the value /*int*/0 or the value (void*)0, and only the former can
    validly be compared to the return from strcmp. If you insist on
    having a name, define your own:
    #define STR_EQUAL 0 /* or */
    enum { STR_EQUAL = 0 };

    Note that your prompt above talked about "Addisjon" but you are
    looking for input "addition" here. These are not the same; both
    differences in spelling *and differences in case* matter. Pick which
    you want and be consistent.

    > {
    >
    > combo(&tal1, &tal2, &resultat, 1);
    > printf("%d + %d = %d\n", tal1, tal2, resultat);
    >

    %d is wrong for a float (which is actually promoted to double).
    Use %f or conceivably %g (and the same for double, NOT %lf).

    > }
    >
    > if(strcmp(type, "subtraction") == NULL)
    >
    > {
    >
    > combo(&tal1, &tal2, &resultat, 2);
    > printf("%d - %d = %d\n", tal1, tal2, resultat);
    >
    > }
    >
    > if(strcmp(type, "multiplication") == NULL)
    >
    > {
    >
    > combo(&tal1, &tal2, &resultat, 3);
    > printf("%d * %d = %d\n", tal1, tal2, resultat);
    >
    > }
    >

    Ditto, ditto, ditto.

    > if(strcmp(type, "division") == NULL)
    >
    > {
    >
    > div(&tal1, &tal2, &dres);
    > printf("%d / %d = %f\n", tal1, tal2, dres);
    >

    There seems no point to using dres here instead of resultat.
    You did get *one* of the formats correct (%f) here, though.

    > }
    >
    > return 0;
    >

    Good exit status. Another bonus point.

    > }
    >
    > void combo(float *tE, float *tT, float *R, int m)
    >
    > {
    >
    > if(m == 1)
    > {
    > *R = *tE + *tT;
    > }
    > if(m==2)
    > {
    > *R = *tE - *tT;
    > }
    > if(m==3)
    > {
    > *R = *tE * *tT;
    > }
    >
    > }
    >
    > void div(float *tE, float *tT, float *R)
    >
    > {
    >
    > *R = (float) *tE / *tT;
    >

    The cast is redundant; *t is already a float.

    > }
    >
    > Thank you again for answering in a nice and polite manner.
    > Eirik


    - David.Thompson1 at worldnet.att.net
     
    Dave Thompson, Sep 1, 2003
    #3
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Marina

    Re: WHY, WHY WON'T IT WORK???

    Marina, Jun 29, 2004, in forum: ASP .Net
    Replies:
    2
    Views:
    359
    Marina
    Jun 29, 2004
  2. Chad
    Replies:
    4
    Views:
    8,375
  3. learningjava
    Replies:
    8
    Views:
    488
    John C. Bollinger
    Dec 12, 2003
  4. Joona I Palaste

    Re: Why won't this code work?

    Joona I Palaste, Aug 19, 2003, in forum: C Programming
    Replies:
    1
    Views:
    354
    Joona I Palaste
    Aug 19, 2003
  5. Mr. SweatyFinger
    Replies:
    2
    Views:
    2,076
    Smokey Grindel
    Dec 2, 2006
Loading...

Share This Page