Thread: Look at my crappy program. Look at it!

So, I managed to get up to the Functions section of the C tutorials. With my newly gained knowledge, I made a fricking calculator. A pretty crappy calculator, I'm not afraid to say, but the first program I ever made by myself. So . . . look at it! Appraise it and give me criticism. Please? >.>

Code:

#include <stdio.h>

int add (int A, int S);
int dif (int D, int F);
int mult (int Q, int W);
int div (int E, int R);

int main()
{
    int A,S,D,F,Q,W,E,R;
    int sheep=1,skin=2,seat=3,covers=4;
    int dumbass;

    /* I know I have weird variable names that don't pertain, at all, to their function.
    That was kind of the point. If you have to look at my crappy code, you may as well have a reason to smile about it.
    Does that compute? */

    printf( "This is Hiru's crappy calculator.\nPlease choose which crappy operation you would like to perform:\n" );
    printf( "(1) Addition; (2) Subtraction; (3) Multiplication; (4) Division: " );
    scanf( "%d", &dumbass );
        if (dumbass==sheep){
            printf( "Enter the two numbers to be added:\n" );
            scanf( "%d", &A );
            scanf( "%d", &S );
            printf( "Your numbers were %d and %d. Their sum is: %d\n", A, S, add (A,S) );
            getchar();
        }
        else if (dumbass==skin){
            printf( "Enter the two numbers to be subtracted:\n" );
            scanf( "%d", &D );
            scanf( "%d", &F );
            printf( "Your numbers were %d and %d. Their difference is: %d\n", D, F, dif (D,F) );
            getchar();
        }
        else if (dumbass==seat){
            printf( "Enter the two numbers to be multiplied:\n" );
            scanf( "%d", &Q );
            scanf( "%d", &W );
            printf( "Your numbers were %d and %d. Their product is: %d\n", Q, W, mult (Q,W) );
            getchar();
        }
        else if (dumbass==covers){
            printf( "Enter the two number to be divided:\n" );
            scanf( "%d", &E );
            scanf( "%d", &R );
            if (R == 0){
            printf( "I'm sorry, Dave. I'm afraid I can't do that.\n" );}
            else if (E == 0){
            printf( "Uh, you can't divide nothing.\nNo matter how badly you want to share it with your friends.\n" );}
            else printf( "Your numbers were %d and %d. Their quotient is: %d\n", E, R, div (E,R) );
            getchar();
        }
}

int add (int A, int S)
{
    return A + S;
}
int dif (int D, int F)
{
    return D - F;
}
int mult (int Q, int W)
{
    return Q * W;
}
int div (int E, int R)
{
    return E / R;
}

-Hiru

Here's some suggestions:
- No offensive words in variables (dumbass).
- Constants that MEAN something make the code easier to read. If they are just names instead of numbers, it doesn't make any sense.
- Single letter variable names (in upper-case, no less) is best avoided.
- two lines of scanf() to read two numbers can be turned into a single scanf("%d %d", &a, &b);
- Dividing zero with non-zero is valid, so there's no reason to check for that.

--
Mats

1. C89 at least defines only 2 variations of main,
int main(void)
int main(int argc, char * argv[])

Main also must return something (explicitly is best).

Pick one on a case-by-case basis, note that int foo() is NOT the same as int foo(void).

2. The variable names are terrible, they don't even relate to what the program does
3. The brace style and indenting is inconsistant
4. There is very little error checking offered with scanf(). Such as overflow, consider using fgets() and strtol() or similar.

Is it some sort of joke that it reads "dumbass sheep skin seat covers"? I have them in my car, very comfortable

Originally Posted by zacs7

Pick one on a case-by-case basis, note that int foo() is NOT the same as int foo(void).

In this case int foo() is the same as int foo(void) because it is part of a function definition.

Originally Posted by matsp

Here's some suggestions:
- No offensive words in variables (dumbass).

Sowwy. :P

- Constants that MEAN something make the code easier to read. If they are just names instead of numbers, it doesn't make any sense.
- Single letter variable names (in upper-case, no less) is best avoided.
- two lines of scanf() to read two numbers can be turned into a single scanf("%d %d", &a, &b);
- Dividing zero with non-zero is valid, so there's no reason to check for that.

--
Mats

- Just giving it a personal touch. ^___^
- I know what you mean, but a single letter variable should be okay for a five-minute program. :P
- Awesome. I suspected as much, but didn't quite figure that that was how it was written.
- I know, but still. Zero out of anything is still zero. Why the hell would you want to divide nothing? You already know the answer just by looking at it. XD

EDIT:
@Zacs
1. I'm at lesson 4, not lesson 104. =.=
2. Precisely!
3. Only because this is a small program, with superfluous things. :P
4. As soon as I learn what the heck those are . . .

Is it some sort of joke that it reads "dumbass sheep skin seat covers"? I have them in my car, very comfortable

Heh. "Sheepskin seat covers" is reference to "Ed, Edd, and Eddy". "Dumbass" comes out of my self-abusive nature. I kept forgetting semicolons and making newbie mistakes, so I made the text responses address me as "dumbass". "Declare integers first, dumbass." "You forgot a semicolon again, dumbass." Et cetera.

-Hiru

Apart from the comments above, here are some more suggestions:

1)
Don't try to apply a personal touch to your code: it's annoying at best. Once you start working on the same piece of code with several other people at once, it will be *really* annoying. If it's a five minute program, don't show it to us in the first place.

2)
Always use proper variable names. If you don't learn it right now, you'll have a hard time later. Same goes for all other comments: try to do it right the first time. Programming will become harder once you advance, so learn the easy things right away. Later, there won't be enough time.

3)
You don't need that many variables. You need two for the numbers, one for the operation and maybe one for the result. In my opinion, proper names would be "a", "b", "op" and "result". I'd replace sheep, skin et.al. with an enum {add, sub, mul, div} or with #defines.

4)
Your functions do similar things, so make them look similar:

Code:

int add (int a, int b);
int dif (int a, int b);
int mult (int a, int b);
int div (int a, int b);

5)
The function argument's name doesn't need to be the same as in the call to the function. The following code is perfectly legal:

Code:

int foo(int a);

[...]

int x;
foo(x);

6)
If you use switch() instead of a series of if-statements, it will make the code much more clear to the reader:

Code:

switch(op) {
        case add:
                printf("%d + %d = %d\n", a, b, add(a, b));
                break;
        case sub:
                [...]
}

7)
Sacrifice verbosity for the sake of simplicity: you don't need 8 scanf() statements to read two numbers. Simply ask for the numbers (once) and for the operation to be performed on them, then use the switch to actually do it:

Code:

puts("Enter two numbers:");
scanf("%d\n%d\n", &a, &b);
puts("1=add, 2=sub, 3=mul, 4=div:");
scanf("%d\n", &op);

switch(op) {   // ...

8)
> Uh, you can't divide nothing.

Sure you can. 0/x is "0", not "nothing". Your output isn't funny, it merely indicates that you don't know how division works.


9)
> I'm at lesson 4, not lesson 104.

If you don't understand a specific comment or ciriticism, ask for an explanation and try to incorporate it into your way of coding. If you don't show any effort to advance your coding skills, someone else might not be willing to show effort and answer your next question.

10)
All of your branches end with getchar(). Simply put the getchar() after the last if-statement.

11)
main() has a return type and hence deserves a return value. One might argue that this is not needed since C99, but the general rule is to explicitly write everything down until you know exactly why you can omit it.

12)
Don't add meta-information to your code, such as "crappy". Either we're able to conclude it ourselves or it's simply wrong. Use comments for explanations, not excuses.


If that's your first program, it's better than most first programs I've seen so far.

Greets,
Philip

Originally Posted by laserlight

In this case int foo() is the same as int foo(void) because it is part of a function definition.

Are you sure?

Code:

int foo()
{
	return 0;
}

int boo(void)
{
	return 2;
}


int main() 
{
	foo(1);
	boo(2);
	return 0;
}

Originally Posted by vart

Are you sure?

hmm... looks like I am wrong on a technicality, since when a function definition acts as a prototype, the point of there being a difference for function declarations comes into play. On the other hand, the underlying point is that a declaration of a function without specifying the parameter list does not declare a different function, thus defining any function, especially main, with a void parameter or with no parameters is effectively the same.

1. "Personal touches" are for personal programs, not group stuff. As long as it's mine, I'm gonna sneak something in there. Sorry.

Furthermore, as I said, it's the first program I've ever made. I'd damn proud of myself, no matter how crappy it is. XD Don't worry, though, the euphoria is wearing off now. I'll have to make an even better program to get it back.

2. I know, I know. Still, while I'm young, I'm going to goof around a little. As my programs get more and more complex, I'll slap myself upside the head. I always do.

3. Still learning. Sorry. :P

4. This is why I showed you my "five-minute" program. So you don't have to give me the same advice fifty programs down the road. Especially when it's advice I can use now.

5. I think I see where you're getting. "Foo" can be used to manipulate another variable? O.o

6. Just went on to lesson 5. Love it already. I am going to definitely use it in the future. XD

Also, it acts like a switch, like a button on your remote, right? I am correct in that interpretation?

7. Oh, that is some sexy little piece of code. You definitely know what you're doing, unlike me. ==;;

8. Meh. Like I said, superfluous. I didn't even think it was that funny myself when I wrote it. Which kinda makes it worse. >.>

9. The point of this wasn't necessarily for expert criticism, but for little things. Also, to brag about my "1337 5|<1LLZ". You can be assured that my future posts will be much more serious.

10. Good advice. Thanks.

11. So, include "return 0;" at the end? Just to make sure it looks pretty?

12. As they said at Leadership Challenge Camp, explanations are just long excuses. :P
That comment was actually added after people kept bugging me about my unorthodox variables. >.>

Yes, it is indeed my first program. I did do several of those tutorials an hour earlier, however, even changing them to test the code here and there. The program I have presented here is the first one I made by myself, for myself, with no instruction whatsoever. I took what I knew and decided to do something with it. Unfortunately, you can see what little I did with that knowledge. XD

EDIT:
I think I'll begin studying American Government for the time being instead. I still need 2.0 credits for my High School Diploma. I'll kind need that if I want to get into college, and a decent job. I'll pick back up again in a short while (hours, likely).

-Hiru

Originally Posted by laserlight

hmm... looks like I am wrong on a technicality, since when a function definition acts as a prototype, the point of there being a difference for function declarations comes into play. On the other hand, the underlying point is that a declaration of a function without specifying the parameter list does not declare a different function, thus defining any function, especially main, with a void parameter or with no parameters is effectively the same.

Are we talking C or C++?

in C++
int f()
is equivalent of
int f(void)

in C

int f()

is equivalent of

int f(...)

and you have no function overloading in C, so you cannot declare several function with the same name and different parameters in any case

Originally Posted by vart

Are we talking C or C++?

C. In C++ they are always the same, even for function prototypes.

Originally Posted by vart

in C

int f()

is equivalent of

int f(...)

No, in C, a missing parameter specification in a function declaration merely states that the parameters of the function are not specified.

US Government is only slightly less dull than US History. So, I took a little break from my studies and worked a little on my code. Here's the latest iteration:

Code:

#include <stdio.h>

int add (int a, int b);
int dif (int a, int b);
int mult (int a, int b);
int div (int a, int b);

int main()
{
    int a,b;
    int op;

    printf( "This is Hiru's crappy calculator.\nPlease choose which crappy operation you would like to perform:\n" );
    printf( "(1) Addition; (2) Subtraction; (3) Multiplication; (4) Division: " );
    scanf( "%d", &op );

    switch (op)
    {
    case 1:
        printf( "Enter two numbers to be added (separate w/ space): " );
        scanf( "%d %d", &a, &b );
        printf( "\nYour numbers were %d and %d. Their sum is: %d\n", a, b, add(a,b) );
        break;
    case 2:
        printf( "Enter two numbers to be subtracted (separate w/ space): " );
        scanf( "%d %d", &a, &b );
        printf( "\nYour numbers were %d and %d. Their difference is: %d\n", a, b, dif(a,b) );
        break;
    case 3:
        printf( "Enter two numbers to be multiplied (separate w/ space): " );
        scanf( "%d %d", &a, &b );
        printf( "\nYour numbers were %d and %d. Their product is: %d\n", a, b, mult(a,b) );
        break;
    case 4:
        printf( "Enter two numbers to be divided (separate w/ space): " );
        scanf( "%d %d", &a, &b );
        if ( b == 0 )
            {
            printf( "\nI'm sorry, Dave. I'm afraid I can't do that.\n" );
            getchar();
            }
        else printf( "\nYour numbers were %d and %d. Their quotient is: %d\n", a, b, div(a,b) );
        break;
    default:
        printf( "\nPlease choose only the operations listed. Thank you.\n" );
        break;
        }
    getchar();
    return 0;
}

int add (int a, int b)
{    return a + b;  }

int dif (int a, int b)
{   return a - b;   }

int mult (int a, int b)
{    return a * b;  }

int div (int a, int b)
{    return a / b;  }

Still not spectacular but a little prettier. In an awkward sort of way. >.>

By the way, as an irrelevant point of little interest, I've been replacing the name in the version I show here. You see my pseudonym while I see my real name. :P

EDIT: Oops! Oh crap, I forgot to change the variables in the definitions. XD One moment, please . . . . Done!


-Hiru

you have a code that repeats itself

Code:

#include <stdio.h>

int add (int a, int b);
int dif (int a, int b);
int mult (int a, int b);
int div (int a, int b);

struct operation
{
	const char* sDo;
	const char* sRes;
	int (*f)(int,int);
};

int main()
{
	int a,b;
	int op;
	struct operation ops[] = {
		{"added" , "sum",&add},
		{"subtracted" , "difference" , &dif},
		{"multiplied" , "product" , &mult},
		{"divided" , "quotient" , &div}
	};
	printf( "This is Hiru's crappy calculator.\nPlease choose which crappy operation you would like to perform:\n" );
	printf( "(1) Addition; (2) Subtraction; (3) Multiplication; (4) Division: " );
	scanf( "%d", &op );

	switch (op)
	{
	case 1:
	case 2:
	case 3:
	case 4:
		printf( "Enter two numbers to be %s (separate w/ space): " , ops[op-1].sDo);
		scanf( "%d %d", &a, &b );
		if (op == 4 &&  b == 0 )
		{
			printf( "\nI'm sorry, Dave. I'm afraid I can't do that.\n" );
			getchar();
		}
		else
		{
			printf( "\nYour numbers were %d and %d. Their %s is: %d\n", a, b, ops[op-1].sRes,ops[op-1].f(a,b) );
		}
		break;
	default:
		printf( "\nPlease choose only the operations listed. Thank you.\n" );
		break;
	}
	getchar();
	return 0;
}

int add (int a, int b)
{    return a + b;  }

int dif (int a, int b)
{   return a - b;   }

int mult (int a, int b)
{    return a * b;  }

int div (int a, int b)
{    return b / b;  }

As strange as it sounds, all that code actually makes sense to me. O.o

I may not understand how every little piece in it works but I can see how it's working in that code. Even so, I still don't have much of a clue as to how a pointer works yet. Even though I've already read that tutorial and can see how useful they can be. @_@

Oh, and a question. This is the basic purpose for creating a function, more or less, right?

Code:

#include whatever

Do_This();

int main()
{

    printf( "Make your choice:\n );

    /* Which leads to . . . */

    Do_This();

}


Do_This()
{

    /* Whatever was intended to be done. */

}

EDIT: Main() doesn't and shouldn't handle everything. As the programs get more complex, I should be putting less and less in main() and using more and more independent functions. Right? If so, I can see how everything works now. Main() starts the program, then the program takes it from there. Right?

-Hiru