Need help to spot some slight error . - page 2

New comment
 
Anyone mind to give a hand please .....
 

Part of the problem is the way you wrote that code with big long if conditions full off &&, ||, and function call after function call makes it difficult to debug, you would be lucky to find anyone with the time to unravel that mess. You should look at the coding examples in the docs to see how code should be formatted in much shorter lines and commented.

 

" Makes it difficult to debug "?? :(  Never heard of these though , is this real .... 

Compiler also have hard time to debug my code :( ?

If that's the reason means I had to rethink all my coding idea for this part ?? Then it will turn up a totally different thing ...... :( :(

 

Yes it is hard to debug for example look at this code for trailing stop. It is easy to see what each line does therefore easy to spot mistakes.

   int total=OrdersTotal();
   
   for(int cnt=0; cnt<total; cnt++)
   {if(!OrderSelect(cnt,SELECT_BY_POS,MODE_TRADES))  
    {if(OrderType()==OP_BUY)
     {if(TrailingStop>0)
      {if(Bid-OrderOpenPrice()>Point*TrailingStop)
       {if(OrderStopLoss()<Bid-Point*TrailingStop)
        {if(!OrderModify(OrderTicket(),OrderOpenPrice(),Bid-Point*TrailingStop,OrderTakeProfit(),0,Green))
         {Print("OrderModify error ",GetLastError());
          return;
   }}}}}}}
 
I tried removing the && condition and put them as a if statement as after .... But still didn't work all the time , sometimes it just only moved the stop loss once , sometimes it worked just fine for both buy and sell orders . 
 
SDC: Yes it is hard to debug for example look at this code for trailing stop. It is easy to see what each line does therefore easy to spot mistakes.
   int total=OrdersTotal();
   
   for(int cnt=0; cnt<total; cnt++)
   {if(!OrderSelect(cnt,SELECT_BY_POS,MODE_TRADES))  
    {if(OrderType()==OP_BUY)
     {if(TrailingStop>0)
      {if(Bid-OrderOpenPrice()>Point*TrailingStop)
       {if(OrderStopLoss()<Bid-Point*TrailingStop)
        {if(!OrderModify(OrderTicket(),OrderOpenPrice(),Bid-Point*TrailingStop,OrderTakeProfit(),0,Green))
         {Print("OrderModify error ",GetLastError());
          return;
   }}}}}}}
  1. You MUST count down when closing/deleting in the presence of multiple orders. Think the EA on other charts. Get in the habit.
       for(int cnt=OrdersTotal() - 1; cnt >= 0; --cnt)
   {if(!OrderSelect(cnt,SELECT_BY_POS,MODE_TRADES))  
    {if(OrderType()==OP_BUY)
     {if(TrailingStop>0)
      {if(Bid-OrderOpenPrice()>Point*TrailingStop)
       {if(OrderStopLoss()<Bid-Point*TrailingStop)
        {if(!OrderModify(OrderTicket(),OrderOpenPrice(),Bid-Point*TrailingStop,OrderTakeProfit(),0,Green))
         {Print("OrderModify error ",GetLastError());
          return;
   }}}}}}}
    Also prefer ++x over x++ (especially when dealing with classes)
  2. The language is for() statement, if() statement, so it make little sense to put braces around a single statement.
    You wouldn't write
    		but 
    { i = 1 + 2; }
{ if(i == 3) { Print(i); } }
    i = 1 + 2;
if(i == 3) Print(i);
    So Simplify
       for(int cnt=OrdersTotal() - 1; cnt >= 0; --cnt)
   if(!OrderSelect(cnt,SELECT_BY_POS,MODE_TRADES))  
   if(OrderType()==OP_BUY)
   if(TrailingStop>0)
   if(Bid-OrderOpenPrice()>Point*TrailingStop)
   if(OrderStopLoss()<Bid-Point*TrailingStop)
   if(!OrderModify(OrderTicket(),OrderOpenPrice(),Bid-Point*TrailingStop,OrderTakeProfit(),0,Green))
   {  Print("OrderModify error ",GetLastError());
      return;
   }
  3. Now that "&&" (and "||") are short circuit operators don't chain ifs
       for(int cnt=OrdersTotal() - 1; cnt >= 0; --cnt)
   if(!OrderSelect(cnt,SELECT_BY_POS,MODE_TRADES))  
   && OrderType()==OP_BUY)
   && TrailingStop>0)
   && Bid-OrderOpenPrice()>Point*TrailingStop)
   && OrderStopLoss()<Bid-Point*TrailingStop)
   && !OrderModify(OrderTicket(),OrderOpenPrice(),Bid-Point*TrailingStop,OrderTakeProfit(),0,Green))
   {  Print("OrderModify error ",GetLastError());
      return;
   }
  4. Do you want to modify the order when the order select has failed?
       for(int cnt=OrdersTotal() - 1; cnt >= 0; --cnt)
   if(OrderSelect(cnt,SELECT_BY_POS,MODE_TRADES))  
   && OrderType()==OP_BUY)
   && TrailingStop>0)
   && Bid-OrderOpenPrice()>Point*TrailingStop)
   && OrderStopLoss()<Bid-Point*TrailingStop)
   && !OrderModify(OrderTicket(),OrderOpenPrice(),Bid-Point*TrailingStop,OrderTakeProfit(),0,Green))
   {  Print("OrderModify error ",GetLastError());
      return;
   }
  5. Remove all tests that do not change out of the loop.
    if TrailingStop>0){
   for(int cnt=OrdersTotal() - 1; cnt >= 0; --cnt)
   if(OrderSelect(cnt,SELECT_BY_POS,MODE_TRADES))  
   && OrderType()==OP_BUY)
   && Bid-OrderOpenPrice()>Point*TrailingStop)
   && OrderStopLoss()<Bid-Point*TrailingStop)
   && !OrderModify(OrderTicket(),OrderOpenPrice(),Bid-Point*TrailingStop,OrderTakeProfit(),0,Green))
   {  Print("OrderModify error ",GetLastError());
      return;
   }
}
  6. Don't keep calculating the same thing.
       if(Bid-OrderOpenPrice()>Point*TrailingStop)
   if(OrderStopLoss()<Bid-Point*TrailingStop)
   if(!OrderModify(OrderTicket(),OrderOpenPrice(),Bid-Point*TrailingStop,O...
    Figure out what you want to do and wither you should
    if TrailingStop>0){
   for(int cnt=OrdersTotal() - 1; cnt >= 0; --cnt)
   if(OrderSelect(cnt,SELECT_BY_POS,MODE_TRADES))  
   && OrderType()==OP_BUY){
      double SL = Bid - Point*TrailingStop;
      if(SL > OrderOpenPrice()    // Start trailing above open price
      && SL > OrderStopLoss()     // And it moves up ONLY
      && !OrderModify(OrderTicket(),OrderOpenPrice(),SL,OrderTakeProfit(),0,Green))
      {  Print("OrderModify error ",GetLastError());
         return;
      }
   }
}
  7. What about the other cases? What about other charts and EAs?
    if TrailingStop>0){
   for(int cnt=OrdersTotal() - 1; cnt >= 0; --cnt)
   if(OrderSelect(cnt,SELECT_BY_POS,MODE_TRADES)
   && OrderMagicNumber() == MyExternal
   && OrderSymbol()      == Symbol()  
   ){  
      if(OrderType()==OP_BUY)
      {
         double SL = Bid - Point*TrailingStop;
         if(SL > OrderOpenPrice()    // Start trailing above open price
         && SL > OrderStopLoss()     // And it moves up ONLY
         && !OrderModify(OrderTicket(),OrderOpenPrice(),SL,OrderTakeProfit(),0,Green))
         {  Print("OrderModify error ",GetLastError());
            return;
         }
      }  // OP_BUY
      else // OP_SELL (or pending)
      { ...                       
      }                           
   }  // OrderSelect
}  // Trailing

 
WHRoeder:
  1. You MUST count down when closing/deleting in the presence of multiple orders. Think the EA on other charts. Get in the habit.Also prefer ++x over x++ (especially when dealing with classes)
  2. The language is for() statement, if() statement, so it make little sense to put braces around a single statement.
    You wouldn't write
    		but
    So Simplify
  3. Now that "&&" (and "||") are short circuit operators don't chain ifs
  4. Do you want to modify the order when the order select has failed?
  5. Remove all tests that do not change out of the loop.
  6. Don't keep calculating the same thing. Figure out what you want to do and wither you should
  7. What about the other cases? What about other charts and EAs?

I posted that code just as an example of easy to read code, it was not meant to be an example of a completed stand alone function.

It is the buy orders section of the trailing stop code from the MetaQuotes MACD sample EA included with MT4.

1) Not true, you can count up or down, the loop is more efficient, OrdersTotal() is called once and assigned to a local var.

 
SDC: I posted that  code just as an example o
I posted just an enlargement of the subject.
 

Thank you SDC . Thanks for the tips too WHRoeder . It's useful .

I tried swaping OrderClosePrice() to the MarketInfo one on the previous code and the edited ( removing the && condition and put them as a if statement as after , the one in the 2nd for loop ) , but the result is still sometimes working sometimes not working .

The for loop for counting to total orders in the pool I uses count down loop but with x-- . I don't understand why you suggest --x though . 

I googled "short circuit operators" but don't really understand what it means at mql4 , do you mind to explain a bit ^_^ ?? Why is it bad to chain 'if' ? 

By the way , the above code that SDC suggested , that's not the code I'm using >.< .

 

It is not bad to chain ifs. The developers of the MQL4 language wrote the code I posted above. It is some code I cut from their macd sample EA as an example.

WHR was referring to a recent change to the way && || conditions are evaluated which now makes them equally as efficient as chained if conditions. Previously they were less efficient. You can use either method. Chained ifs are useful when there are divergences in the code so you can use 'else'.

Long lines of if( && || )  conditions can create parentheses confusion which makes finding errors more difficult, that is why I don't like doing it. Also there is an accepted standard for coding that says it should not be more than 80 characters wide. A lot of coders dont bother to adhere to that standard though, and the MQL4 developers keep creating enumerated identifiers with big long names to be used in their function calls to functions with equally big long names, which doesn't help with code formatting a whole lot.

12345
New comment
Reason: