Which option is faster...

[Raphaël Jakse]

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).