Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

How can I reduce the lines I use with this code block?

if ((test.contains("1.jpg") || test.contains("1.png")) && (maxP == false)){
        page++;
        add = 1;
        if (QFile().exists(basepath + spage + ".jpg")){
            p_left = basepath + spage + ".jpg";
        }
        else if(QFile().exists(basepath + spage+ ".png")){
            p_left = basepath + spage + ".png";
        }
        if (QFile().exists(basepath + QString::number(page + 1) + ".jpg")){
            p_right = basepath + inc + ".jpg";
        }
        else if(QFile().exists(basepath + QString::number(page + 1) + ".png")){
            p_right = basepath + inc + ".png";
        }
    }
    else if ((test.contains("1.jpg") || test.contains("1.png")) && (maxP == true)){
        gpage = max;
        add = 1;
        if (QFile().exists(basepath + smax + ".jpg")){
            p_left = basepath + smax + ".jpg";
        }
        else if(QFile().exists(basepath + smax + ".png")){
            p_left = basepath + smax + ".png";
        }
        if (QFile().exists(basepath + QString::number(page + 1) + ".jpg")){
            p_right = basepath + sinc + ".jpg";
        }
        else if(QFile().exists(basepath + QString::number(page + 1) + ".png")){
            p_right = basepath + sinc + ".png";
        }
    }
share|improve this question
1  
Please, state only code purpose in the title. Also, some plaintext description of the problem at hand always helps – Caridorc Mar 5 at 10:30
up vote 7 down vote accepted

I see a number of things that may help you improve your code.

Use named constants

There are a number of constant strings which are used repeatedly. If they're used more than once, it's probably a sign that you should use named constants instead.

constexpr static char[] JPG_EXT{".jpg"};
constexpr static char[] PNG_EXT{".png"};

Consolidate tests in compound if statements

Right now the code is basically this:

if ((test.contains("1.jpg") || test.contains("1.png")) && (maxP == false)){
    // do things
} 
else if ((test.contains("1.jpg") || test.contains("1.png")) && (maxP == true)){
    // do other things
}

This can be simplified like this:

if ((test.contains("1.jpg") || test.contains("1.png")) {
    if (maxP) {
        // do other things
    } else {  // maxP must be false
        // do things
    }
}

Consolidate code where only the data changes

The code currently has repeated patterns like this:

if (QFile().exists(basepath + smax + ".jpg")){
    p_left = basepath + smax + ".jpg";
}

It then repeats, but with slightly different data. That suggests further consolidation:

if ((test.contains("1.jpg") || test.contains("1.png")) {
    QString leftbase, rightbase;
    if (maxP) {
        gpage = max;
        leftbase = basepath + smax;
        rightbase = basepath + sinc;
    } else {
        page++;
        leftbase = basepath + spage;
        rightbase = basepath + inc;
    }
    add = 1;
    if (QFile(leftbase + JPG_EXT).exists()) {
        p_left = leftbase + JPG_EXT;
    } else if (QFile(leftbase + PNG_EXT).exists()) {
        p_left = leftbase + PNG_EXT;
    }
    if(QFile(basepath + QString::number(page + 1) + JPG_EXT).exists()){
        p_right = rightbase + JPG_EXT;
    } else if(QFile(basepath + QString::number(page + 1) + PNG_EXT).exists(){
        p_right = rightbase + PNG_EXT;
    }
}

Use a function for repeated actions

If you find yourself writing similar code multiple times, it might be a sign that you could use a function. In this code, I'd write this:

void setIfJpgOrPngExists(const QString& querybase, const QString& answerbase, 
        QString &target) 
{
    if (QFile(querybase + JPG_EXT).exists()) {
        target = answerbase + JPG_EXT;
    } else if (QFile(querybase + PNG_EXT).exists()) {
        target = answerbase + PNG_EXT;
    }
}

if ((test.contains("1.jpg") || test.contains("1.png")) {
    QString leftbase, rightbase;
    if (maxP) {
        gpage = max;
        leftbase = basepath + smax;
        rightbase = basepath + sinc;
    } else {
        page++;
        leftbase = basepath + spage;
        rightbase = basepath + inc;
    }
    add = 1;
    setIfJpgOrPngExists(leftbase, leftbase, p_left);
    setIfJpgOrPngExists(basepath + QString::number(page + 1), rightbase, p_right);
}
share|improve this answer

There is a lot of duplicated code in there. Most of the later parts of the blocks in fact.

And there is a condition in common in both outer conditions:

So lets pull that out first:

if (test.contains("1.jpg") || test.contains("1.png")) {
    if(!maxP){
        page++;
        add = 1;
        if (QFile().exists(basepath + spage + ".jpg")){
            p_left = basepath + spage + ".jpg";
        }
        else if(QFile().exists(basepath + spage+ ".png")){
            p_left = basepath + spage + ".png";
        }
        if (QFile().exists(basepath + QString::number(page + 1) + ".jpg")){
            p_right = basepath + inc + ".jpg";
        }
        else if(QFile().exists(basepath + QString::number(page + 1) + ".png")){
            p_right = basepath + inc + ".png";
        }
    }
    else {
        gpage = max;
        add = 1;
        if (QFile().exists(basepath + smax + ".jpg")){
            p_left = basepath + smax + ".jpg";
        }
        else if(QFile().exists(basepath + smax + ".png")){
            p_left = basepath + smax + ".png";
        }
        if (QFile().exists(basepath + QString::number(page + 1) + ".jpg")){
            p_right = basepath + sinc + ".jpg";
        }
        else if(QFile().exists(basepath + QString::number(page + 1) + ".png")){
            p_right = basepath + sinc + ".png";
        }
    }
}

then pulling out the common code is easy:

if (test.contains("1.jpg") || test.contains("1.png")) {
    if(!maxP){
        page++;
        add = 1;
        if (QFile().exists(basepath + spage + ".jpg")){
            p_left = basepath + spage + ".jpg";
        }
        else if(QFile().exists(basepath + spage+ ".png")){
            p_left = basepath + spage + ".png";
        }

    else {
        gpage = max;
        add = 1;
        if (QFile().exists(basepath + smax + ".jpg")){
            p_left = basepath + smax + ".jpg";
        }
        else if(QFile().exists(basepath + smax + ".png")){
            p_left = basepath + smax + ".png";
        }
    }  
    if (QFile().exists(basepath + QString::number(page + 1) + ".jpg")){
        p_right = basepath + sinc + ".jpg";
    }
    else if(QFile().exists(basepath + QString::number(page + 1) + ".png")){
        p_right = basepath + sinc + ".png";
    }

}

To do something more significant more I'd need to see more of what you are trying to do.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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