# Grade my first lab? (and program :D)

Show 80 post(s) from this thread on one page
Page 1 of 2 12 Last
• 02-21-2012
Cowmoogun
Grade my first lab? (and program :D)
Well this is my first program and it runs well with no bugs =D, but that doesnt do much if the professor doesnt think the code is modular or easy to read so I was wondering if you could give some feedback on this program. It's very simple, user inputs three numbers and it will respond with what type of triangle it is and its area with a loop. I appreciate any and all feedback :cool:

Code:

```#include <stdio.h> #include <conio.h> #include <math.h> int main () {         int side1, side2, side3;         double S;         double Area;                 do{ printf("\nPlease enter three sides of a triangle.\n");        /* S is the semiperimeter of a triangle which is essential in finding*/                 scanf("%d%d%d", &side1, &side2, &side3);                                /*        a consistent area for the program.                                                                */                 S= (float)(side1+side2+side3)/2;                 Area = sqrt (S * (S-side1) *(S-side2)*(S-side3));                                                                 if (((side1 == side2) && (side2 == side3) && (side1 == side3)) && ((side1 + side2 > side3) && (side2 + side3 > side1) && (side1 + side3 > side2)))        /* All sides must be equal */                                                                                                                                                                                                                                                                                                                                 /* and follow the general laws of a triangle:*/                                 {                                                                                                                                                                                                                                                                                                /* The sum of any two sides must be greater than the remaining side. */                                 printf("This triangle's sides are %d,%d,%d and is an Equalateral triangle.\nIts area is %.05f.", side1, side2, side3, Area);                                                 }                         else if (((side1 == side2) || (side2 == side3) || (side1 == side3)) && (side1 + side2 >side3) &&  (side2 + side3 >side1) &&  (side1 + side3 > side2))  /* Two sides must be equal */                                 {                                 printf("This triangle's sides are %d,%d,%d and is an Isosceles triangle.\nIts area is %.05f.", side1, side2, side3, Area);                         }                                                 else if ((side1 != side2) && (side3 != side2)&& (side1 != side3) && (side1 + side2 >side3 && side2 + side3 >side1 &&  side1 + side3 > side2))                        /* None of the sides can be the same */                                 {                                 printf("This triangle's sides are %d,%d,%d and is an Scalene triangle.\n Its area is %.05f.", side1, side2, side3, Area);                                                 }                                                 else                                 {                                        printf("%d,%d,%d are not valid sides to a triangle.\n", side1,side2,side3);        /* Will end the program once user inputs an incorrect combination of numbers. */                                 getch();                         }                                   }while (side1 + side2 >side3 && side2 + side3 >side1 && side1 + side3 > side2);                 return 0;                 getch(); }```
• 02-21-2012
I did not look through the entire program, but this struck me:
Code:

`if (((side1 == side2) && (side2 == side3) && (side1 == side3))`
If side1 == side2 && side2 == side3, then you've already proven than side1 == side3, so no need to test that again.
• 02-21-2012
Cowmoogun
Quote:

I did not look through the entire program, but this struck me:
Code:

`if (((side1 == side2) && (side2 == side3) && (side1 == side3))`
If side1 == side2 && side2 == side3, then you've already proven than side1 == side3, so no need to test that again.

Ah yes that is a bit redundant, thank you for pointing that out!
• 02-21-2012
manasij7479
You can structure it better (as some of the conditions are being checked many times).
Code:

``` if(<check if triangle is valid>) {     if(<check for equilateral>)         //...     else if (<check for isosceles>)         //...     else         //scalene } else   //<print your error>```
• 02-21-2012
nvoigt
Some formal feedback. Note that it's style and can (and will) be argued.

Remove any empty lines after the first in each empty space. One empty line is enough to space things out.

Your side variables are lowercase, your S variable is uppercase and Area is so called Pascal-Case. Decide which you want (all lowercase is common for local variables) and stick with it for all three/five.

Each curly brace should reside on a line of it's own. From each closing brace, the corresponding opening brace should be straight up so you just have to draw a vertical line to find the two matching braces. Look at manasij7479s code to get a feeling for it.
• 02-21-2012
stahta01
Style issue on this line.
Code:

`S= (float)(side1+side2+side3)/2;`
I would used a better name for "S" as others has stated.

I would tend to use this code below. I avoid cast if possible.
Code:

`S= (side1+side2+side3)/2.0;`
or you might use this code (S is of type double)
Code:

`S= (double)(side1+side2+side3)/2;`
Tim S.
• 02-21-2012
Cowmoogun
Quote:

Originally Posted by manasij7479
You can structure it better (as some of the conditions are being checked many times).
Code:

``` if(<check if triangle is valid>) {     if(<check for equilateral>)         //...     else if (<check for isosceles>)         //...     else         //scalene } else   //<print your error>```

I appreciate all the comments so far and I'm implementing them to stream line the code but i'm coming across a new problem in my do while statement. Now in my while statement i have the law of a triangle, so i took them out of the if statements and now it will say it's a triangle even when it isn't -__-. Anyone know why this is? Wouldnt it check to make sure it fits within the while statement so it doesnt "do it."
• 02-21-2012
manasij7479
Quote:

Originally Posted by Cowmoogun
I appreciate all the comments so far and I'm implementing them to stream line the code but i'm coming across a new problem in my do while statement. Now in my while statement i have the law of a triangle, so i took them out of the if statements and now it will say it's a triangle even when it isn't -__-. Anyone know why this is? Wouldnt it check to make sure it fits within the while statement so it doesnt "do it."

That is because you used a do while loop.
For the first time, it gets executed regardless of the conditions.
Change
Code:

`do {...}while (condition);`
to
Code:

`while(condition){...}`
But then, the values will be checked before they are entered.
A better solution is to:
Code:

```while(1)//infinite loop {     do     {         //User input     }while(<triangle is NOT valid>);//     //Everything else }```
• 02-21-2012
anduril462
Please post your new code. Also, you could "flatten" the structure manasij7479 suggested:
Code:

```if triangle is invalid     print error else if equilateral     ... else if isosceles     ... else     // scalene```
Your first call to getch should probably not be inside the else clause, otherwise it wont always be executed (i.e. why would you only want to call getch when they entered an invalid input). Your last call to getch is after the return 0; in main, so it will never get executed. Either remove it or switch the two lines.

Lastly, as others pointed out, S is poorly names. So much so that you need a comment to explain that S is the semiperimeter. Just call it semiperimeter and be done with it.
• 02-21-2012
Cowmoogun
Quote:

Originally Posted by anduril462
Please post your new code. Also, you could "flatten" the structure manasij7479 suggested:
Code:

```if triangle is invalid     print error else if equilateral     ... else if isosceles     ... else     // scalene```
Your first call to getch should probably not be inside the else clause, otherwise it wont always be executed (i.e. why would you only want to call getch when they entered an invalid input). Your last call to getch is after the return 0; in main, so it will never get executed. Either remove it or switch the two lines.

Lastly, as others pointed out, S is poorly names. So much so that you need a comment to explain that S is the semiperimeter. Just call it semiperimeter and be done with it.

Here is the new code I have, I rewrote it..but now the weirdest thing..the math for my areas is wrong. For example..sides 3,4,5 should have area 6 but now im getting 7.2 -_-.

Code:

```#include <stdio.h> #include <conio.h> #include <math.h> int main () {     int side1, side2, side3;                    double semiperimeter, area;                                                                                                                                do         {      printf("\n Please enter three sides to a triangle. \n");           scanf("%d%d%d", &side1, &side2, &side3);             semiperimeter = (double)(side1 + side2 + side3)/2;           area = sqrt(semiperimeter * (semiperimeter - side1) * (semiperimeter - side1) * (semiperimeter - side3));                     if (side1 + side2 > side3 && side2 + side3 > side1 && side1 + side3 > side2)           {    if ((side1 == side2) && (side2 == side3))                     printf("This triangles sides are %d,%d,%d and is an equalateral triangles.\nIts area is %.05f.\n", side1, side2, side3, area);                                 else if ((side1 == side2) || (side2 == side3) || (side1 == side3))                     printf("This triangles sides are %d,%d,%d and is an Isosceles triangle.\nIts area is %.05f.\n", side1, side2, side2, area);                                 else                   printf("This triangles sides are %d,%d,%d and is a Scalene triangle.\nIts area is %.05f.\n", side1, side2, side3, area);           }           else               printf("%d,%d,%d are not valid sides to a triangle.\n", side1,side2,side3);         }while ((side1 + side2 >side3) && (side2 + side3 >side1) && (side1 + side3 > side2));             getch();         return 0; }```
• 02-21-2012
You have an error in Heron's formula. Side1 is in there twice, side2, not at all.
• 02-21-2012
Cowmoogun
Quote:

You have an error in Heron's formula. Side1 is in there twice, side2, not at all.

Wow...I want to kill myself with that mistake. Really have to work on my attention to detail, thank you. How is the layout of the code now based on your opinion? Is it cleaner then the first?
• 02-21-2012
anduril462
Much better, though the indentation on line 16 seems to be off by one, and everything from lines 13-36 should be de-indented one step, so it lines up with the declarations on lines 8-9. As an issue of personal preference, I would move the first statment of a block onto a line of it's own (i.e. don't share it with the opening curly bracket). Also, I would remove (subjectively) unnecessary blank lines (10, 11, 18, 19, 24, 27). My suggestion is to pick one of the first 3 styles mentioned on Indent style - Wikipedia, the free encyclopedia, and stick to it.
• 02-21-2012
Cowmoogun
Quote:

Originally Posted by anduril462
Much better, though the indentation on line 16 seems to be off by one, and everything from lines 13-36 should be de-indented one step, so it lines up with the declarations on lines 8-9. As an issue of personal preference, I would move the first statment of a block onto a line of it's own (i.e. don't share it with the opening curly bracket). Also, I would remove (subjectively) unnecessary blank lines (10, 11, 18, 19, 24, 27). My suggestion is to pick one of the first 3 styles mentioned on Indent style - Wikipedia, the free encyclopedia, and stick to it.

I really appreciate the responses on this forum, I've received a lot of help from all who responded! Anduril, i'm doing exactly what you told me to do now, i'll post the final code when I'm completed and i'll let you guys know what my professor gave me afterwards :).
• 02-21-2012
rogster001
You have certainly taken on board the advice given, the code is more concise and readable, it is good that you dropped the 'S' variable, apart from your general changes to remove this, THINGS_IN_CAPS should be reserved for constant values.
A further point is conio.h is not standard, you should not rely on this in your code, and consider your compiler choice, if you are using turbo c IDE change it.
Show 80 post(s) from this thread on one page
Page 1 of 2 12 Last