HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Conditionally set strings if files exist

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
conditionallyexistfilesstringsset

Problem

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";
        }
    }

Solution

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);
}

Code Snippets

constexpr static char[] JPG_EXT{".jpg"};
constexpr static char[] PNG_EXT{".png"};
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
}
if ((test.contains("1.jpg") || test.contains("1.png")) {
    if (maxP) {
        // do other things
    } else {  // maxP must be false
        // do things
    }
}
if (QFile().exists(basepath + smax + ".jpg")){
    p_left = basepath + smax + ".jpg";
}
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;
    }
}

Context

StackExchange Code Review Q#121967, answer score: 7

Revisions (0)

No revisions yet.