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.
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++)
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.
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.
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.
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 "<="
- Free trading apps
- Over 8,000 signals for copying
- Economic news for exploring financial markets
You agree to website policy and terms of use
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.