1
\$\begingroup\$

This code sets and shows an image file on a QGraphicsView.

I'm interested in seeing how to improve readability and reduce redundancy.

void QtReader::addPadding(const int& querybase, const int& answerbase, QString &target){
    if (padding){
        if (querybase >= 10){
            target =  "0" + QString::number(answerbase);
        }
        else if (querybase < 10){
            target =  "00" + QString::number(answerbase);
        }
        else{
            target = QString::number(answerbase);
        }
    }
    else{
        target = QString::number(answerbase);
    }
}

void QtReader::getArchiveList(const QString& dpath){
    ui->comboBox->clear();
    dirpath = dpath;
    QDir dir(dpath);
    QStringList list = dir.entryList(QDir::Dirs|QDir::NoDotAndDotDot);
    if (list.count() >= 1){
        foreach (QString i, list){
            QString n = QFileInfo(i).fileName();
            ui->comboBox->addItem(n);
        }
    }
}

void QtReader::getFileList(const QString& path){
    test.clear();
    ext << "*.jpg" << "*.png" << ".bmp";
    QString l_path(path);
    QDir *dir = new QDir(l_path);
    dir->setFilter(QDir::Files|QDir::NoDotAndDotDot);
    dir->setNameFilters(ext);
    test = dir->entryList(QDir::Files|QDir::NoDotAndDotDot);
    max = test.length();
    delete dir;
}

void QtReader::setIfExtension(const QString& querybase, const QString& answerbase, QString &target){
    if (QFile(querybase + JPG).exists()){
        target = answerbase + JPG;
    }
    else if (QFile(querybase + PNG).exists()) {
        target = answerbase + PNG;
    }
}

void QtReader::showScene(QGraphicsScene* targetScene, QGraphicsView* target, QString file, const int page){
    targetScene->addPixmap(file);
    SceneVect.push_back(targetScene);
    target->setScene(targetScene);
    /*QGraphicsTextItem *pageText = new QGraphicsTextItem;
    pageText->setPos(0,10);
    if (!file.isEmpty()){
        pageText->setPlainText(QString::number(page+1));
    }
    target->scene()->addItem(pageText);*/
}

void QtReader::determineImage(const int& page, bool mP){
    vectDelete(false);
    if (max > 0){
        gpage = page;
        QGraphicsScene* sLeft = new QGraphicsScene(this);
        QGraphicsScene* sRight = new QGraphicsScene(this);
        QString left, right, pl, pr;

        if (mP){
            gpage = max;
        }

        if (padding || test.contains("1.jpg") || test.contains("1.png")){
            QString leftbase, rightbase;
            if (!mP){
                addPadding(gpage + 1, gpage + 1, left);
                addPadding(gpage + 2, gpage + 2, right);
                leftbase = basepath + left;
                rightbase = basepath + right;
            }
            else if (mP){
                addPadding(gpage - 1, gpage - 1, left);
                addPadding(gpage, gpage, right);
                leftbase = basepath + left;
                rightbase = basepath + right;
            }
            setIfExtension(leftbase, leftbase, pl);
            setIfExtension(rightbase, rightbase, pr);
        }
        else {
            if ((page < max) && (!mP)){
                pl =  basepath + test[page];
                if (page + 1 < max){
                    pr = basepath + test[page + 1];
                }
                else{
                    pr.clear();
                }
            }
            else if (mP){
                pl = basepath + test[max - 2];
                pr = basepath + test[max - 1];
            }
        }

        if (((max % 2) > 0) && mP){
            pl = pr;
            pr.clear();
            gpage = gpage - 1;
        }

        if (mP || ((gpage + 2) > max)){
            setWindowTitle(prefix + QString::number(max) + extension );
        }
        else if((gpage + 2) <= max){
            setWindowTitle(prefix + QString::number(page + 2) + extension );
        }

        if (this->isFullScreen() && ui->mainToolBar->isVisible() && (page >= max) && (page != 0)){
            ui->mainToolBar->hide();
        }

        showScene(sLeft, ui->vLeft, pl, gpage);
        showScene(sRight, ui->vRight, pr, gpage + 1);

        leftEmpty = false;
        if (!pl.isEmpty()){
            rightEmpty = false;
        }

        saveState(gpage);

        ui->vLeft->fitInView(ui->vLeft->sceneRect(), Qt::KeepAspectRatio);
        ui->vRight->fitInView(ui->vRight->sceneRect(), Qt::KeepAspectRatio);
    }
}

Header File:

private:
    Ui::QtReader *ui;
    void addPadding(const int& querybase, const int& answerbase, QString &target);
    void Clear();
    void determineImage(const int& page, bool mP);
    void getArchiveList(const QString& dpath);
    void getFileList(const QString& path);
    void showScene(QGraphicsScene* targetScene, QGraphicsView* target, QString file, const int page);
    void setIfExtension(const QString& querybase, const QString& answerbase, QString &target);
    bool leftEmpty, rightEmpty, padding;
    int gpage, max, v;
    QString basepath, dirpath, extension, series, volume;
    QStringList ext, list, test;
    QVector<QGraphicsScene*> SceneVect;

Variable Declaration

const QString prefix = "QtReader - [";
const QString JPG = ".jpg";
const QString PNG = ".png";
int gpage = 0;
int max = 0;
int v = 0;
\$\endgroup\$
  • 4
    \$\begingroup\$ Hi, could you please edit the question to add a description of what your code does? Take a look at How to Ask \$\endgroup\$ – jacwah Mar 31 '16 at 10:08
  • 1
    \$\begingroup\$ Preferably you should give us enough code to compile and run the code you have so far. \$\endgroup\$ – Nobody Mar 31 '16 at 10:12
  • \$\begingroup\$ And what you mean by "improve" ? Readability ? Speed ? Memory ? Security ? \$\endgroup\$ – Garf365 Mar 31 '16 at 10:21
  • 2
    \$\begingroup\$ It's not a requirement that you provide everything needed to compile the code. \$\endgroup\$ – 200_success Mar 31 '16 at 10:34
  • \$\begingroup\$ It would be helpful if you also added the header file for this class. Right now there are references to data members in the code without a way to see their type etc. \$\endgroup\$ – jacwah Mar 31 '16 at 11:17
1
\$\begingroup\$

I'm sorry for being blunt, but your code is hard to read. The main reason for this is that you consistently use arcane names like test, list, ext and v for data members, which leads to confusion as it's really hard to understand what they represent. Instead, use descriptive names, both for data and functions. Think of an outsider who hasn't written the code themselves, how would you explain each variable and function to them? On top of helping others understand, you will also help yourself a couple of months from now.

Sometimes it's okay to use shorter names for variables, like list in getArchiveList. Because the scope is very limited, it doesn't interfere with readability. The problem with list is that it shadows the data member with the same name, again making the code confusing.

Now let's talk about determineImage. I have no idea what this function does, but it's clear that the name doesn't describe this adequately. In fact, it's probably doing too much as it's quite long. Try splitting it up into smaller functions with names that explain their purpose. In addition, give the local variables better names than lr etc.

On another note: commented out code. I consider leaving commented out lines in your code without explanation a bad practise. Let me quote @nhgrif in this answer:

Source control should help you keep track of code that used to be there, so there's not a real good excuse to leave it there for any historical reason.

Arguably, you might want to leave it in if it's something you're frequently uncommenting for some sort of testing purposes, but if that's the case, perhaps leave a comment above the line, something to the effect of:

// Uncomment the following line to ... 

Because you use source control, right?

\$\endgroup\$
  • \$\begingroup\$ I do use source control. I was prototyping something in the function. \$\endgroup\$ – Akito Mar 31 '16 at 12:06

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.