Przejrzenie i zoptymalizowanie mojego kodu

0

Witam, napisałem prostą gierkę, w której chodzi o to żeby użytkownik zgadł jaką liczbę wylosował komputer. Dopiero się uczę programować w C++ i chciałbym żebyście przejrzeli mój kod i dali parę rad odnośnie tego co tam napisałem, co można zapisać łatwiej, czyli optymalizacja :)

Chciałbym zaznaczyć, że jestem laikiem jeśli chodzi o programowanie, przerabiam Symfonie C++ Standard (aktualnie zatrzymałem się na tablicach) i po prostu chciałbym usłyszeć parę rad od was :) Byłoby miło!

Oczywiście gra działa tak jak tego chciałem, nie mogę się przyczepić do niczego, w niedalekiej przyszłości (jak już będę wiedział jak to zrobić) zabezpieczę ją przed ewentualnym wpisywaniem liter itp. :)

Krótki opis: początek programu to tzw. "menu" gier, które będę rozwijał w przyszłości o inne pozycje, które napisze :P Gdy użytkownik wybierze w jaką grę chce zagrać, przekierowuje go do niej (w tym wypadku do loterii). Użytkownik w grze Loteria musi zgadnąć liczbę jaką wylosował komputer z przedziału 1-20, a gdy zgadnie ma wybór czy grać dalej czy skończyć :)

Pewnie powiecie, że nie ma się czym szczycić, ale jak dla mnie to nie dość, że była super zabawa, to jeszcze traktuje to jako duży sukces :) W końcu dopiero się uczę, a napisanie takiego kodu od zera to dla mnie (na obecną chwilę) coś ekstra.

Liczę na was :)

Oto kod:
loteria.hpp

#ifndef LOTERIA_HPP_INCLUDED
#define LOTERIA_HPP_INCLUDED
#define ANIM(tab,a,predk) for(int i = 0; i < a; i++) { Sleep(predk); cout<<tab[i]<<" "; }

using std::cout;
using std::cin;
using std::endl;
using std::string;

//ANIM(tekst,tekst.length());
//ANIM(brw,7);

void los_main(), j_raz(), los(), petla(), podaj();
extern int liczba, wls, wls_p, licznik;
extern bool start;
extern char wybor;

#endif

 

loteria.cpp

#include <iostream>
#include <windows.h>
#include <ctime>
#include <cstdlib>
#include "loteria.hpp"

//---------------------------------------------------------------------------
int liczba, wls, wls_p;
bool start;
char wybor;
//---------------------------------------------------------------------------
int licznik = 0;
string brw = "BRAWO!!!!";
string mys = "------------------------------------";
//---------------------------------------------------------------------------


void los_main()
{
    start = true;
    los();
    podaj();
    petla();

    cout<<"\nGrasz jeszcze raz? (y/n)\n";
    cout<<"Wybor: ";
    cin>>wybor;

    while(wybor=='y' || wybor == 'Y') j_raz();


    cout<<"\nDzieki za gre! :)\n";
    system("PAUSE");
}


void j_raz()
{
        ANIM(mys,mys.length(),50);
        cout<<endl;

        los();
        podaj();
        petla();
}


void los()
{
    srand( time( NULL ) );
    int wls = (std::rand() % 20 ) + 1;
    wls_p=wls;
}

void podaj()
{
    if (start == false)
    {
        cout<<"Dawaj smialo jeszcze raz: ";
    }
    else if (start == true)
    {
        cout<<"Program wylosowal liczbe i musisz ja odgadnac! :)\nDawaj smialo: ";
    }

    cin>>liczba;
    cout<<endl;
}

void petla()
{
    start = false;
    while (liczba)
    {
        if (liczba>wls_p)
        {
            ++licznik;
            cout<<"Za duza liczba!\n";
            podaj();
        }

        else if (liczba<wls_p)
        {
            ++licznik;
            cout<<"Za mala liczba!\n";
            podaj();
        }

        else
        {
            ++licznik;
            ANIM(brw,brw.length(),200);
            cout<<"\nTrafiles za " <<licznik<< " razem! :)\n";
            licznik = 0;
            break;
        }
    }

}

 

main.cpp

 
#include <iostream>
#include <cstdlib>
#include <windows.h>
#include "loteria.hpp"

int main()
{

        int menu;

        cout<<"------------MENU GIER----------------\n"
        <<"1. Loteria - komputer losuje, a ty zgadujesz!\n"
        <<"2. ------||------\n"
        <<"3. ------||------\n"
        <<"4. ------||------\n"
        <<"Wybieram: ";
        cin>>menu;

        cout<<endl<<endl;

        switch(menu)
        {
            case 1:
            {
                los_main();
                break;
            }
            default:
            {
                break;
            }
        }
        cout<<"\n";
        system("PAUSE");
}
6
#define ANIM(tab,a,predk) for(int i = 0; i < a; i++) { Sleep(predk); cout<<tab[i]<<" "; }

Należy ograniczać makra do minimum, takie coś nadaje się na funkcję, nie makro.

using std::cout;
using std::cin;
using std::endl;
using std::string;

Absolutnie nie należy używać using w plikach nagłówkowych. Natomiast w pliku .cpp już wolałbym using namespace std niż takie wyliczanki.

extern int liczba, wls, wls_p, licznik;
extern bool start;
extern char wybor;
...
int licznik = 0;
string brw = "BRAWO!!!!";
string mys = "------------------------------------";

Nie należy używać zmiennych globalnych.

void los_main(), j_raz(), los(), petla(), podaj();

Deklaracji funkcji raczej się nie łączy, każda deklaracja w osobnej linijce.

void los_main();
void j_raz();
if (start == true)

Po co?

default:
{
    break;
}

Po co?

0
twonek napisał(a):

Nie należy używać zmiennych globalnych.

Dlaczego? Przecież gdybym zastosował zmienne lokalne, to nie mógłbym się odnieść do tych zmiennych w większości funkcjach..

twonek napisał(a):

if (start == true)

Po co?

Tu akurat chodzi o to, że nie będzie wyświetlało za każdym kolejnym podejściem "Program wylosowal liczbe i musisz ja odgadnac".

twonek napisał(a):

default:
{
break;
}

Po co?

Opisałem to w pierwszym poście, jeśli wybierzemy coś innego niż jeden to po prostu przerywa prace programu.

3

Dlaczego? Przecież gdybym zastosował zmienne lokalne, to nie mógłbym się odnieść do tych zmiennych w większości funkcjach..
Zmienne globalne to bardzo mocny antypattern, ponieważ zaciemniają kod oraz często są źle nazwane. Generalnie Twój kod powinien być podzielony na namespace'y oraz klasy, w obecnej formie jest kompletnie nieczytelny.

Tu akurat chodzi o to, że nie będzie wyświetlało za każdym kolejnym podejściem "Program wylosowal liczbe i musisz ja odgadnac".
Nie o to chodzi - dlaczego piszesz == true?
Wystarczy samo if (cośtam).

Opisałem to w pierwszym poście, jeśli wybierzemy coś innego niż jeden to po prostu przerywa prace programu.
Nie, nie. Po prostu default: break; nie robi absolutnie nic.
break; służy do wyjścia z warunku switcha (w tym kontekście), więc to tak, jakbyś napisał:

if (cośtam) {
}

i zostawił to w takiej formie - także nic nie robi.

3

Sposób w jaki to napisałeś, sprawia, że użycie zmiennych globalnych jest mniej lub bardziej niezbędne. Natomiast dlaczego jest to złe? Twoja gierka ma kilkadziesiąt linijek, ale jeżeli miała by kilka tysięcy+ to a)trudno by było zapanować nad stanem b) zmienne globalne mają ogromną wadę: są globalne :) więc możesz się do nich odwołać z każdego miejsca w programie co jest pokusą, której ciężko się oprzeć, tym kod który piszesz, jest a) zagmatwany, b)sprzyja silnym powiązaniom - jest mało elastyczny, i nie odporny na zmiany.
Ale spokojnie :) Jak na początek jest nieźle. Musisz teraz podszkolić się z OOP'u,, i przepisać swoją gierkę by spełniała jego podstawowe zasady. Na start skup się na koncepcji hermetyzacji :). Powodzenia.

5
szym3ns napisał(a):

Dlaczego? Przecież gdybym zastosował zmienne lokalne, to nie mógłbym się odnieść do tych zmiennych w większości funkcjach..

Doprawdy:

int los()
  {
   //srand(time(0)); // to idzie do main
   return 1+std::rand()%20;
  }

void petla(int liczba)
  {
   unsigned licznik=0;
   cout<<"Program wylosowal liczbe i musisz ja odgadnac! :)\nDawaj smialo: ";
   for(int input;;++licznik)
     {
      cin>>input;
      if(input>liczba) cout<<"Za duza liczba!\n";
      else if(input<liczba) cout<<"Za mala liczba!\n";
      else break;
      cout<<"Dawaj smialo jeszcze raz: ";
     }
   ANIM(brw,brw.length(),200);
   cout<<"\nTrafiles za " <<licznik<< " razem! :)\n";
  }

void los_main() // tu żadnych powtórek z menu użytkownik powtórzy jeszcze raz
  {
   podaj(los());
   cout<<"\nDzieki za gre! :)\n"; 
  }
szym3ns napisał(a):

Tu akurat chodzi o to, że nie będzie wyświetlało za każdym kolejnym podejściem "Program wylosowal liczbe i musisz ja odgadnac".

start ma wartość true lub false ale napisać if(start) nie możemy więc najpierw sprawdzamy start==true ale
start==true ma wartość true lub false ale napisać if(start==true) nie możemy (z tego samego powodu co wyżej) więc najpierw sprawdzamy (start==true)==true ale
(start==true)==true ma wartość true lub false ale napisać if((start==true)==true) nie możemy (z tego samego powodu co wyżej) więc najpierw sprawdzamy ((start==true)==true)==true ale ... mniej niż 32 razy nie wypada.
Normalni ludzie piszą tak:

if(start) do_something();
else do_something_else();
szym3ns napisał(a):

Opisałem to w pierwszym poście, jeśli wybierzemy coś innego niż jeden to po prostu przerywa prace programu.
jak nic nie robisz w default to na to samo wyjdzie jak go nie ma.

0

Może dodaj sprawdzanie czy wprowadzona przez użytkownika liczba jest mniejsza czy większa od wylosowanej?
I wyświetl tą podpowiedź w konsoli. Chyba nic trudnego a znacznie uatrakcyjni zgadywankę

1 użytkowników online, w tym zalogowanych: 0, gości: 1