patterncppMinor
Conditionally set strings if files exist
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.
Consolidate tests in compound
Right now the code is basically this:
This can be simplified like this:
Consolidate code where only the data changes
The code currently has repeated patterns like this:
It then repeats, but with slightly different data. That suggests further consolidation:
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:
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 statementsRight 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.