7
\$\begingroup\$

I would like some help condensing this code down to size. I am trying to utilize Qt's container classes as efficiently as possible. I do not believe this code reflects my objective.

#include <QCoreApplication>
#include <QDebug>

int main(int argc, char *argv[])
{
    QCoreApplication a(argc, argv);

    QVector<int> vect;

    vect.append(0);
    vect.append(0);
    vect.append(0);
    vect.append(0);
    vect.append(1);
    vect.append(1);
    vect.append(2);
    vect.append(1);

    QVectorIterator<int> ivect(vect);

    double median = 0;
    if(vect.count() % 2 == 0){

        median =
        static_cast<double>((vect[vect.count() / 2 - 1] + vect[vect.count() / 2])) / 2;
    }
    else{

        median = vect[vect.count() / 2];
    }

    int sum = 0;
    while(ivect.hasNext()){

        sum += ivect.next();
    }

    double mean = static_cast<double>(sum) / vect.count();

    QSet<int> set;

    ivect.toFront();
    while(ivect.hasNext())
    {
        set << ivect.next();
    }

    QMap<int, int> map;

    QSetIterator<int> iset(set);

    ivect.toFront();
    while(iset.hasNext())
    {
        int count = 0;
        while(ivect.hasNext())
        {
            if(iset.peekNext() == ivect.next())
            {
                count++;
            }
        }
        ivect.toFront();
        map.insert(iset.next(), count);
    }

    QMapIterator<int, int> imap(map);

    int maxValue = 0;
    QString mode;
    while(imap.hasNext())
    {
        imap.next();
        if(imap.value() > maxValue)
        {
           maxValue = imap.value();
           mode = QString::number(imap.key());
        }
        else if(imap.value() == maxValue)
        {
            mode = "NULL";
        }
    }

    qDebug() << set;
    qDebug() << map;
    qDebug() << "Mean:" << mean;
    qDebug() << "Median:" << median;
    qDebug() << "Mode frequency:" << maxValue;
    qDebug() << "Mode:" << mode;

    return a.exec();
}
\$\endgroup\$
4
\$\begingroup\$

You might have decided to use Qt, but that doesn't mean you must forsake all that standard C++ has to offer.
Actually, Qt does its best to be as compatible as possible, just to make that possible.

So, let's see:

  1. Use an initializer-list for the vector.
  2. Don't be afraid of using a conditional-operator where it makes things easier or at least shorter without becoming cryptic.
  3. const-qualifying as much as possible helps catch errors.
  4. Your median-calculation (though not the median with that example-data) is wrong, you have to sort first.
  5. You really want to accumulate in a double using the standard algorithm, for brevity and correctness. As a bonus, you can make it const too.
  6. As the vector is sorted, the problem of finding the mode is equivalent to finding the longest consecutive run, which can be solved far more efficiently than with a map.
  7. median and mean assume a non-empty input, not so mode, though that will only calculate the first mode.

(I only proved it right, I didn't run it.)

#include <QCoreApplication>
#include <QDebug>
#include <algorithm>
#include <numeric>

int main(int argc, char *argv[])
{
    QCoreApplication a(argc, argv);
    QVector<int> vect = { 0, 0, 0, 0, 1, 1, 2, 1 };
    std::sort(vect.begin(), vect.end());
    const auto median = vect.size()%2
        ? vect[vect.size() / 2]
        : ((double)vect[vect.size() / 2 - 1] + vect[vect.size() / 2]) * .5;
    const auto mean = std::accumulate(vect.begin(), vect.end(), .0) / vect.size();
    const auto mode = [&]{
        size_t bestC = 0, tmpC = 0;
        int    bestV = 0, tmpV = 0;
        auto apply = [&](int next){
            if(tmpC > bestC)
                bestC = tmpC, bestV = tmpV;
            tmpC = 1, tmpV = next;
        };
        for(int x : vect)
            if(c == tmpV)
                tmpC++;
            else
                apply(x);
        apply(0);
        return bestC ? QString::number(bestV) : "NULL";
    }();
    qDebug() << "Mean:" << mean;
    qDebug() << "Median:" << median;
    qDebug() << "Mode:" << mode;
    return a.exec();
}
\$\endgroup\$
  • 1
    \$\begingroup\$ accululate is defined in numeric \$\endgroup\$ – vlad_tepesch Mar 6 '17 at 11:29
4
\$\begingroup\$

Sorted vs Unsorted

Your QVector sequence contains an unsorted sequence of values. Your median calculation has a precondition that your sequence is sorted. Consider the sequence 1, 0, 1, what is the median?

Small, Focused, and Testable Functions

Prefer writing functions that employ the single responsibility principle. Splitting out median and testing it with different sequence types would have pointed out a problem. How about calculating the mean of an empty container? Is division by zero a problem?

Standard Algorithms

Being able to understand when you should use standard algorithms and applying them is very important. Mean can be written using std::accumulate(). Median can be written in terms of std::nth_element(). Mode can be written using boost::bimap (or utilize std::unordered_map+std::multimap) and std::upper_bound() with std::transform() to return the modes of a sequence. Not all sequences are uni-modal.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.