Le 05/08/2013 15:59, jicman a écrit :
Greetings!
I have this code,
foreach (...)
{
if (std.string.tolower(fext[0]) == "doc" ||
std.string.tolower(fext[0]) == "docx" ||
std.string.tolower(fext[0]) == "xls" ||
std.string.tolower(fext[0]) == "xlsx" ||
std.string.tolower(fext[0]) == "ppt" ||
std.string.tolower(fext[0]) == "pptx")
continue;
}
foreach (...)
{
if (std.string.tolower(fext[0]) == "doc")
continue;
if (std.string.tolower(fext[0]) == "docx")
continue;
if (std.string.tolower(fext[0]) == "xls")
continue;
if (std.string.tolower(fext[0]) == "xlsx")
continue;
if (std.string.tolower(fext[0]) == "ppt")
continue;
if (std.string.tolower(fext[0]) == "pptx")
continue;
...
...
}
thanks.
josé
Like others said, writing if( ...) continue; if(...) continue; versus
if(... || ...) continue; is not the problem in the code.
Computing the lower case of the string for each comparison is a greater
problem that can be spotted at the first glance. Storing the value in a
variable and then using this variables in comparisons would be better.
Both versions of your code should be equivalent, though this should be
verified.
I would suggest another way of doing this:
switch(std.string.tolower(fext[0])) {
case "doc":
case "docx":
case "xls":
case "xlsx":
case "ppt":
case "pptx":
continue;
default:
// do something else
}
This way of writing your code seems far more readable and elegant to me.
Even more concise:
switch(std.string.tolower(fext[0])) {
case "doc","docx","xls","xlsx","ppt","pptx":
continue;
default:
// do something else
}
Much like what morarch_dodra proposed, I guess.
But notice that tolower is deprecated in current D2 version, toLower
should be used instead. If you still use D1, maybe a better thing to do
before considering such optimization would be opting for D2.
I would also suggest avoiding usage of continue if you don't need it.
Here, it is likely that you can write something more structured like:
import std.string : toLower
// ...
foreach(...) {
switch(fext[0].toLower) {
case "doc","docx","xls","xlsx","ppt","pptx":
// do what is to be done here with these cases, or nothing
break;
default:
// do something for other cases
}
}
If you need to use fext[0].toLower in any case, I would suggest you
write this kind of code instead:
import std.string : toLower
// ...
auto ext = fext[0].toLower;
switch(ext) {
case "doc","docx","xls","xlsx","ppt","pptx":
// do what is to be done here with these cases, or nothing
// use ext instead of fext[0].toLower, if needed
break;
default:
// do something for other cases
// use ext instead of fext[0].toLower, if needed
}
auto ext = fext[0].toLower; should be placed before the foreach loop if
fext[0] isn't changed in the loop (avoid computing a value more than one
time when it is possible, though thinking before applying this rule or
any other rule is not forbidden).
There might exist solutions that could be faster, like testing the first
letter before the second / using the length of the string / using finite
state machine to save tests, but beware of the maintainability of your
code and if you go into this, triple check that these solutions are
indeed more efficient in terms of execution.
See morarch_dodra, JS, bearophile and others posts for more precise
information.
If this part of your code is not known to be a bottleneck, I would opt
for readability / elegance over over-optimization and for this
particular case, I would trust the compiler for optimizing enough with
the switch version of the code (though it is not more than a opinion here).
