is my code the most efficient?

New comment
 

Hello all,

i have written a snippet of code to add to my EA but i'm not sure that i am taking the best approach.I was wandering if someone can help me make it cleaner and/or faster. The goal of this function is to update the StopLoss value to the location of the last parabolic sar. The problem is that i feel like i'm wasting resources when I put this into my ea. can someone help?

 bool brk = 0; // break out of the loop 
      double Psar =iSAR(NULL,0,0.02,0.2,1);
      double pastPsar =iSAR(NULL,0,0.02,0.2,1);
      
      if(OrderType()<=OP_BUY)
         { 

           while(OrdersTotal()>0)
           {
              
              if(OrderSelect(0,SELECT_BY_POS,MODE_TRADES))
              {
                if(iSAR(NULL,0,0.02,0.2,0)>OrderOpenPrice() && brk !=1)
                {
                  OrderModify(OrderTicket(),OrderOpenPrice(),(Psar),OrderTakeProfit(),0,Blue);
                }
               else
               {
                  OrderModify(OrderTicket(),OrderOpenPrice(),(pastPsar),OrderTakeProfit(),0,Blue);
               }
                brk =1;

              }
             if(brk ==1)
             {
               break;
             }
           }
         }
         else if(OrderType()<=OP_SELL)
             { 

               while(OrdersTotal()>0)
               {
                 brk = 0;
                 if(OrderSelect(0,SELECT_BY_POS,MODE_TRADES))
                 {                      
                   if(iSAR(NULL,0,0.02,0.2,0)>OrderOpenPrice() && brk !=1){
                     OrderModify(OrderTicket(),OrderOpenPrice(),(Psar),OrderTakeProfit(),0,Blue);
                  }
                  else{OrderModify(OrderTicket(),OrderOpenPrice(),(pastPsar),OrderTakeProfit(),0,Blue);}
                  brk =1;
                 }
                 if(brk ==1){
                   break;
                 }
               }
            }           
         else
           Print("open orders: ",OrdersTotal());

   }
 

There are several problems here.

if(OrderType()<=OP_BUY)

Whilst syntactically legal, this is bad practice. You are using the absolute values of #define'd constants; these should not be relied on.

if(OrderType()==OP_BUY || OrderType()==OP_SELL)

is clearer, safer against compiler changes, and easier to read (if indeed this is what you intend). I see later on you do a compare <=OP_SEND, so It is unclear what you actually want. And looking up the values that isn't what you want anyway! All you actually want apparently is ==OP_BUY. But I had to go and hunt out the values to see what the code was supposed to do rather than just reading the code. It is therefore slow to use, easier to make mistakes, and harder to maintain. That's why it is bad practice!


while(OrdersTotal()>0)

You are not checking against OrderSymbol() and OrderMagicNumber() so you are dealing with all orders on this account which is not good practice since one EA rules the account.


Your indenting is all over the place so it is hard to read the structure of the code.

You do not check the OrderModify to see if it works or not.

You do not update psar or pastPsar within the OrdersTotal loops so they can easily be out of date.

  
               while(OrdersTotal()>0)
               {
                 brk = 0;
                 if(OrderSelect(0,SELECT_BY_POS,MODE_TRADES))
                 {                      
                   if(iSAR(NULL,0,0.02,0.2,0)>OrderOpenPrice() && brk !=1){
                     OrderModify(OrderTicket(),OrderOpenPrice(),(Psar),OrderTakeProfit(),0,Blue);
                  }
                  else{OrderModify(OrderTicket(),OrderOpenPrice(),(pastPsar),OrderTakeProfit(),0,Blue);}
                  brk =1;
                 }
                 if(brk ==1){
                   break;
                 }
               }

I have just re-formatted this so I can read it ...

   while( OrdersTotal()>0 ){
      brk = 0;
      if( OrderSelect(0,SELECT_BY_POS,MODE_TRADES) ){                      
         if( iSAR(NULL,0,0.02,0.2,0)>OrderOpenPrice() && brk !=1 ){
            OrderModify(OrderTicket(),OrderOpenPrice(),(Psar),OrderTakeProfit(),0,Blue);
         }
         else{
            OrderModify(OrderTicket(),OrderOpenPrice(),(pastPsar),OrderTakeProfit(),0,Blue);
         }
         brk =1;
      }
      
      if( brk ==1 ){
         break;
      }
   }

So if the OrderSelect succeeds you break. Why? And what is the point of setting the brk variable. Once you decide to break just do it. Perhaps you think the if statements cause a problem to the break. No. Break will get you the hell out of the closest loop, without regard for ifs

 
And another thing, you start the code snippet with an OrderType(). That means the (correct) order must already have been selected (otherwise it is already a failing process). If the correct order is already OrderSelect'ed why are you selecting it again?
 

My apology to dabbler coz, before this, I actually pm dabbler about this thread and I said something very arrogant and very ignorant ( Check again please, coz I'm watching F1. ).

Thank you for showing me my arrogant and my ignorant.

:(

 


king2k23 2012.05.26 10:54

Hello all,

i have written a snippet of code to add to my EA but i'm not sure that i am taking the best approach.I was wandering if someone can help me make it cleaner and/or faster. The goal of this function is to update the StopLoss value to the location of the last parabolic sar. The problem is that i feel like i'm wasting resources when I put this into my ea. can someone help?

Here's my opinion about you code : Please open your (MetaEditor > Navigator Window (Ctrl + D) > Dictionary tab > Trading functions). You will see there that most of the functions over there has note like this : "The order must be previously selected by the OrderSelect() function".

This code below will never works, coz they're not previously selected by OrderSelect(),

if(OrderType()<=OP_BUY)
if(OrderType()<=OP_SELL)

Those 2 codes also wrong. Because OP_BUY and OP_SELL has it's own defined value which you can check at (MetaEditor > Navigator Window (Ctrl + D) > Dictionary tab > Standards Constans > Trade Operations). OP_BUY has defined value of 0, while OP_SELL has defined value of 1. So when you do this :

if(OrderType()<=OP_SELL)

You actually also selecting OP_BUY, while you may actually just want to select OP_SELL. :(

These code below 

 while(OrdersTotal()>0)
    {
    if(OrderSelect(0,SELECT_BY_POS,MODE_TRADES))

The OrdersTotal() show how many opened position in your account, it can be just 1 position or more. However there's a little mistake in OrderSelect(). The first parameter of OrderSelect() is index of opened position, and in your code it point at 0. That mean this Orderselect() ...

if(OrderSelect(0,SELECT_BY_POS,MODE_TRADES))

will only select the first trade - not first opened - in your MT terminal window. If you plan to open only 1 position, then there's no problem with this OrderSelect(). However, if there are more than 1 opened position, this OrderSelect() will not select other trade coz it's only select first trade in your MT terminal.

Don't believe what I says, just look and click at your MetaEditor.

I will write later coz I keep losing my internet connection. :D

So IMHO your code should be like this

extern int My_Magic_Number=12345;

double Psar     =iSAR(NULL,0,0.02,0.2,1);
double pastPsar =iSAR(NULL,0,0.02,0.2,1);
      
for (int pos = 0; pos <= OrdersTotal(); pos ++)
   {
   if (OrderSelect(pos, SELECT_BY_POS, MODE_TRADES) == true &&
       OrderSymbol () == Symbol() &&                           // <<== so I can run the EA with any symbol ...    
       OrderMagicNumber () == My_Magic_Number)                 // <<== ... even with the same symbol but different magic number
       {
       if (OrderType() == OP_BUY)
          {
                                                               // <<== put your modifying code here
          }
          
        if (OrderType() == OP_SELL)                            // <<== ... and here
          {
          
          }
       }
    }  
 
dabbler:

I didn't think it was either of those ...

but you can feel bad if you like :-)

I feel bad already :D

I'm bad, but thanks anyway.

 

Who's Bad

 

wow! thanks for all the help!

Your indenting is all over the place so it is hard to read the structure of the code.


I'm sorry if i made it difficult for you all to help me I'm normally very careful with the uniform of my code, trying to implemented the suggested idea I've read from other messages has gotten me turned around inside my own structure, also I was beyond tired at the time I decided to ask for help ;p

the goal was to have one trade open for each of the pairs I have the EA applied. When I first started this, I was only concerned with getting one order opened and updated but now that has changed

onewithzachy:

Here's my opinion about you code : Please open your (MetaEditor > Navigator Window (Ctrl + D) > Dictionary tab > Trading functions). You will see there that most of the functions over there has note like this : "The order must be previously selected by the OrderSelect() function".

...
...
...

So IMHO your code should be like this

thanks to you as well

this is the refactored version after dabbler suggestions 

void upDateStop()
   {    

      double Psar =iSAR(NULL,0,0.02,0.2,0);
      double pastPsar =iSAR(NULL,0,0.02,0.2,1);
      double osl = OrderStopLoss(); // osl stoploss value
      
      if(OrderType()== OP_BUY){
            OrderSelect(0, SELECT_BY_POS, MODE_TRADES);
               //check if the stop should be updated
               if(osl < Psar && Psar != OrderStopLoss()){
                  OrderModify(OrderTicket(),OrderOpenPrice(),(iSAR(NULL,0,0.02,0.2,0)),OrderTakeProfit(),0,Yellow);
               }
               else if(OrderStopLoss() != Psar){
                  OrderModify(OrderTicket(),OrderOpenPrice(),(iSAR(NULL,0,0.02,0.2,1)),OrderTakeProfit(),0,Yellow);
               }
      }
      else if(OrderType()== OP_SELL){
         OrderSelect(0, SELECT_BY_POS, MODE_TRADES);
               //check if the stop should be updated
               if(osl > Psar && Psar != OrderStopLoss()){
                  OrderModify(OrderTicket(),OrderOpenPrice(),(iSAR(NULL,0,0.02,0.2,0)),OrderTakeProfit(),0,White);
               }
               else if(OrderStopLoss() != Psar){
                  OrderModify(OrderTicket(),OrderOpenPrice(),(iSAR(NULL,0,0.02,0.2,1)),OrderTakeProfit(),0,Yellow);
               }
     }
         else
           Print("open orders: ",OrdersTotal());
   }

i will look at the shell you've given me and first try to understand what's missing and then install it.

thank you both onewithzachy and dabbler

 
king2k23:

wow! thanks for all the help!


I'm sorry if i made it difficult for you all to help me I'm normally very careful with the uniform of my code, trying to implemented the suggested idea I've read from other messages has gotten me turned around inside my own structure, also I was beyond tired at the time I decided to ask for help ;p

the goal was to have one trade open for each of the pairs I have the EA applied. When I first started this, I was only concerned with getting one order opened and updated but now that has changed

thanks to you as well

this is the refactored version after dabbler suggestions

i will look at the shell you've given me and first try to understand what's missing and then install it.

thank you both onewithzachy and dabbler

Did dabbler even suggest that ... ?. LOL. I think dabbler gonna be ... eh ...negative (have no idea how to describe his reaction) reading your codes.

O boy ... look at that OrderSelect(). :D
 
onewithzachy:

LOL. I think dabbler gonna be ... eh ...negative (have no idea how to describe it) reading your codes.

And then some!

So we come into the function with an order already needing to have been selected (since we are using OrderStopLoss)

then we select (the same?) order again but don't check if that succeeds

then we do a compare against a double (psar and OrderStopLoss) which has almost no chance of ever being equal

and do an OrderModify which is then not checked.


I think more "re-factoring" is in order.

12
New comment
Reason: