Losowanie bez powtorzen - zadanie

Question

Zadanie: Napisz program, który wczyta 3 liczby podane przez użytkownika do tablicy, a następnie wylosuje 2 z nich bez powtórzeń. Wynik wypisz na ekran. Postaraj się napisać ten program w oparciu o funkcje. 

Czesc mam pytanie czy ten kod jest dobrze napisany bo zroilem pare testow i losuje dwie rozne liczby z tych liczb ktore uzytkownik wprowadza ale jest jeden problem wyspuje sie wtedy kiedy podaje dwie te same liczby ?

Kod:

#include <iostream>
#include <cstdlib>
#include <ctime>


using namespace std;

bool czy_byla(int liczba, int tab[], int ile) {

    int i=0;
    do {
        if(liczba == tab[i]) return true;
        i++;
    }while(i<ile);

    return false;
}

int losuj() {

    return rand()%2;
}

int main() {

    srand(time(NULL));
    int tab[3];
    cout << "Wypisz trzy liczby: ";
    for(int i=0; i<3;i++) {
        cin>> tab[i];
    }
    int it;
    int tab2[2];
    int i=0;
    do {
        it = losuj();
        if(czy_byla(tab[it], tab2, i) == false) {
            tab2[i] = tab[it];
            i++;
        }

    }while(i<2);

    for(int s=0; s<2; s++) {
        cout << tab2[s]<< " ";
    }
    return 0;
}

 

Answer 1

Hmm, a czego się spodziewasz gdy podasz takie same liczby do losowania :P? Od strony programisty mógłbyś jedynie zrobić walidacje liczb podawanych przez użytkownika - tzn tuż po wczytaniu sprawdzasz czy są one rozne, jesli nie są to prosisz o podanie roznych liczb i wczytujesz jeszcze raz.

A co do kodu to jest kilka błędów i dziwnych rzeczy:

1. To dziwne do while w funkcji czy_byla powoduje, że jeśli zmienna ile wynosi zero to i tak sprawdzisz zerowy element. Ba - nawet jesli zmienna ile wynosi -345345 tez sprawdzisz zerowy element. Ta pętla do while nie dosc ze tam logicznie nie pasuje to jeszcze źle działa. Zawsze gdy wiesz ile razy będziesz iterował używaj zwykłej pętli for.
2. Mam nadzieję, że implementacja funkcji czy_byla to jedynie Twój własny wybór, żeby poćwiczyć algorytmy, ale normalnie użyłbyś std::find, bo Twojego ifa można strescic do tego:

   if(find(begin(tab2),end(tab2),tab[it]) == end(tab2))
   

3. Drugi do-while tez nie jest w zaden sposob uzasadniony. Generalnie zasady w wyborze pętli sa takie:
   - uzyj gotowców z <algorithm>  jesli sie da
   - jesli wiesz ile razy iterujesz to zawsze for
   - jesli nie wiesz ile razy będziesz iterowal to while
   - jesli nie wiesz ile razy, ale niezaleznie od warunku potrzebujesz wykonac cialo petli co najmniej raz to do-while
4. Ucz się od samego początku używac C++'owej biblioteki a nie staroci z C. Czyli zamiast:

   srand(time(NULL));
   int losowa_wartosc = rand() % 2;

   powinienes zrobic:
    

   	std::random_device r; //generator losowego seeda
   	//jego wywolanie to odpowiednik time(NULL)
   	std::default_random_engine engine(r()); //poniekad odpowiednik srand
   	std::uniform_int_distribution dist(0, 2); 
       //wywolania dist(engine) zwracaja inta [0,2]
   	int value = dist(engine); //odpowiednik rand() % 2

5. Nazywaj zmienne zgodnie z ich przeznaczeniem. Tak ja na szybko bym zmienił Twoje brzydkie nic nie mowiace nazwy:
   - tab - liczby
   - tab2 - dotychczas_wylosowane
   - it - losowy_indeks

 

Answer 2

Zamiast sprawdzać, czy wylosowałeś już daną wartość liczbową, sprawdź, czy wylosowałeś już dany indeks.