Losowanie liczb z zakresu i obliczenie ich średnij

Mam problem z działaniem kodu ,który stworzyłem. Program ma za zadanie wylosować taką ilość liczb jaką podamy z klawiatury (n) z zakresu <15,67> a następnie wypisać je na ekran i policzyć średnią z nich. Co tutaj jest nie tak

#include <iostream>
#include <math.h>
#include <cstdio>
#include <cstdlib>
#include <ctime>
using namespace std;

double srednia(int n,double elementy[]);


int main(int argc, const char * argv[]) {
	srand( time( NULL ) );
    int n;
	double * elementy = new double[n];
    cout<<"Ile losowac? :";
    cin>>n;
    cout<<srednia(n,elementy)<<endl;
	for(int i=0;i<n;i++) {
		cout<<elementy[i];
	}
    system("pause");
    return 0;
}

double srednia(int n,double elementy[]) {
    double srednia=0;
    for(int i=0;i<n;i++) {
	srednia=srednia+rand() % 53 + 15;
	elementy[i]=rand() % 53 + 15;	
    }
	return srednia/n;
}
 

?

Co tutaj jest nie tak

    double * elementy = new double[n];
    cout<<"Ile losowac? :";
    cin>>n;

UB, pierw alokujesz tablicę o UB-elementową, a potem wczytujesz n.

    srednia=srednia+rand() % 53 + 15;
    elementy[i]=rand() % 53 + 15;  

Inną liczbę bierzesz do średniej, a inną zapisujesz do tablicy.

double * elementy = new double[n];

3.1 Masz wyciek pamięci
3.2 używanie nagiego new i delete to antyidiom w C++
Użyj std::vector

4. używasz rand() zamiast nagłówka <random>.
   https://channel9.msdn.com/Events/GoingNative/2013/rand-Considered-Harmful

    for(int i=0;i<n;i++) {
        cout<<elementy[i];
    }

Nie oddzielasz niczym elementów.

system("pause");

Nieprzenośny kod

7. Polskie nazwy. Dobrym zwyczajem jest ograniczanie się do języka angielskiego w nazewnictwie.

Ja bym to zrobił tak:

	vector<double> doubles;
	int numberOfDoubles;
	cin >> numberOfDoubles;
	doubles.resize(numberOfDoubles);

	mt19937_64 gen{random_device{}()};
	uniform_int_distribution<> dis(15,67);

	generate_n(doubles.begin(), numberOfDoubles, [&]{ return dis(gen); });

	double average = std::accumulate(doubles.cbegin(), doubles.cend(), 0., plus<>{}) / numberOfDoubles;

	DBG(average);
	DBG_CONT(doubles);

http://melpon.org/wandbox/permlink/uM6SqlutY1vCJHrx

po zmianie

#include <iostream>
#include <math.h>
#include <cstdio>
#include <cstdlib>
#include <ctime>
using namespace std;

double srednia(int n,double elementy[]);


int main(int argc, const char * argv[]) {
	srand( time( NULL ) );
    int n;
    cout<<"Ile losowac? :";
    cin>>n;
	double * elementy = new double[n];

	for(int i=0;i<n;i++) {
		cout<<"Liczba " << i+1 << " jest rowna ";
		cout<< elementy[i] << endl;
}
    cout<<srednia(n,elementy)<<endl;
	delete [] elementy;
    system("pause");
    return 0;
}

double srednia(int n,double elementy[]) {
    double srednia=0;
    for(int i=0;i<n;i++) {
	elementy[i]=rand() % 53 + 15;
	srednia=srednia+elementy[i];
    }

	return srednia/n;
}
 

Ach, jeszcze jedno:
rand() % 53 + 15 wyniki będą z zakresu [15,67]. Po co się męczyć z gorszym rozwiązaniem?

kq - o taki właśnie zakres mi chodzi i dlaczego jest to gorsze rozwiązanie skoro działa ?

Tak czy inaczej, przypisujesz wartości elementom w funkcji srednia, którą wywołujesz dopiero PO wyświetleniu tablicy. To jest UB + kłamstwo w nazwie funkcji (srednia nie liczy średniej, tylko nadpisuje tablicę i liczy nową średnią).

Program się kompiluje ,i podanie średniej z wylosowanych liczb prawdopodobnie działa (nie wiem jak to sprawdzić) ,ponieważ wypisywanie wylosowanych liczb niestety jest błędne :(

kq napisał(a):

Tak czy inaczej, przypisujesz wartości elementom w funkcji srednia, którą wywołujesz dopiero PO wyświetleniu tablicy. To jest UB + kłamstwo w nazwie funkcji (srednia nie liczy średniej, tylko nadpisuje tablicę i liczy nową średnią).

Niestety mam mały problem jak to przerobić. Czy jest szansa na jakąś podpowiedź ?

Podziel srednia na 2 różne funkcje:

1. generuj - uzupełniająca tablicę (zupełnie jak generate w moim przykładzie)
2. srednia - która tylko oblicza średnią, nic nie nadpisuje.

Jeśli nie chcesz tego robić, to przenieś funkcję srednia nad pętlę wypisującą tablicę.

Rozumiem że bardzo chcesz aby poprawić to co napisałeś? Ok, choć pamiętaj takie rozwiązanie dalekie jest (bardzo) od optymalnego... i poprawki są w trybie "minimalne nie zmieniające zamysłu kodu" :-/

#include <iostream>
//#include <math.h> // XXX: Do jakich operacji ma być math? Jeśli już ma być to cmath w C++
//#include <cstdio> // XXX: A po co jak masz iostream?
#include <cstdlib>  // XXX: Jak rozumem dla system("pause");   :-/
#include <ctime>

using namespace std;
 
double srednia(int n, double elementy[]);
void wypelnij_losowo(int n, double elementy[]);
 
 
int main(void) {                // XXX: Jak nie używasz argc i/lub argv, to "zwyczajowo" void lub nic...
    srand(time(NULL));
    int n;
    cout << "Ile losowac?: ";
    cin >> n;
    double * elementy = new double[n];

    wypelnij_losowo(n, elementy);  // XXX: Zanim wyświetlisz, wypełnij tablicę.

    for(int i = 0; i < n; ++i) {   // XXX: Lepiej jest preinkrementować w takich miejscach 
        cout << "Liczba " << i + 1 << " jest rowna ";
        cout << elementy[i] << endl;
    }

    cout << "Średnia: " << srednia(n, elementy) << endl;

    // XXX: Jak zrobiłeś tablicę na stercie, to ją sprzątnij...
    delete [] elementy;

    system("pause"); // XXX: To jest straszna "popelyna", no ale rozumiem potrzebę.. 

    //return 0;   // XXX: C++ nie wymaga jak C zwracania 0 jeśli jest ok. "Sam je sobie wstawia".
}

void wypelnij_losowo(int n, double elementy[]) {
    for(int i = 0; i < n; ++i) {
        elementy[i] = rand() % 53 + 15;
    }
}

double srednia(int n, double elementy[]) {
    double suma = 0; // XXX: Nie nazywaj zmiennej srednia jak to ma byc suma.. 
    for(int i = 0; i < n; ++i) {
        suma += elementy[i];
    }
 
    return suma / n;
}

Mokrowski napisał(a):

Rozumiem że bardzo chcesz aby poprawić to co napisałeś? Ok, choć pamiętaj takie rozwiązanie dalekie jest (bardzo) od optymalnego... i poprawki są w trybie "minimalne nie zmieniające zamysłu kodu" :-/

#include <iostream>
//#include <math.h> // XXX: Do jakich operacji ma być math? Jeśli już ma być to cmath w C++
//#include <cstdio> // XXX: A po co jak masz iostream?
#include <cstdlib>  // XXX: Jak rozumem dla system("pause");   :-/
#include <ctime>

using namespace std;
 
double srednia(int n, double elementy[]);
void wypelnij_losowo(int n, double elementy[]);
 
 
int main(void) {                // XXX: Jak nie używasz argc i/lub argv, to "zwyczajowo" void lub nic...
    srand(time(NULL));
    int n;
    cout << "Ile losowac?: ";
    cin >> n;
    double * elementy = new double[n];

    wypelnij_losowo(n, elementy);  // XXX: Zanim wyświetlisz, wypełnij tablicę.

    for(int i = 0; i < n; ++i) {   // XXX: Lepiej jest preinkrementować w takich miejscach 
        cout << "Liczba " << i + 1 << " jest rowna ";
        cout << elementy[i] << endl;
    }

    cout << "Średnia: " << srednia(n, elementy) << endl;

    // XXX: Jak zrobiłeś tablicę na stercie, to ją sprzątnij...
    delete [] elementy;

    system("pause"); // XXX: To jest straszna "popelyna", no ale rozumiem potrzebę.. 

    //return 0;   // XXX: C++ nie wymaga jak C zwracania 0 jeśli jest ok. "Sam je sobie wstawia".
}

void wypelnij_losowo(int n, double elementy[]) {
    for(int i = 0; i < n; ++i) {
        elementy[i] = rand() % 53 + 15;
    }
}

double srednia(int n, double elementy[]) {
    double suma = 0; // XXX: Nie nazywaj zmiennej srednia jak to ma byc suma.. 
    for(int i = 0; i < n; ++i) {
        suma += elementy[i];
    }
 
    return suma / n;
}

Wielkie dzięki :) już teraz wiem ile błędów zrobiłem ,a co do tej preinklementacji w czym jest ona lepsza analizując ten przykład ?

Tak chyba powinien wyglądać kod o którym mówili powyżej. Jak coś źle to się czepiajcie :P

#include <iostream>
#include <vector>
#include <ctime>

using namespace std;

double generate(vector<int> &numbers, int n, int from, int to){
	srand( time(NULL) );
	for (int i=0; i<n; i++){
		numbers.push_back(rand() % (to - from) + from);		
	}

}

double average(vector<int> numbers){
	int sum = 0;
	int c = numbers.size();
	for (int i=0; i< c; i++ ){
		sum+=numbers[i];
	}
	return (double)sum/c;
}
int main() {
	int n;
	cout << "Podaj n:"<<endl;
	cin >> n;
	const int from = 15;
	const int to = 67; 
	vector<int> numbers;
	
	generate(numbers,n,from,to);
	cout << "Liczby :" << endl;
	
	for (int i=0; i < numbers.size(); i++){
		cout << numbers[i] << " ";
	}
	cout << endl;
	cout << "Srednia: " << endl;
	cout << average( numbers);

	return 0;
}

generate zadeklarowane jako zwracające wartość, nic nie zwraca (zresztą co by miało znaczyć double)? UB i bezsens.

double average(vector<int> numbers){ przyjęcie argumentu przez kopię, fuj.

numbers.push_back(rand() % (to - from) + from); off-by-one tutaj albo tutaj:

    const int from = 15;
    const int to = 67; 

Dodatkowo korzystasz z rand.

Pierdoły typu int c zamiast size_t można pominąć.

1. Owszem, zapomniałem miałem tam dodać void zamiast double - później to zauwazylem
2. Miałem przyjąć przez referencje?
3. tego nie rozumiem. Miałem zrobić global const from i to ? A jakbym chciał wykorzystać znowu tą funkcje zeby liczyła inny zakres to co wtedy?

https://en.wikipedia.org/wiki/Off-by-one_error

Losujesz <15,66>, a nie <15,67>. Nigdzie o globalnych nie pisałem.

Ahh faktycznie. A co z tym argumentem?

Przez referencję/wskaźnik. Niby możesz zwracać nowy wektor, ale to raczej mało praktyczne.