Klassen in C++ Durchschnitt von zwei Zahlen berechnen

EinNutzer0

Danke fürs Feedback/Review @DocShoe ...

Warum ich das mache? Weil ich es nicht besser wusste... Hast du einen Änderungsvorschlag? lg.

Schlangenmensch

@EinNutzer0 Naja, schreibt @DocShoe doch. im Header kein using namespace sondern std:: vor die jeweiligen Sachen schreiben. Wobei du das nur in der main() hast, daher brauchst du im Header gar kein using namespace
Die Member Funktionen, die const sein können, sollten das auch sein. Das betrifft vor allem deine Getter.
Anstelle c-Style casts gäbe es auch C++ Alternative.

Apropos c-Style Cast, noch ein paar Anmerkungen von mir:
Vielleicht kannst du dein Programm auch so ändern, das gar nicht gecasted werden muss?
Warum nimmt dein Rechner nur int entgegen? Kann es nicht sein, dass man auch einen Mittelwert von nicht ganzen Zahlen berechnen möchte?

Was soll die Konvention mit dem exact? Wenn das false ist und die Anzahl der hinzugefügten Elemente nicht mit count übereinstimmt, ist der Mittelwert einfach falsch. Warum bietest du das überhaupt an, das als API ist maximal unintuitiv.

Was soll der Konstruktor Parameter size? Von was ist das die size? Der Member heißt doch auch anders.

Warum hast du einen Destruktor definiert?

In deiner main iterierst du ja direkt über eine range und fügst die Elemente einzeln hinzu. Als Idee für eine Weiterentwicklung, vlt wäre es ja Sinnvoll, wenn dein Rechner direkt auf einer range arbeiten könnte.

Und zum Schluss: Deine Übrprüfung in der main ist für Nachvolllziehbarkeit im Forum gut, aber für dich wäre es noch ein bisschen besser, wenn du das in Unit Tests ausgliedern würdest

SeppJ

Unter der Annahme (der Code ist wegen der schlechten Bezeichner recht unlesbar, daher nur Annahme), dass der exact-Parameter irgendwie zwischen zwei unterschiedlichen Strategien wählen soll: Nicht so. if-Abfragen, um zwischen verschiedenen Strategien wählen zu können haben viele Nachteile:

Muss an vielen Stellen konsistent gehalten werden -> Katastrophe buchstäblich vorprogrammiert
Nicht erweiterbar, wenn mehr Möglichkeiten gebraucht werden
Man schleppt überall den Müll von den nicht-benutzten Strategien herum (hier z.B. avg)
Laufzeitkosten
Schlecht lesbar, da jede einzelne Funktion alle Strategien enthält, und daher jede Funktion aufgeblasen ist und der Code für die verschiedenen Strategien immer abwechselnd durchmischt ist (hier gehören z.B. Zeile 54 und 42 zusammen, dazwischen ist aber Codezeile 50, die zu 37f. gehört)

Ansonsten schreit der Numeriker in mir, dass exact einen weniger genauen Algorithmus benutzt.

EinNutzer0

Ja, ich verfolgte damit zwei unterschiedliche "Summierungsstrategien"...

Ich werde das noch mal komplett überarbeiten und melde mich später nochmal. Hohe Fehleranfälligkeit, nicht erweiterbar und unnötige Laufzeitanforderung leuchtet mir sofort ein.

EinNutzer0

Bitte nicht hauen , aber habe ich jetzt noch etwas übersehen?

#include <iostream>
#include <vector>
#include <algorithm>

class Average
{
private:
public:
    virtual void addElement(double e) = 0;
    virtual double getAvg() = 0;
};

class ExactAverage : public Average
{
private:
    uint32_t count = 0;
    double sum = 0;

public:
    ExactAverage(){};
    void addElement(double e)
    {
        count++;
        sum += e;
    }
    double getAvg()
    {
        return sum / count;
    }
};
class NormalAverage : public Average
{
private:
    const uint32_t numberOfElements;
    double avg = 0;

public:
    NormalAverage(uint32_t numberOfElements) : numberOfElements(numberOfElements){};
    void addElement(double e)
    {
        avg += e / numberOfElements;
    }
    double getAvg()
    {
        return avg;
    }
};

int main(int argc, char const *argv[])
{
    const uint32_t n = 3 * 10;
    std::vector<Average *> myAverages;
    myAverages.push_back(new ExactAverage());
    myAverages.push_back(new NormalAverage(n));
    double vals[] = {
        -23.45, 456.7, -543.21};
    for (size_t i = 0; i < 10; i++)
    {
        for (size_t j = 0; j < sizeof(vals) / sizeof(vals[0]); j++)
        {
            for_each(myAverages.begin(), myAverages.end(), [&](Average *a)
                     { a->addElement(vals[j]); });
        }
    }
    std::cout.precision(17);
    for_each(myAverages.begin(), myAverages.end(), [&](Average *a)
             { std::cout << a->getAvg() << "\n"; });
    return 0;
}

Das new geht nicht, oder?

SeppJ

@EinNutzer0 sagte in Klassen in C++ Durchschnitt von zwei Zahlen berechnen:

habe ich jetzt noch etwas übersehen?

Fettes Speicherleck, und das merkwürdige Design von ExactAverage, das wohl jedem hier ein komplettes Rätsel ist.

EinNutzer0

@SeppJ Wie geht es besser (das Design)? Abstrakte Klassen dürfen ja keine Member haben...

Quiche Lorraine

@EinNutzer0

Warum nicht so?

#include <numeric>
#include <vector>
#include <cmath>
#include <iostream>


class Average
{
private:
    size_t mCount = 0;
    double mSum = 0;

public:
    void Add(double v)
    {
        mSum += v;
        mCount++;
    }
    
    double GetAvg() const
    {
        return mSum / mCount;
    }
    
    //double GetStdDev() const ...
};

class MovingAverage
{
private:
    std::vector<double> mValues;
    size_t mSize;                       ///< Fenstergröße
    size_t mWritePos = 0;

public:
    explicit
    MovingAverage(size_t Size = 5) : 
        mSize(Size)
    {    
    }
    
    void Add(double v)
    {
        if (mValues.size() < mSize)
        {
            mValues.push_back(v);
        }
        // Haben wir bereits mSize Elemente eingelesen, so überschreiben 
        // wir den ältesten Wert.
        else
        {
            mValues[mWritePos] = v;
            mWritePos = (mWritePos + 1) % mSize;
        }
    }
    
    double GetAvg() const
    {
        return std::accumulate(std::begin(mValues), std::end(mValues), 0.0) / mValues.size();
    }
    
    //double GetStdDev() const ...
};


int main(void) {
    constexpr double Tolerance = 1E-6;
    const std::vector<double> Values {1.54, 2.12, 3.23, 4.62, 5.91, 6.13, 7.02, 8.34, 9.83};
    Average a;
    MovingAverage a2;
        
    std::for_each(std::begin(Values), std::end(Values), [&](double v) {
        a.Add(v);
        a2.Add(v);
    });     

    if (std::fdim(a.GetAvg(), 5.415555) < Tolerance)
        std::cout << "Test mit Average war erfolgreich";
    else
        std::cout << "Test mit Average war nicht erfolgreich";
    
    if (std::fdim(a2.GetAvg(), 7.446) < Tolerance)
        std::cout << "Test mit Average2 war erfolgreich";
    else
        std::cout << "Test mit Average2 war nicht erfolgreich";
    return 0;
}

wob

Ich halte das für komplett overdone. Klassenhierachie für eine Mittelwert-Berechnung? Ernsthaft?

Ich wäre da in eine ganz andere Richtung gegangen und hätte möglicherweise den Typ für die Summe getemplatet. Normalerweise sollte man dann einfach ein Average<double> wollen, aber vielleicht braucht man ja mal einen anderen Typ? Und ganz ehrlich: wenn man die Anzahl Elemente vorher kennt, dann würde ich eher einfach eine Summe bilden wollen und von Hand teilen als einem Average-Objekt das vorher mitzuteilen.

Man kann auch einfach von boost ein accumulator_set<double, stats<tag::mean>> verwenden - bei mean ist es den boost-Aufwand nicht wirklich wert, aber wenn man noch mehr über die Zahlen wissen will, die man so hat, dann könnte es lohnen Auch den Moving Average von @Quiche-Lorraine kann man damit gleich erschlagen. Siehe https://www.boost.org/doc/libs/1_80_0/doc/html/accumulators/user_s_guide.html

DocShoe

@EinNutzer0 sagte in Klassen in C++ Durchschnitt von zwei Zahlen berechnen:

@SeppJ Wie geht es besser (das Design)? Abstrakte Klassen dürfen ja keine Member haben...

Doch, dürfen sie. Ich frage mich aber, warum du überhaupt eine Klassenhierachie baust. Sollen die einzelnen Objekte/Strategien austauschbar sein? Wenn du verallgemeinern willst, dann hol´ doch noch weiter aus und nimm´ sowas als Basisklasse:

#include <cmath>
#include <memory>
#include <vector>

template<typename T>
class UnaryNumericOperation
{
public:
   UnaryNumericOperation() = default;
   virtual ~UnaryNumericOperation() = default;

   virtual T    result() const = 0;
   virtual void update( T value ) = 0;
};

class AverageCalculator ; public UnaryNumericOperation<double>
{
   double Total_ = 0;
   unsigned int Count_ = 0;
public:
   AverageCalculator() = default;

   double result() const override
   {
      if( Count_ == 0 ) return std::nan( "" );
      else              return Total_ / Count_;
   }

   void update( double value ) override
   {
      Total_ += value;
      ++Count_;
   }
};

int main()
{
   using UnaryOpsPtr_t = std::unique_ptr<UnaryNumericOperation>;
   std::vector<UnaryOpsPtr_t> ops;
   ops.push_back( std::make_unique<AverageCalculator>() );

   ...
}

Je nach Geschmack kannste die Basis- oder abgeleitete Klassen als template/konkrete Klasse implementieren, dann musste dir nur Gedanken darüber machen, wie ungültige Ergebnisse behandelt werden sollen (Ergebnis als std::optional zB).

DocShoe

@wob sagte in Klassen in C++ Durchschnitt von zwei Zahlen berechnen:

Ich halte das für komplett overdone. Klassenhierachie für eine Mittelwert-Berechnung? Ernsthaft?

Hihi, wir benutzen genau sowas. Wir haben Messdatenreihen, für die sich der Kunde passende Filter zusammenklicken kann, um sich sein Diagramm zusammenzustellen.

wob

@DocShoe sagte in Klassen in C++ Durchschnitt von zwei Zahlen berechnen:

Hihi, wir benutzen genau sowas.

Vermutlich nicht, wenn ich dich richtig verstehe. Ihr habt (vermute ich) eine Klassenhierachie für verschiedene Werte, die man berechnen kann. Nicht verschiedene Möglichkeiten, den Mittelwert zu berechnen. Also eher: mach irgendwas mit einer Zahlenfolge. -> Variante 1: Mittelwert. Variante 2: Summe. Oder so. Aber doch nicht x verschiedene Varianten für ein und denselben Wert, die ihrerseits wieder Struktur haben.

Und mir ist klar, dass man abhängig von der Datenmenge manchmal z.B. aufsummierende Varianten haben will, die nicht so exakt arbeiten wie andere (z.B. wenn man die Varianz berechnen will). Aber auch da wäre dann die Frage, ob die wirklich voneinander erben sollten (oder eine abc haben sollen). Beide könnten auch bei dir von der globalen "mach was mit Zahlen"-Klasse erben.

Generell finde ich kleinere / flachere Klassenhierachien immer besser.

SeppJ

@wob sagte in Klassen in C++ Durchschnitt von zwei Zahlen berechnen:

Ich halte das für komplett overdone. Klassenhierachie für eine Mittelwert-Berechnung? Ernsthaft?

Ich dachte es ginge um eine Demonstration, wie man so etwas stringent designt. Ob in den Funktionen, die tatsächlich etwas tun, dann ein trivialer Einzeiler oder wer weiß was für komplexe Berechnungen stattfinden, ist ja egal für die Fragestellung, wie man beispielsweise unterschiedliche Strategien für diese Berechnungen sauber implementieren kann.

Um meinen Senf dazu zu geben: Ich hätte hier die Strategien vermutlich als ein Plugin-System auf Templateebene implementiert. So a la AverageInterface<MovingAverageEngine> und AverageInterface<AccumulatingAverageEngine>. Denn wie auch schon wob und DocShoe sehe ich keinen Grund, wieso sich dies polymorph verhalten sollte. Man wird ja stets nur eine Strategie für eine Aufgabe wählen.

@EinNutzer0 sagte in Klassen in C++ Durchschnitt von zwei Zahlen berechnen:

@SeppJ Wie geht es besser (das Design)? Abstrakte Klassen dürfen ja keine Member haben...

Was besser? Weder das Speicherleck noch das komische Interface für deinen ExactAverage haben etwas mit exakten Klassen zu tun. Das erste ist einfach eine Frage der korrekten technischen Umsetzung, wie man korrektes C++ programmiert, und das zweite ist halt einfach nur komisch und keiner hier weiß, wieso du das überhaupt so gemacht hast anstatt wie bei der anderen Variante mitzuzählen.

It0101

@DocShoe sagte in Klassen in C++ Durchschnitt von zwei Zahlen berechnen:

@wob sagte in Klassen in C++ Durchschnitt von zwei Zahlen berechnen:

Ich halte das für komplett overdone. Klassenhierachie für eine Mittelwert-Berechnung? Ernsthaft?

Hihi, wir benutzen genau sowas. Wir haben Messdatenreihen, für die sich der Kunde passende Filter zusammenklicken kann, um sich sein Diagramm zusammenzustellen.

Habe sowas ähnliches auch schon bei Indikatoren im Börsenumfeld benutzt. Es gibt N-Eingangszeitreihen, M-Ausgangszeitreihen und dazwischen ist eine Blackbox. Lässt sich auch sehr gut über eine solche Klassenhierarchie abbilden. Ist im einfachsten Fall auch nur eine fortlaufende Mittelwertberechnung, wenn auch geringfügig mehr als nur Summe / Anzahl.

EinNutzer0

@Quiche-Lorraine :

Es ist nicht nötig, alle Werte zu sammeln.
Du hast eine Fallunterscheidung in der Logik, die immer wieder aufgerufen wird, unschön.

@DocShoe :

Wo ist meine Klassenhierarchie hin???
Ich hab explizit gesagt, dass ich zwei unterschiedliche Summierungsstrategien umsetzen möchte.

Erzählt mir doch nix, private Member abstrakter Klassen sind in von ihr abgeleiteten Klassen nicht sichtbar.....

DocShoe

@EinNutzer0 sagte in Klassen in C++ Durchschnitt von zwei Zahlen berechnen:

@DocShoe :

Wo ist meine Klassenhierarchie hin???
Ich hab explizit gesagt, dass ich zwei unterschiedliche Summierungsstrategien umsetzen möchte.

Lesen, nachdenken, verstehen, antworten.
Dann bau dir doch eine ExactAverageCalculator Klasse, die von UnaryNumericOperation erbt. Damit kannste noch mehr Strategien implementieren, nicht nur Mittelwertbestimmung.

@EinNutzer0 sagte in Klassen in C++ Durchschnitt von zwei Zahlen berechnen:

Abstrakte Klassen dürfen ja keine Member haben...

ist was anderes als
@EinNutzer0 sagte in Klassen in C++ Durchschnitt von zwei Zahlen berechnen:

... private Member abstrakter Klassen sind in von ihr abgeleiteten Klassen nicht sichtbar.....

EinNutzer0

Na dann erklär mal...

DocShoe

@EinNutzer0
Stell doch mal ne Frage...

Swordfish

@DocShoe sagte in Klassen in C++ Durchschnitt von zwei Zahlen berechnen:

@EinNutzer0
Stell doch mal ne Frage...

Bitte nicht :((

Quiche Lorraine

@EinNutzer0 sagte in Klassen in C++ Durchschnitt von zwei Zahlen berechnen:

Es ist nicht nötig, alle Werte zu sammeln.
Du hast eine Fallunterscheidung in der Logik, die immer wieder aufgerufen wird, unschön.

Hmm das glaube ich nicht.

Denn so ein Moving Average (Filter) berechnet den Mittelwert über die letzten N Werte und dieser wird kontinuierlich aufgerufen, heißt man steckt üblicherweise ein Wert in die Klasse und fragt danach den neuen Mittelwert ab. Steckt man also X Werte rein, so bekommt man X gemittelte Werte raus.

Der Moving Average Filter wird dazu benutzt um Datenreihen zu glätten. Ein Beispiel: Reihe {1, 0, 1, 0, 1}, N = 2

Der erste Wert ist 1, gemittelt 1.
Der zweite Wert ist 0, gemittelt (1+0)/2 = 0.5.
Der dritte Wert ist 1, gemittelt (0 + 1)/2 = 0.5.
Der vierte Wert ist 0, gemittelt (1 + 0)/2 = 0.5.
Der fünfte Wert ist 1, gemittelt (0 + 1)/2 = 0.5.

Solche geglättete Kurven sieht man z B. manchmal bei der Wetter-Temperaturprognose und bei den Grafiken zu den Corona-Fallzahlen.