Pierwszy program napisany samodzielnie do Oceny /OPINIE/SUGESTIE

Witam wszystkich serdecznie, od niedawna zacząłem bawić się w programowanie i stworzyłem swój pierwszy program do liczenia kalorii. Proszę wszystkich o opinie i sugestie.

#include <stdio.h>
#include <stdlib.h>

void powitanie();
void podsumowanie();
int CEL();
int DANE();
float PAL();
float TDEE();
float BMI();
float CPM();

int a=0;
int odp,plec,wk,palwybor,cel;
float wg,wg1,wz1,wz,wz2,bmi,bmr_k,bmr_m,bmr_a,bmr_b,bmr_c,pal,cpm;



int main()
{	
	
	powitanie();

	if (a==1)
	{	
		BMI();
		system("cls");	
		printf("\nTwoje BMI to: %f \n\n",bmi);
	}
else
	{	
		if (a==2)
		{
			DANE();
			TDEE();	
		}
		
		if(a==3) 
		{
			DANE();
			PAL();
			TDEE();
			CEL();
		}	
	}
	
	
	while(a==1)
{
	printf("Czy chcesz wyswietlic podsumowanie?\n 1.tak\n 2.nie");
	scanf("%d",&odp);
	
		if(odp==1)
		{
			podsumowanie();
			
			a=0;
		}
}
	getchar();
	return 0;
}

void powitanie()
{
	printf("________________________________________________________________________________\n");
	printf("	\n \t\t\t \t Witaj \n\n\t Pomozemy obliczyc twoje zapotrzebowanie kaloryczne :)\n\n");
	printf("_______________________________________________________________________________");
	printf("\nwybierz rodzaj operacji:\n \n 1. Obliczanie BMI\n 2. Dzienne zapotrzebowanie kaloryczne.\n 3. CPM - Calkowita przemiana materii.\n");
	scanf("%d",&a);
}
int DANE()
{
	printf(" \n Podaj wage w kg: ");
	scanf("%f",&wg);
	printf(" \n Podaj wzrost w centymetrach: ");
	scanf("%f",&wz);
	printf(" \n Podaj wiek: ");
	scanf("%d",&wk);
	printf(" \n Podaj swoja plec: \n 1.Kobieta \n 2.Mezczyzna ");
	scanf("%d",&plec);
	
	return wg,wz,wk,plec;
}
int CEL()
{
	printf ("Jaki jest twoj cel: \n1. Utrzymanie wagi.\n2. Redukcja wagi.\n3. Zwiekszenie wagi.\n");	
	scanf("%d",&cel);
	
	if (cel==1)
		
		printf("Aby utrzymac wagę musisz jesc - %f kalori",cpm);
	
	if (cel==2)
	{
		cpm*=0.9;	
		printf("Aby zredukowac wage musisz jesc - %f kalori", cpm);
	}

	if (cel==3)
	{
		cpm*=1.12;
		printf("Aby zwiekszyc mase musisz jesc - %f kalori",cpm);	
	}
}
float BMI ()
{

printf("Podaj wage w kilogramach: ");
scanf("%f",&wg);
printf("podaj wzrost w metrach: ");
scanf("%f",&wz);
wz2=wz*wz;
bmi=wg/wz2;

}
void podsumowanie()
{
	printf("Twoj wzrost - %.2f\n",wg);
	printf("Twoja waga - %.2f\n",wz);
	printf("Twoje BMI - %.2f\n",bmi);
	
if(bmi<17)

		{	printf("wyglodzenie"); }
else

{	
	if(bmi>17&&bmi<=20)
	{ printf("niedozywienie"); }		

	if(bmi>20&&bmi<=25)
	{ printf("BMI w normie"); }

	if(bmi>25&&bmi<=30)
	{ printf("nadwaga");	}
	
	if(bmi>30&&bmi<=35)
	{ printf("I-stopien nadwagi");}
	
	if(bmi>35&&bmi<=40)
	{ printf(" II stopien nadwagi");}
	
	if(bmi>40)
	{ printf(" III stopien nadwagi");}
}


}
float TDEE()	
{
bmr_a=10;
bmr_b=6,25;
bmr_c=5;

if(plec==1)
		{
		bmr_k = bmr_a*wg+bmr_b*wz-bmr_c*wk-161;
		cpm = bmr_k*pal;
		}
else
{
if(plec==2)
		{
		bmr_m = bmr_a*wg+bmr_b*wz-bmr_c*wk+5;
		cpm=bmr_m*pal;
		}	
}
return bmr_m;
return cpm;
}
float PAL()
{
	printf("\nMusisz wybrac swoj wspolczynnik aktywnosci: \n1. Niska aktywnosc (wiekszosc osob nie trenujacych.\n2. Srednia aktywnosc (wiekszosc trenujacych 3-5 x w tygodniu)\n3. Wysoka aktywnosc (Praca fizyczna + regularne treningi )\n\n");
	scanf("%d",&palwybor);

if (palwybor==1)
{
	pal=1.545;
}
else
{
if(palwybor==2)
	{
		pal=1.845;
	}
	else
	{
	
	if(palwybor==3)
	{
		pal=2.2;
	}
}
}
return pal;
}

Pozdrawiam
Krzysztof

Zacznij od prostych rzeczy:
Krok zero: sformatuj kod. Np. po While masz klamre na poczatku linijki a nie wcieta.
Krok pierwszy: wywal magic numbers
Krok dwa: uzyj znaczacych nazw dla zmiennych
Krok trzy: uzyj lepszych nazw dla fuunkcji mowiacych co dana funkcja robi. Idealnie funkcja powinna robic tylko jedna rzecz. Jesli funkcje musialbys nazwac: WypiszCosPotemSprawdzWyborANastepnieWZaleznosciOdTegoCoWyszloWykonajJedneLubDrugieObliczenia (przejaskrawiam celowo) to znaczy ze funkcje trzeba rozbic.
Krok cztery: drabinki ifow to zło, wlaszcza takie ktore sprawdzaja kilka razy ten sam warunek, i robia sprawdzenia do konca pomimo ze wiadomo ze nic juz nie bedzie pasowac.
Krok piec: dodaj walidacje danych od usera

Przepuszczone przez http://format.krzaq.cc/ i trochę ręcznych poprawek:

#include <stdio.h>
#include <stdlib.h>

void powitanie();
void podsumowanie();
int CEL();
int DANE();
float PAL();
float TDEE();
float BMI();
float CPM();

int a = 0;
int odp, plec, wk, palwybor, cel;
float wg, wg1, wz1, wz, wz2, bmi, bmr_k, bmr_m, bmr_a, bmr_b, bmr_c, pal, cpm;

int main()
{

    powitanie();

    if (a == 1) {
        BMI();
        system("cls");
        printf("\nTwoje BMI to: %f \n\n", bmi);
    } else {
        if (a == 2) {
            DANE();
            TDEE();
        }

        if (a == 3) {
            DANE();
            PAL();
            TDEE();
            CEL();
        }
    }

    while (a == 1) {
        printf("Czy chcesz wyswietlic podsumowanie?\n 1.tak\n 2.nie");
        scanf("%d", &odp);

        if (odp == 1) {
            podsumowanie();
            a = 0;
        }
    }
    getchar();
    return 0;
}

void powitanie()
{
    printf("________________________________________________________________________________\n");
    printf("    \n \t\t\t \t Witaj \n\n\t Pomozemy obliczyc twoje zapotrzebowanie kaloryczne :)\n\n");
    printf("_______________________________________________________________________________");
    printf("\nwybierz rodzaj operacji:\n \n 1. Obliczanie BMI\n 2. Dzienne zapotrzebowanie kaloryczne.\n 3. CPM - Calkowita przemiana materii.\n");
    scanf("%d", &a);
}

int DANE()
{
    printf(" \n Podaj wage w kg: ");
    scanf("%f", &wg);
    printf(" \n Podaj wzrost w centymetrach: ");
    scanf("%f", &wz);
    printf(" \n Podaj wiek: ");
    scanf("%d", &wk);
    printf(" \n Podaj swoja plec: \n 1.Kobieta \n 2.Mezczyzna ");
    scanf("%d", &plec);

    return wg, wz, wk, plec;
}

int CEL()
{
    printf("Jaki jest twoj cel: \n1. Utrzymanie wagi.\n2. Redukcja wagi.\n3. Zwiekszenie wagi.\n");
    scanf("%d", &cel);

    if (cel == 1) {
        printf("Aby utrzymac wagę musisz jesc - %f kalori", cpm);
    }

    if (cel == 2) {
        cpm *= 0.9;
        printf("Aby zredukowac wage musisz jesc - %f kalori", cpm);
    }

    if (cel == 3) {
        cpm *= 1.12;
        printf("Aby zwiekszyc mase musisz jesc - %f kalori", cpm);
    }
}

float BMI()
{
    printf("Podaj wage w kilogramach: ");
    scanf("%f", &wg);
    printf("podaj wzrost w metrach: ");
    scanf("%f", &wz);
    wz2 = wz * wz;
    bmi = wg / wz2;
}

void podsumowanie()
{
    printf("Twoj wzrost - %.2f\n", wg);
    printf("Twoja waga - %.2f\n", wz);
    printf("Twoje BMI - %.2f\n", bmi);

    if (bmi < 17)
    {
        printf("wyglodzenie");
    } else {
        if (bmi > 17 && bmi <= 20) {
            printf("niedozywienie");
        }

        if (bmi > 20 && bmi <= 25) {
            printf("BMI w normie");
        }

        if (bmi > 25 && bmi <= 30) {
            printf("nadwaga");
        }

        if (bmi > 30 && bmi <= 35) {
            printf("I-stopien nadwagi");
        }

        if (bmi > 35 && bmi <= 40) {
            printf(" II stopien nadwagi");
        }

        if (bmi > 40) {
            printf(" III stopien nadwagi");
        }
    }
}

float TDEE()
{
    bmr_a = 10;
    bmr_b = 6, 25;
    bmr_c = 5;

    if (plec == 1) {
        bmr_k = bmr_a * wg + bmr_b * wz - bmr_c * wk - 161;
        cpm = bmr_k * pal;
    } else {
        if (plec == 2) {
            bmr_m = bmr_a * wg + bmr_b * wz - bmr_c * wk + 5;
            cpm = bmr_m * pal;
        }
    }
    return bmr_m;
}

float PAL()
{
    printf("\n"
           "Musisz wybrac swoj wspolczynnik aktywnosci: \n"
           "1. Niska aktywnosc (wiekszosc osob nie trenujacych.\n"
           "2. Srednia aktywnosc (wiekszosc trenujacych 3-5 x w tygodniu)\n"
           "3. Wysoka aktywnosc (Praca fizyczna + regularne treningi )\n"
           "\n");
    scanf("%d", &palwybor);

    if (palwybor == 1) {
        pal = 1.545;
    } else {
        if (palwybor == 2) {
            pal = 1.845;
        } else {
            if (palwybor == 3) {
                pal = 2.2;
            }
        }
    }
    return pal;
}

Od razu to lepiej wygląda i widać więcej problemów

1. Zmienne globalne to samo zło
2. Nazwy funkcji są mało zrozumiałe
3. Konwencja nadawania nazw funkcji jest niekonsekwentna. Powinno być małymi literami z "camel case" (tylko pierwsze litery wyrazów powinny być duże).
4. main trochę za długie
5. liczby magiczne
6. Część UI-owa pomieszana z logiką.

A tak ogólnie, to jak na pierwszy program większy od "hello world" to całkiem nieźle (większość początkujących pisze o wiele bardziej koślawie, a potem wychodzą na ludzi).

Te magiczne liczby to liczby stałe ?? Dzięki za rady zastosuje :)

Czy nie powinno być bmr_b = 6.25 zamiast bmr_b = 6,25???

@koszalek-opalek: racja
Zresztą kompilator wykrywa dużo więcej problemów, a wynik działania też jest bezsensu (za szybko pochwaliłem):
https://wandbox.org/permlink/PdWmCX5wnpgt0vHF

Twoj wzrost - 142.00
Twoja waga - 123.00
Twoje BMI - 0.01
wyglodzenie

:), chyba jakiś standard amerykański mimo że w kilogramach a nie funtach :)

@MarekR22: Ale akurat przecinka kompilator nie wykryje, bo 6,25 jest poprawne -- tylko nie równa się 6.25 ale 25... :)

Moja sugestia jest taka, aby na tym etapie nauki w ogóle nie zawracać sobie głowy opiniami innych osób, a JEDYNĄ rzeczą, jaką należy się na tym etapie przejmować, to czy program działa poprawnie. Podział na funkcje jest - ok - więcej w kwestii dobrych praktyk na ten moment nie trzeba.
Jak się próbuje nauczyć za dużo na raz, to skutki są zwykle nieszczególne - głównie zniechęcenie.

Wczoraj jeszcze troszkę posiedziałem wziąłem sobie do serca rady i chyba jest lepiej :)
Ps. ktoś wytłumaczy w prosty sposób czemu zmienne globalne to takie zło, jak dla mnie na tym etapie to złoto jest :)

#include <stdio.h>
#include <stdlib.h>

typedef float f;
typedef int i;
typedef const int ci;
typedef const float cf;

void powitanie();     // funkcja witajaca uzytkownika
void podsumowanie();  // podsumowanie dla BMI

i Cel();   // cel stosowania diety
i Dane();  // dane od uzytkownika

f Aktywnosc();  // wspolczynnik aktywnosci
f Ppm();        // podstawowo przemiana materii
f Bmi();        // wskaznik masy ciala
f Cpm();        // calkowita przemiana materii

i a = 0;
i odp, plec, wiek, wybor_aktywnosci, cel;

f waga, wzrost, wzrost_x_dwa, wskaznik_masy_ciala, ppm_kobieta, ppm_mezczyzna,
    aktywnosc_user, cpm_user;

cf bmr_a = 10;    // 1 stała wzoru
cf bmr_b = 6.25;  // 2 stala wzoru
cf bmr_c = 5;     // 3 stala wzoru

cf niska_aktywnosc = 1.545;
cf srednia_aktywnosc = 1.845;
cf wysoka_aktywnosc = 2.2;

int main() {
  powitanie();

  if (a == 1) {
    Bmi();
    system("cls");
    printf("\nTwoje BMI to: %f \n\n", wskaznik_masy_ciala);
  } else {
    if (a == 2) {
      Dane();
      Ppm();
    }

    if (a == 3) {
      Dane();
      Aktywnosc();
      Ppm();
      Cel();
    }

    if (a < 1 || a > 3) printf("wybrano zla opcje\n\n koniec programu");
  }

  while (a == 1) {
    printf("Czy chcesz wyswietlic podsumowanie?\n 1.tak\n 2.nie");
    scanf("%d", &odp);

    if (odp == 1) {
      podsumowanie();

      a = 0;
    }
  }
  getchar();
  return 0;
}

void powitanie() {
  printf(
      "_____________________________________________________________________"
      "___________\n");
  printf(
      "	\n \t\t\t \t Witaj \n\n\t Pomozemy obliczyc twoje "
      "zapotrzebowanie kaloryczne :)\n\n");
  printf(
      "_____________________________________________________________________"
      "__________");
  printf(
      "\nwybierz rodzaj operacji:\n \n 1. Obliczanie BMI\n 2. Dzienne "
      "zapotrzebowanie kaloryczne.\n 3. CPM - Calkowita przemiana "
      "materii.\n");
  scanf("%d", &a);
}
int Dane() {
  printf(" \n Podaj wage w kg: ");
  scanf("%f", &waga);
  printf(" \n Podaj wzrost w centymetrach: ");
  scanf("%f", &wzrost);
  printf(" \n Podaj wiek: ");
  scanf("%d", &wiek);
  printf(" \n Podaj swoja plec: \n 1.Kobieta \n 2.Mezczyzna ");
  scanf("%d", &plec);

  return waga, wzrost, wiek, plec;
}
int Cel() {
  printf(
      "Jaki jest twoj cel: \n1. Utrzymanie wagi.\n2. Redukcja wagi.\n3. "
      "Zwiekszenie wagi.\n");
  scanf("%d", &cel);

  if (cel == 1) printf("Aby utrzymac wagę musisz jesc - %f kalori", cpm_user);

  if (cel == 2) {
    cpm_user *= 0.9;
    printf("Aby zredukowac wage musisz jesc - %f kalori", cpm_user);
  }

  if (cel == 3) {
    cpm_user *= 1.12;
    printf("Aby zwiekszyc mase musisz jesc - %f kalori", cpm_user);
  }
}
float Bmi() {
  printf("Podaj wage w kilogramach: ");
  scanf("%f", &waga);
  printf("podaj wzrost w metrach: ");
  scanf("%f", &wzrost);
  wzrost_x_dwa = wzrost * wzrost;
  wskaznik_masy_ciala = waga / wzrost_x_dwa;
}
void podsumowanie() {
  printf("Twoj wzrost - %.2f\n", wzrost);
  printf("Twoja waga - %.2f\n", waga);
  printf("Twoje BMI - %.2f\n", wskaznik_masy_ciala);

  if (wskaznik_masy_ciala < 17)

  {
    printf("wyglodzenie");
  } else

  {
    if (wskaznik_masy_ciala > 17 && wskaznik_masy_ciala <= 20) {
      printf("niedozywienie");
    }

    if (wskaznik_masy_ciala > 20 && wskaznik_masy_ciala <= 25) {
      printf("BMI w normie");
    }

    if (wskaznik_masy_ciala > 25 && wskaznik_masy_ciala <= 30) {
      printf("nadwaga");
    }

    if (wskaznik_masy_ciala > 30 && wskaznik_masy_ciala <= 35) {
      printf("I-stopien nadwagi");
    }

    if (wskaznik_masy_ciala > 35 && wskaznik_masy_ciala <= 40) {
      printf(" II stopien nadwagi");
    }

    if (wskaznik_masy_ciala > 40) {
      printf(" III stopien nadwagi");
    }
  }
}
float Ppm() {
  if (plec == 1) {
    ppm_kobieta = bmr_a * waga + bmr_b * wzrost - bmr_c * wiek - 161;
    printf("Twoje BMR - %f", ppm_kobieta);
    cpm_user = ppm_kobieta * aktywnosc_user;
  } else {
    if (plec == 2) {
      ppm_mezczyzna = bmr_a * waga + bmr_b * wzrost - bmr_c * wiek + 5;
      printf("Twoje BMR - %f", ppm_mezczyzna);
      cpm_user = ppm_mezczyzna * aktywnosc_user;
    }
  }
  return ppm_mezczyzna;
  return cpm_user;
}
float Aktywnosc() {
  printf(
      "\nMusisz wybrac swoj wspolczynnik aktywnosci: \n1. Niska aktywnosc "
      "(wiekszosc osob nie trenujacych.\n2. Srednia aktywnosc (wiekszosc "
      "trenujacych 3-5 x w tygodniu)\n3. Wysoka aktywnosc (Praca fizyczna + "
      "regularne treningi )\n\n");
  scanf("%d", &wybor_aktywnosci);

  if (wybor_aktywnosci == 1) {
    aktywnosc_user = niska_aktywnosc;
  } else {
    if (wybor_aktywnosci == 2) {
      aktywnosc_user = srednia_aktywnosc;
    } else {
      if (wybor_aktywnosci == 3) {
        aktywnosc_user = wysoka_aktywnosc;
      }
    }
  }
  return aktywnosc_user;
}

Wg mnie to co robisz tutaj:

typedef float f;
typedef int i;
typedef const int ci;
typedef const float cf;

...jedynie wszystko zaciemnia. Kiedy pierwszy raz spojrzałem na Twój kod to kompletnie nie wiedziałem co jest zwracane przez funkcje. Nazwa typów danych w ogóle ich nie opisuje.

Czemu zmienne globalne są złe? Zostało to opisane już w sieci mnóstwo razy np. tutaj: http://wiki.c2.com/?GlobalVariablesAreBad
Szczerze mówiąc nawaliłeś tyle tych zmiennych globalnych, że głowa mała :-)

Co do nazw funkcji: powinny opisywać czynność wyrażoną w czasowniku np. CreateMenu albo ComputeBMI.

Noooooo i Twoje funkcje wiele razy niczego nie zwracają pomimo tego, że powinny np: float Bmi(). Generalnie warto zwracać uwagę na warningi, które wypluwa kompilator, bo nie wierzę, że nie pokazał żadnych informacji o braku returna.

Coredump napisał(a):

Moja sugestia jest taka, aby na tym etapie nauki w ogóle nie zawracać sobie głowy opiniami innych osób, a JEDYNĄ rzeczą, jaką należy się na tym etapie przejmować, to czy program działa poprawnie. Podział na funkcje jest - ok - więcej w kwestii dobrych praktyk na ten moment nie trzeba.
Jak się próbuje nauczyć za dużo na raz, to skutki są zwykle nieszczególne - głównie zniechęcenie.

Nie wazne, ze scialem dwa znaki i skosilem sasiadowi plot, wazne ze dojechalem do domu. Dopoki nikogo nie rozjade to jest ok.
Nie potrzebne zmienne globalne, duplikowanie kodu,niewlasciwe zwracnie wartosci, zaciemnianie poprzez dziwne typedefy i nazwy zmiennych, etc.