Please check my code

New comment
 

I have a function that checks only open trades on current symbol to see if they exceed a specific pip value provided by user called "CashCollectInPips". Let's assume 10 pips. The problem here is that although I believe the code to be correct, I sometimes see wrong orders being closed or once in a while getting a message about closing problems. I am just asking you to take a look at it to see if you can see any issues.

{
   
   int CompTotal = 0;
   double ChartPipTotal = 0.0;
   double TicketPips = 0.0;
   bool Pass = true;

   RefreshRates();
      
   for(int i = 0; i <= OrdersTotal(); i++) {   // loop through all orders
      OrderSelect(i, SELECT_BY_POS, MODE_TRADES);
      if(OrderMagicNumber() == MagicNumber && OrderSymbol() == Symbol()) { // make sure we look at symbols orders
         CompTotal = CompTotal + 1;  // Get a count of symbols orders
         if(OrderType() == OP_BUY)
         { // Add up the pips for all the buys on this symbol
            TicketPips = NormalizeDouble(OrderClosePrice() - OrderOpenPrice(), Digits);
            Logger(StringConcatenate(Symbol(), " - Buy, OrderClosePrice: ", DoubleToStr(OrderClosePrice(), Digits), " OrderOpenPrice: ", DoubleToStr(OrderOpenPrice(), Digits), " Total: ", DoubleToStr(TicketPips, Digits)));
         }
         else
         { // Add up the pips for all sells on this symbol (only working with buys/sells no pending
            TicketPips = NormalizeDouble(OrderOpenPrice() - OrderClosePrice(), Digits);
            Logger(StringConcatenate(Symbol(), " - Buy, OrderOpenPrice ", DoubleToStr(OrderOpenPrice(), Digits), " OrderClosePrice: ", DoubleToStr(OrderClosePrice(), Digits), " Total: ", DoubleToStr(TicketPips, Digits)));
         }
         if(TicketPips < 0) // If we get even 1 negative trade, we want to procross, otherwise, pass
         {
            Pass = false;
         }
         ChartPipTotal += TicketPips; // Add up both positive and negative pips
      }
   }
   
   if(Pass)  // save time and bail out, nothing to look at yet
   {
      return (0);
   }
   

   Logger(StringConcatenate(Symbol(), " - ChartPipTotal: ", DoubleToStr(ChartPipTotal, Digits), " Cash CompTotal: ", CompTotal));
   if (ChartPipTotal > CashCollectInPips* PipPoints) // Is the Pip Total greater than user amount of pips?
   {
      Print(StringConcatenate(Symbol(), " - ChartPipTotal: ", DoubleToStr(ChartPipTotal, Digits), " Cash CompTotal: ", CompTotal));
      Print(StringConcatenate(Symbol(), " - Profits exceed ", DoubleToStr(CashCollectInPips*PipPoints, Digits), " pips, We are closing trades on ", Symbol(), "."));
      StopOpeningTrades = true; // Stop opening trades while we process the closures

      for(i=CompTotal; i>=0; i--)
      {
         OrderSelect(i, SELECT_BY_TICKET, MODE_TRADES);
         if(OrderMagicNumber() == MagicNumber && OrderSymbol() == Symbol()) 
         { // confirmed that all trades are still a part of this symbols activities
            if(!OrderClose(i,OrderLots(),OrderClosePrice(),3, Orange))
            { // did we get an erro, if so spit it out.
               Print(OrderError());
            }
         }
      }

      Logger(StringConcatenate("Cash OrdersTotal: ", OrdersTotal()));
      StopOpeningTrades = false;  // Turn on trader again
   }   
}
 
LEHayes:

I have a function that checks only open trades on current symbol to see if they exceed a specific pip value provided by user called "CashCollectInPips". Let's assume 10 pips. The problem here is that although I believe the code to be correct, I sometimes see wrong orders being closed or once in a while getting a message about closing problems. I am just asking you to take a look at it to see if you can see any issues.

There seem to be at least two issues in the code. Firstly, this line looks wrong:


for(i=CompTotal; i>=0; i--)


Let's say that there's a total of 10 orders open in MT4, of which 3 are for the current EA. I.e. the other 7 are for a different EA and/or symbol. And let's say that the 3 orders for the current EA are at positions #0, #4 and #8 as returned by OrderSelect().


CompTotal will get set to 3. If the code then identifies that it needs to close everything, the above line loops down from 3 to 0. This is processing the first four orders returned by OrderSelect(), not the three orders related to the current EA. It will try to close the order at position #0, but it will miss the EA's orders at #4 and #8. I can't see why you don't just loop down from OrdersTotal()-1 rather than from CompTotal, keeping the current check that you're only processing orders related to the current EA.


The second issue is this line:


if(!OrderClose(i,OrderLots(),OrderClosePrice(),3, Orange))


The first parameter for OrderClose() should be a ticket number, not a zero-based order index. As far as I can see, this will only work at all in the strategy tester, which generates dummy ticket numbers starting at 1.


If you think that the above code fundamentally does work then, unless I'm having a bad day, it must be the case that you've only tried it in the strategy tester. It will happen to work there - partially - because there will be no orders from other EAs, and ticket numbers are 1-based.

 
You may be right on the money, what you describe is what I have been encountering. If this was re-written, how would you do it? If you don't mind....
 
LEHayes:
If this was re-written, how would you do it? If you don't mind....

Try changing:

for(i=CompTotal; i>=0; i--)

to

for(i=OrdersTotal()-1; i>=0; i--)


And also:

if(!OrderClose(i,OrderLots(),OrderClosePrice(),3, Orange))

to

if(!OrderClose(OrderTicket(),OrderLots(),OrderClosePrice(),3, Orange))


There may be other issues. For example, you ought to do something like a RefreshRates() before each OrderClose(), in case the price you're using goes stale while trying to close multiple orders. And the majority of people posting on this site use Ask or Bid (as appropriate) as the price parameter for OrderClose(), rather than OrderClosePrice(). However, using OrderClosePrice() should in fact work, and is more elegant.

  
  for(int i = 0; i <= OrdersTotal(); i++)
Does not it has to be? 
  for(int i = 0; i < OrdersTotal(); i++)
 
Yes, that also. I always count down
for(int openOrder = OrdersTotal() - 1; openOrder >= 0; openOrder--) if (
    OrderSelect(openOrder, SELECT_BY_POS)
&&  OrderMagicNumber()	== MagicNumber				// with my magic number,
&&  OrderSymbol()	== Symbol() ) {				// in my chart.
//...
}
since closing an order renumbers higher numbered ones.
 

Itachi wrote >>
Does not it has to be?

for(int i = 0; i < OrdersTotal(); i++)

Very true. As things stand, if the last order (with index OrdersTotal() - 1) belongs to the EA, then its number of pips will get double-counted, potentially leading to everything getting closed too early. (I've got a personal preference for checking the return value from OrderSelect(). It does occasionally fail in real life.)


WHRoeder wrote >>

Yes, that also. I always count down

Being fair, LEHayes's original code is counting down where it's actually needed.

 
Itachi wrote >>
Does not it has to be?

Actually counting down is the way to handle closing orders because the index will shift on you if you do it counting up, by counting down you are taking the last indexed item and closing it first so it will not shift your index count ahead of you for the next item.

 
jjc wrote >>

Try changing:

to

And also:

to

There may be other issues. For example, you ought to do something like a RefreshRates() before each OrderClose(), in case the price you're using goes stale while trying to close multiple orders. And the majority of people posting on this site use Ask or Bid (as appropriate) as the price parameter for OrderClose(), rather than OrderClosePrice(). However, using OrderClosePrice() should in fact work, and is more elegant.

I got it, sorry, I was running all night without sleep and I have been short on sleep for the last 3 weeks trying to resolve this bug. I knew I was close but my brain went to mush.

 
LEHayes:

Actually counting down is the way to handle closing orders because the index will shift on you if you do it counting up, by counting down you are taking the last indexed item and closing it first so it will not shift your index count ahead of you for the next item.

I just meant, it has to "<" instead "<="

New comment
Reason: