Which design is correct? - page 8

New comment
 
valenok2003:

Thank you very much, I am processing the errors, the message comes - wrong price, and I can't figure out what's wrong.

then also add a line 

int err = GetLastError();

at the beginning of the function - clear error buffer: sometimes it is possible to read "someone else's" error, and this way the error buffer will be cleared at the start of the function.

This is not required, but if the programs for the real world, it is highly desirable to do everything as correctly as possible.

Good luck.

 
VladislavVG:

then also add a line

at the beginning of the function - clear error buffer: sometimes it is possible to read "someone else's" error, and this way the error buffer will be cleared at the start of the function.

This is not required, but if the programs for the real world, it is highly desirable to do everything as correctly as possible.

Good luck.


Yes, thank you, that's what I do in real software.
 
Good people, can you tell me why the function returns 0, although there are no open orders. Is there supposed to be some kind of error?
//+------------------------------------------------------------------+
//| функция закрытия ордеров по символу
//+------------------------------------------------------------------+
int Close_This_Symbol_All(string _Symbol_Name, string _Type_Order, int _Slippage, bool _Alert_ON)
{
  bool _Result;
  ERROR = 0;
//----
  if(OrdersTotal() == 0) return(-1);
  for (int _Cnt = OrdersTotal()-1; _Cnt >= 0; _Cnt--) 
  {
    if(!OrderSelect(_Cnt, SELECT_BY_POS, MODE_TRADES)) break;
    if(OrderSymbol() == _Symbol_Name)
    { 
      while (!IsTradeAllowed()) Sleep(1000);
      RefreshRates();
      if(OrderType() == OP_BUY   && _Type_Order == "BUY") 
      {
        _Result = OrderClose(OrderTicket(), OrderLots(), MarketInfo(_Symbol_Name,MODE_BID), _Slippage, CLR_NONE);
        ERROR = GetLastError();
      }
      if(OrderType() == OP_SELL && _Type_Order == "SELL") 
      {
        _Result = OrderClose(OrderTicket(), OrderLots(), MarketInfo(_Symbol_Name,MODE_ASK), _Slippage, CLR_NONE);
        ERROR = GetLastError();
      }
      if(_Result == true) PlaySound(Name_Sound_Close);
      else 
      {
        Error(Name_Expert," Close "+_Type_Order+": ",ERROR,_Alert_ON);
        break;
      }
    }  
  }
//----
  return(ERROR);
}
//+------------------------------------------------------------------+
 
Once again, are we picking apart the pieces?
 
valenok2003:
Good people, can you tell me why this function returns 0 even though there are no open orders. Shouldn't there be some kind of error?
int OrdersTotal( )

Returns the total amount of open and pending orders.

Still writing to order?

 

-- Using names that start with an underscore and a double underscore is bad form. I'd rather use an underscore at the end.


-- Why _Type_Order string? Unrealistically awkward, I think. Then it would have to be cited. Because if you write "Sell" or "sell" it won't work.


-- If you want to make a mistake, make a mistake:

ERROR = -1;

Better yet.

#define ERROR_NOTHING_TO_CLOSE -1
...

...
ERROR = ERROR_NOTHING_TO_CLOSE;


-- 

if(!OrderSelect(_Cnt, SELECT_BY_POS, MODE_TRADES)) break;

Why break? Suppose, a millisecond ago, the order was closed by TP - why should we not close the rest? This is better:

if(!OrderSelect(_Cnt, SELECT_BY_POS, MODE_TRADES)) continue;

--

while (!IsTradeAllowed()) Sleep(1000);

The line is worse than useless -- it is harmful. Is that what you must have meant?

while (IsTradeContextBusy()) Sleep(1000);


--

else 
      {
        Error(Name_Expert," Close "+_Type_Order+": ",ERROR,_Alert_ON);
        break;
      }

That is, if an order suddenly fails to close, "let go of the wheel, close your eyes and start screaming".

There's no need to close the others? No need to try again?


-- Why is there an error return here at all? It would be much more appropriate to return, for example, true if all were successful to close and false if not.


That's all for now.

 
TheXpert:

-- Using names that start with an underscore and a double underscore is bad form. I'd rather have an underscore at the end.


By using names that start with underscores inside functions, I eliminate the risk of variable overriding. Thus, I can use the same names in start() but without underscores, which increases code transparency.

- Why is _Type_Order string? Unrealistically inconvenient, imho. Then it should be given. Because if we write "Sell" or "sell" nothing will work.

You're right, of course. However, using a string increases code transparency and with the amount of code I have at the moment there is no need for an underline. When I come across this problem really, I'll do it, but for now it's not necessary. I simply write SELL or BUY.


Why break? Well, let's assume that an order was closed by TP a millisecond ago, so why should we not close all the others? This is better:

if(!OrderSelect(_Cnt, SELECT_BY_POS, MODE_TRADES)) continue;

You're certainly right, thank you.


The line is worse than useless - it is harmful. Is that what I think you mean?

The IsTradeContextBusy() function informs only about thread occupancy, IsTradeAllowed() is somewhat broader


That is, if suddenly the order cannot be closed, "let go of the wheel, close your eyes and start screaming"?


yes, for the debugging period it is

Why would there be an error return at all? It would be much more appropriate to return true if everything could be closed and false if not.


Thank you, I seem to have figured out what the error is. I'll check and report back.
 

The error was as follows:

the bool _Result variable is not initialized, so it will contain false; thus, since none of the conditions have been met, we have not changed the _Result variable, and it already contains false. And the function proceeds to error handling that really didn't occur.

I fixed the function and am posting it here, I think somebody may find it useful.

//+------------------------------------------------------------------+
//| функция закрытия ордеров по символу
//+------------------------------------------------------------------+
int Close_This_Symbol_All(string _Symbol_Name, string _Type_Order, int _Slippage, bool _Alert_ON)
{
int _Cnt_Close_Orders = 0;
//----
  if(OrdersTotal() == 0) return(_Cnt_Close_Orders);
  for (int _Cnt = OrdersTotal()-1; _Cnt >= 0; _Cnt--) 
  {
    if(!OrderSelect(_Cnt, SELECT_BY_POS, MODE_TRADES)) continue;
    if(OrderSymbol() == _Symbol_Name)
    { 
      while (!IsTradeAllowed()) Sleep(1000);
      RefreshRates();
      if(OrderType() == OP_BUY   && _Type_Order == "BUY") 
      {
        if(OrderClose(OrderTicket(), OrderLots(), MarketInfo(_Symbol_Name,MODE_BID), _Slippage, CLR_NONE))
        {
          _Cnt_Close_Orders++;
          PlaySound(Name_Sound_Close);        
        } 
        else 
        {
          Error(Name_Expert," Close "+_Type_Order+": ",GetLastError(),_Alert_ON);
          continue;
        }
      }
      if(OrderType() == OP_SELL && _Type_Order == "SELL") 
      {
        if(OrderClose(OrderTicket(), OrderLots(), MarketInfo(_Symbol_Name,MODE_ASK), _Slippage, CLR_NONE))
        {
          _Cnt_Close_Orders++;
          PlaySound(Name_Sound_Close);        
        } 
        else 
        {
          Error(Name_Expert," Close "+_Type_Order+": ",GetLastError(),_Alert_ON);
          continue;
        }
      }
    }  
  }
//----
  return(_Cnt_Close_Orders);
}
//+------------------------------------------------------------------+

PS to moderators. I have created such a branch by accident - complete self-tutorial on how to close orders correctly. Maybe it should be renamed as - how to correctly close orders with examples.

PPS Thank you very much to everybody who has taken part. There will probably be more questions.

 
valenok2003:

The error was as follows:

The bool _Result variable is not initialized, so it will contain false; thus, since none of the conditions have been met, we have not changed the _Result variable, and it already contains false. And the function proceeds to error handling that really didn't occur.

I fixed the function and am posting it here, I think somebody may find it useful.

PS to moderators. I have created such a branch by accident - complete self-tutorial on how to close orders correctly. Maybe it should be renamed as - how to correctly close orders with examples.

PPS Thank you very much to everybody who has taken part. There will probably be more questions.


If you look at the CodeBase and carefully read the forum - there are more than a hundred examples of correct order closing, but that does not mean that all branches should be renamed at once. There are enough heroes.
 
valenok2003:

The error was as follows:

the bool _Result variable is not initialized, so it will contain false; thus, since none of the conditions have been met, we have not changed the _Result variable, and it already contains false. And the function proceeds to error handling that really didn't occur.

I fixed the function and am posting it here, I think somebody may find it useful.

PS to moderators. I have created such a branch by accident - complete self-tutorial on how to close orders correctly. Maybe it should be renamed as - how to correctly close orders with examples.

PPS Thank you very much to everybody who has taken part. There will probably be more questions.

IMHO: This function is not clear in its logic. If it is for a tester, it is too much unnecessary, if it is for the real world, the errors should be handled. The returned value has no meaning because you are telling on the external level (on the call level) that the call ended with an error, you are losing the error itself, well, the expert wrote into the log - how does it count?

What would be better: (again, IMHO)

1. Read and save the error number.

2. Split the errors into fixable and non-recoverable. To do this, we should create a handler - those errors, upon appearance of which the order may be closed (a thread is busy, connection failure ..., etc.) should be handled immediately on site. I have done it that way (first the unrecoverable errors, and then those which can be "fixed"):

#define MAXCYKLESCNT 20

int ErrReaction(int err)
{
    switch(err)
    {
        case ERR_TRADE_NOT_ALLOWED    :
                 Print("TRADE NOT ALLOWED ! SWITCH ON option \' Allow live trading\' (Необходимо включить опцию \'Разрешить советнику торговать\')");
        case ERR_INVALID_FUNCTION_PARAMSCNT :    
        case ERR_INVALID_FUNCTION_PARAMVALUE :    
        case ERR_INVALID_STOPS        : 
        case ERR_INVALID_TRADE_VOLUME : 
        case ERR_MARKET_CLOSED        : 
        case ERR_TRADE_DISABLED       : 
        case ERR_NOT_ENOUGH_MONEY     : 
                 return(-err);
        case ERR_NO_CONNECTION        :
                 ReScanServers();
        case ERR_BROKER_BUSY          : 
        case ERR_TRADE_CONTEXT_BUSY   : 
        case ERR_ORDER_LOCKED         :
                 int n=0;
                 while((!IsTradeAllowed())&&(n<20)){ Sleep(500);n++;}
        case ERR_PRICE_CHANGED : 
        case ERR_OFF_QUOTES    : 
        case ERR_REQUOTE       : 
                 RefreshRates();
                 break;
        default: break;
    }//switch(err)
    return(0);
}//int ErrReaction(int err)

3. When an intractable error occurs, to return not an error flag, but its number, so that this situation could be handled in an external module. When analyzing the result of an operation:

bool res    = false;
int  ncykls =     0;

    while((!res)&&(ncykls<MAXCYKLESCNT))
    {
        res = ...............
        if(!res)
        { 
            ncykls++;
            err=GetLastError();
            Print(ncykls," Err(",err,") : ",ErrorDescription(err)); 
            err_res = ErrReaction(err); 
            if(err_res<0) return(err_res);
        } 
    }//while((!res)&&(ncykls<MAXCYKLESCNT))

Good luck.

SZ about break/continue

if( !OrderSelect(i,SELECT_BY_POS,MODE_TRADES) ) break;    // No more Orders
the question is debatable. If there are still orders, then when a take/stop is triggered the order numbering will change - i.e. the order will still be selected: you select by order number, not by ticket. If the order is not selected, it means that there is no order. Since you do not check the number of orders each time, you will run the cycle blank some number of times.
12345678
New comment
Reason: