Adviser's project - page 4

[Alexey Volchanskiy]

Vitaly Muzichenko:

Me too, but I came to the conclusion a long time ago that code should be compact in places where it is never looked at, it is never corrected, and will never be corrected.

Scattering user code with all these slots is an additional headache, because you will need to drag and drop files into different terminals or share them. Of course, you can transfer the includniks to all terminals, but if you change or add something in one terminal, then all of them must be replaced with a new one.

The Expert Advisors and indicators are so small that there is no point in distancing them from the body of the program. To be more correct, they are not small, they are single file, it's not like a site with 10 000 pages where you cannot do without class and inludes. Moreover, there are structures now, and they are quite enough to write compact and 100% workable code.

Here we go.... Do you know about symbolic links to folders?http://skesov.ru/sozdanie-simvolnoy-ssyilki-dlya-papki/

I have all my libraries in one folder, and in a bunch of terminals, there are dozens of them, in mql*\includes folders there are symbolic links to this real folder. Nothing needs to be dragged anywhere.

Also, I actively use storage, if I keep there everything important, I can download it to another terminal in 5 seconds. But for libs symbolic links are more convenient, always full synchronization.

[Vitaly Muzichenko]

Alexey Navoykov:
I recommend using symbolic links or junction links for the MQL folder. All terminals will look in the same folder.

And share it with someone else?

[Alexey Navoykov]

Vitaly Muzichenko:

Sharing with someone?

Well, you have to decide what's more important to you, check or drive.) Is the ease of sharing with someone more important to you than the comfort of coding? If, say, you found a mistake in some function that is used by many Expert Advisors, then you will have to get into the code of each of them and rewrite this function - doesn't it bother you as a programmer?

[Gregory Kovalenko]

Thanks to all for discussing my question.

Decided to look towards OOP in mqlx for starters, keep your (repeatable) functions in a separate file(s). And don't be lazy to comment.

And another + for the symbolic link in Windows! I've used it in linux but forgotten about it in Windows. I will have to give it a try...

[Vitaly Muzichenko]

Alexey Navoykov:

You have to decide what's more important to you, check or drive.) Is the simplicity of the process of sharing with someone more important to you than the comfort of coding? If, for example, you have detected an error in a function, which is used by many Expert Advisors, you will have to go into the code of each of them and rewrite this function - doesn't it bother you as a programmer?

I have had such a case only once, about a month ago, but I have had to add checking for market openness there, there are all checks except for this one and it has popped out for the first time since I have been using it.

If something needs to be added, I add it to the current program, and after that I use the file as a template for the next program. As a result, in a few years the template has everything, well, or almost everything, so that a bot of any complexity can be written in half an hour.

The entire executable code fits in one screen, although the file contains a little more than 4,000 lines, but I look there very rarely, if only what I need to add. From functions in loops refused, used only two, one on the open collects information, the second on the history, and all this in the structure at the bottom of the code. Everything is very simple and close to each other. The main code is commented. The project expands very easily and quickly, without any losses.

[Georgiy Merts]

Alexey Volchanskiy:

Looks good, can we also see TRACE_*** and ASSERT?

Well... For the author of a master class on seduction of women, whom I envy with a black envy, you're welcome.

The debug-version is automatically enabled in my code, if the corresponding system macro is defined. However, if it's not enabled, you can also enable asserts and trace by defines:

#define _FORCETRACE 1
#define _FORCEASSERT 1

In this case - regardless of system settings - debug traces and debug assertions are generated.

I have a directive to connect these macros:

#include <MyLib\DebugOrRelease\DebugSupport.mqh>

This directive connects all necessary files and definitions. I have them all in a separate DebugOrRelease folder. Attaching them here. (The code was written a long time ago, mostly "hastily", so it's not as pretty as the interfaces and history class). The asserts and traces themselves for the debug-version are in the AssertD and TraceD files, the real functions are PerformAssert() and PerformTrace().

Besides these files and macros use global log file (if output to log file is set), I've already posted it once, but, once again. The log file is in my "Common" folder

[Georgiy Merts]

Andrey Kisselyov:

Good work, I like it, but I don't like OOP and try to do without it. I don't like processors with stream splitting (for example, 4 cores and 8 threads). It should be clear that splitting and any virtualization is a loss of performance and loss of machine time for its implementation, whether it is stream splitting in the kernel or virtualization of functions in the code.

brevity is the sister of talent, I think it sounds better.

I learned a long time ago that maintainability and reuse of code is far more important than reducing performance.

OOP - it helps me a lot when I go back to the code after some time to modify it. Not to mention reuse.

But, I agree, it's far from always necessary to use OOP.

Say, I have CDataProvider:pulic CDataProviderI class - data provider, which provides expert with timeseries, indicators, terminal and environment data. In an Expert Advisor, there can be many TSs - each of them would receive pointers to timeseries and indicators from the data provider (each TS would not need to create timeseries - the data provider would provide pointers to the necessary timeseries if they already exist, and would only create timeseries, which have not been created yet).

When you need to receive an indicator from the data provider - fill the indicator description structure, and then request an indicator from the provider, pointing to this structure.

Accordingly, each indicator inside the data provider should be able to identify its structure (the data provider "knows" only the abstract base class of the structure) and according to it create the ready indicator object, which will be created by data provider.

But it's unreasonable to make all this stuff just for the purpose of checking a new idea of an indicator. As a result, for such new indicators everything is "homemade", without any OOP. However, if I see that an indicator is useful for Expert Advisors, it is written "properly" - with full OOP support and creation inside a data provider.

P.S.

By the way, in the case of indicators and data provider we see the advantage of virtualization inheritance. We have the basic interface of indicator parameters CIndicatorParametersI, the successor of this interface are the real parameters of the necessary indicator. When requesting the indicator, we declare these parameters and pass to the data provider a pointer to the abstract interface. Thus the data provider itself doesn't even know what indicator is requested - it is defined in one function, in which the indicator is created according to the new type. And only this created indicator knows what parameters were passed, it retrieves the required parameters from the passed object.

The trick is that almost everywhere inside the data provider there is a simple basic class of parameters (or indicators) - only the most simple and common functions of basic interfaces are available to data provider. This simplifies the code modification (when it is needed), and does not create the temptation to "tamper" with the code of indicators from the data provider. If you want to change an indicator, it is done only inside the indicator, the data provider is only a storage of indicators, the maximum it can do is to create a new indicator.

[Vasiliy Sokolov]

George Merts:

By the way, it makes me very nervous when nesting is more than two levels. I try never to write it that way, spreading code over functions.

And even when there are two nesting levels - after each closing parenthesis I must write a comment, which block it closes (for example, duplicate loop header).

As for style, here's my code for selectinga history position for MT5 (by specified magician, symbol, with specified date range):

The history class itself is a descendant of the CTradeHistoryI abstract interface:

By selecting the required history - you can recalculate its components (positions for MT5 or orders for MT4), and get an interface to any component as an abstract interface:

For MT4 there are corresponding history classes which also inherit from those interfaces - thus the cross-platform nature is provided at the same time - an EA doesn't need to find out where it works, all work with the history is done through abstract interfaces.

Not much of a criticism:

class CTradePosComponentI: public CMyObject
{
...
}

Why re-invent the wheel in the form of CMyObject, when there is a standard CObject that everyone understands?

class class CTradeHistoryI: public CMyObject
{
// Расширенный интерфейс
   virtual void Sort(ESortTPCMode stmMode = STM_BY_OPEN_TIME_A) = 0;
}

The functionality of CObject and CArrayObj is clearly copied here. Why? Quick sorting is built in standard data containers. You should use them.

class CTradePosComponentI: public CMyObject
{
public:
   void CTradePosComponentI() {    SetMyObjectType(MOT_TRADEPOS_COMPONENT_I); };
   virtual void ~CTradePosComponentI() {};
}

If a class has an interface - you'd better hide its constructor in the protected section. Then its object cannot be created directly.

Define an empty constructor? I don't know. I would not do that. You'd better not mention the destructor if you don't need it.

for(iI=0;iI<iHistoryDealsTotal; ++iI)
...

Non-standard increment iI, non-standard iteration ++iI, iHistoryDealsTotal - defined somewhere out there, far before the loop. Better simpler:

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

It's just as fast as the previous version, but much more obvious.

virtual bool               IsTPCInUnloss() const { if(GetTPCStopLoss() <= 0 || GetTPCStopLoss() == EMPTY_VALUE) return(false); if(GetTPCType() == POSITION_TYPE_BUY) { if(GetTPCStopLoss() >= GetTPCOpenPrice()) return(true); } else { if(GetTPCStopLoss() <= GetTPCOpenPrice())return(true); }; return (false); };

The programmers themselves seem to be against such texts but they themselves write them somewhere. No one wants to look into such nonsense. What prevented them from writing it that way:

virtual bool IsTPCInUnloss() const
{
   if(GetTPCStopLoss() <= 0 || GetTPCStopLoss() == EMPTY_VALUE)
      return(false);
   if(GetTPCType() == POSITION_TYPE_BUY)
   { 
      if(GetTPCStopLoss() >= GetTPCOpenPrice())
         return(true);
   } 
   else
   {
     if(GetTPCStopLoss() <= GetTPCOpenPrice())
        return(true);
   }; 
   return (false);
};

And ';' at the end of curly brackets - that's obsolete, you shouldn't do that now.

A giant Select method consists of a giant for loop:

for(iI=0;iI<iHistoryDealsTotal; ++iI)
      {
      ulCurTicket = HistoryDealGetTicket(iI);
      
      if(ulCurTicket == 0)
         return(WRONG_VALUE);
      
      // Получим направление сделки   
      if(HistoryDealGetInteger(ulCurTicket,DEAL_ENTRY,lCurEntry)!=true)
         {
         TRACE_INTEGER("Не удалось получить направление сделки ! Тикет: ",ulCurTicket);
         continue;
         };
      
      // Проверим направление сделки
      if(lCurEntry != DEAL_ENTRY_OUT)
         continue;
      
      // Получим магик сделки
      if(HistoryDealGetInteger(ulCurTicket,DEAL_MAGIC,lCurMagic)!=true)
         {
         TRACE_INTEGER("Не удалось получить магик сделки ! Тикет: ",ulCurTicket);
         continue;
         };
         
      // Проверим магик
      if(ulMagic != NULL && lCurMagic != ulMagic)
         {
         //TRACE_INTEGER("Сделка не подходит ! Имеет неверный магик ! Magic сделки: ",lCurMagic);
         //TRACE_INTEGER("Требуемый Magic : ",ulMagic);
         continue;
         };
      ...
}

Obviously, all the checks for the compliance of the transaction with the current Expert Advisor should be implemented in a separate method, for example like this:

for(iI=0;iI<iHistoryDealsTotal; ++iI)
{
   if(!CheckDeal(iI))
      continue;
   ...
}

And in general, select should be split into 3-4 more methods to make it clear what's going on in it.

George Merts
The Expert Advisor itself consists of five lines. In this file, the object of the EA's parts factory itself is declared and the inclusions are included.

Factory is a very controversial pattern. It's good to use it, but I don't recommend to do everything through a factory.

George Merts
And even when there are two levels of nesting, it is obligatory to write comments after each closing parenthesis, which block it buries (say, duplicate loop header).

Well, that's why you write it in brackets in the ugly way of MQL. If you wrote it that way:

if(OrderSelect())
{
   ...
}

You would always see which bracket closes which code block.

There would be a dozen of other warnings in your code. Of course, the code is not perfect, but you can feel the author's taste for beauty :))

[Georgiy Merts]

Vasiliy Sokolov:

A bit of a criticism:

О. That's the kind of discussion I love. So.

Why reinvent the wheel in the form of CMyObject, when there is a standard CObject that everyone understands?

The functionality of CObject and CArrayObj is obviously copied here. Why? Quick sorting is built in standard data containers. Use them.

CMyObject is an heir to the standard CObject, all lists and arrays in my code are descendants of CArray (and other standard library arrays). I hardly ever use standard array[] arrays.

And, of course, sorting and working with lists uses basic functionality of CObject.

And the difference between them is this: A standard CObject is "a list or sorted array object". A CMyObject is a CObject that has a certain type, and contains some value given when it was created. I needed this object because of widespread reduction of objects to basic abstract class - to understand by pointer which object "actually" points to. Type CMyObject is set by that very function SetMyObjectType (). This function must necessarily be called in the constructors of any derived from CMyObject, to assign an identifier to the class the object belongs to.

It also has SetUDCreationValue() - setting a user-defined value when created. Rarely used. It's needed to distinguish different objects of the same class.

If the class interface - its constructor is better to hide in the protected section. Then its object cannot be created directly.

Protected constructor ??? Yes, I guess it's reasonable for interfaces, I didn't know it was possible.

Define an empty constructor? I don't know. I wouldn't do that. It's better not to mention the destructor if you don't need it.

It's a "cursed legacy of the past". Once upon a time we wrote quite a large project and there if we didn't define an empty destructor, it took quite a long time to remove the objects for some reason. So, I've been doing it on the fly ever since. Generally speaking, destructor must also be virtual.

Non-standard increment iI, non-standard iteration ++iI, iHistoryDealsTotal - defined somewhere out there, far before the loop. It's better to keep it simpler:

Disagree. Increment is perfectly normal, - i, just standardised notation - first with a small letter its type is integer, and then with a capital letter its name is I.

You seem to be against such sheets, but you write it somewhere. No one wants to analyze such nonsense. What prevented you from writing it that way:

In this case I had to choose between "visibility" of the class and beauty of the function. I chose "visibility". The beauty suffered.

The giant Select method consists of a giant for loop:

Obviously, all the checks for the compliance of the transaction with the current Expert Advisor should be in a separate method, e.g. like this:

And in general, select needs to be split into 3-4 more methods to make it clear what's going on in it.

I agree. Here, in principle, this very cycle has "grown", at first it wasn't that big.

Although it's not always convenient to implement minor checks into private functions, as these checks are not always traceable in code.

Factory is a very controversial pattern. Use is fine, but I don't recommend doing everything through a factory.

Now I do not remember, there were several variants of building the Expert Advisor. I stopped at the "Expert Advisor parts factory". In principle, it is no longer a pure classic "factory" pattern. Originally, it was intended to be a "classic" pattern, but now it is rather a "constructor-concentrator" of Expert Advisor parts. And it is also responsible for removal of those parts, which is not characteristic of a factory. But the name remained.

That's why you write it in brackets in the ugly way of MQL. If you wrote it that way:

You'd always see which parenthesis closes which code block.

Why "the ugly way"?

The loop header, then the opening bracket with indentation, then the whole block with the same indentation, and finally the closing bracket - also with indentation.

What do you think is better?

[Georgiy Merts]

Gregory Kovalenko:

I need to get profits on 2 open orders. The last open order, I callOrderProfit2, and the next order -OrderProfit1.

First order is opened first, then the 2nd order, so the 1st order in the loop is called 2)

Where is the error?

You are just going through the orders. Nowhere does it check which one is first and which one is second.

You need to enter a check on the opening time. Then you can distinguish between the order which opened earlier and the order which opened later. Or perhaps they can open at the same time.