Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have this code and I think it is very slow, about 50ms. I need to make it execute faster:

 switch (unit)
   {
       case 0:
       if (u == 2)
       {
           if (u1 == "piece" || u1 == "gram")
               result = quantity * y;
           else
               result = quantity * z;
       }
       if (u == 1)
       {
           if (u1 == "piece" || u1 == "gram")
               result = quantity * y;
           else
               result = quantity;
       }
       res_name = "gram";
       break;

       case 1: //ml
       if (u == 2)
       {
           if (u1 == "piece" || u1 == "gram")
               result = quantity * x;
           else
               result = quantity * y;

       }
       if (u == 0)
       {
           if (u1 == "piece" || u1 == "gram")
               result  = quantity * x;
           else
               result = quantity;
       }
       res_u = "ml";
       break;

case 2:
    res_u = "pieces";
    break;
 }
share|improve this question
1  
Can you give more information about unit, u, u1, res_u, result, quantity, x, y, z? –  Romoku Jan 21 '13 at 15:07
    
@Romoku x, y, and z are fields I get from db. result and res_u are the fields I return from the method. –  Srcee Jan 21 '13 at 15:12
    
I more or less meant what are their object types? –  Romoku Jan 21 '13 at 15:19
    
@Srcee, please describe the meaning of parameters and their values. What the it mean when unit is equal to 0, 1 and 2? Are these the only values it can have? What is the logical meaning of u, and what does the values 0, 1 and 2 mean? Basically your code suffers from well-known magic numbers problem –  almaz Jan 21 '13 at 15:31
5  
@Srcee as to your statement I think it is very slow, about 50ms. I need to make it execute faster - that is not the case, this code will perform much faster unless you're running it on 20 Mhz computer ;). Most likely the slow part is the data retrieval. Please describe how exactly you've measured the performance –  almaz Jan 21 '13 at 15:34
add comment

3 Answers

I agree with Romoku. Unless there is a definite requirement to make this faster and a noticeable effect on performance of your application within this method I would consider making it more DRY, more developer friendly and less procedural.

Without more code and explanation of whats there here are some things I would consider:

  1. Try and remove duplicated code. I notice you repeat your if(u1 == "piece" ... code everywhere. Consider moving that into a method and passing in the parameters required.

    private int GetConversion(UnitsEnum units, int pieceGramsMultipler, int otherMultipler)
    {
      return units == UnitsEnum.Piece || units == UnitsEnum.Gram
                     ? pieceGramsMultipler
                     : otherMultipler;               
    }
    

    Then used like

    if (u == 2)
    {
       result = quantity * GetConversion(u1, y, z);                        
    }
    
  2. Consider changing your string literals into enumerations. So "piece" and "gram" might become:

    private enum UnitsEnum
    {
      Piece,
      Gram
    }
    
  3. Consider changing your case statements into constants or enumerations. So rather than case 1, it might be case Metric.Ml or ML or Milliliters etc

  4. Offer better names for your variables. u1, x, y, z gives me no idea of what they are trying to do.

  5. Use if else instead of 2 if statements. If u == 2 then there is no chance that it will be == 1 so convert your if's into if elses to make that more obvious.

Otherwise, perhaps posting the full code that this piece resides in might get your more help and suggestions. I get the feeling that there is more help this community can offer you here than just what you have posted.

share|improve this answer
    
Very good advice in this post; just one minor quibble: UnitsEnum should probably be called Unit, I don't see a reason to encode the type in the name. In fact, the enums may be unnecessary if the raw integer values can be replaced with domain types — but your answer is still the best one possible given the limited information in the question. –  codesparkle Jan 22 '13 at 19:28
    
@codesparkle Good points. I don't even know if even Unit is the best name. Took a wild guess there :) –  dreza Jan 22 '13 at 19:38
add comment

The first thing I would do is refactor your names to something understandable. After that I would try to create a data structure to encapsulate the nested logic.

Without knowing more about the background it is hard to refactor without breaking some kind of functionality.

share|improve this answer
add comment

Rather than guessing which lines might be the ones taking up a lot of CPU time, you should run this code through a profiler, which will tell you which parts take up most of the run-time, which are the parts you should then target for optimization.

share|improve this answer
    
I get the 50ms time with profiler. –  Srcee Jan 21 '13 at 18:44
    
Which profiler are you using? –  Jargon Jan 21 '13 at 18:56
    
This one: miniprofiler.com –  Srcee Jan 21 '13 at 19:10
    
It looks like miniprofiler has a step function you can use to narrow down how long specific pieces of code take. Alternatively, you could time parts of your code manually with the System.Diagnostics.Stopwatch class. –  Jargon Jan 21 '13 at 19:30
add comment

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.