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

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();
}

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.

Originally Posted by Adak

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!

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>

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.

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.

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."

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
}

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.

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;
}

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

Originally Posted by Adak

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?

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.

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 .

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.