Looking for a friendly coding critique

[Tristen Shaw]

Hello all,

I have been working on learning how to code an EA from scratch. I started in MQL4 and then moved it over to MQL5. I've made some mistakes and reworked it several times. And I'm posting for a critique of my coding style and ability.

DISCLAIMER:

NO I am not looking for anyone to code or debug for me!I'm looking for input on things I need to root out of my habits in order to code better and need less help in the future.

Since I'm looking for a general coding critique, I shouldn't need to post "things I have tried" but I will anyway just to make sure I attempt to satisfy everyones posting standards.

- I have started trying to cut down on nested statements and to make better use of if statement shorthand. I still need to rework some of them.

- I coded this one with the idea of trying to make each of the functions as self contained as possible so that I could test them and not have to worry about breaking one functions by working on another.

-I have read the documentation page for any function I have had issues with, and I have watched dozens of videos and read articles.

If you don't like my post for whatever reason, please feel free to pass it by. No one is holding a gun to your head to reply to this.

int
   nextValidWAEDirection = 0,//0 = Either; 1 = Long; 2 = Short;
   lookingForTrade= 0;//0 = Not looking, 1 = Long, 2 = Short
bool
   isAlarmTriggered = false;
   
datetime 
   newCandleTime;

//+------------------------------------------------------------------+
//| Expert initialization function                                   |
//+------------------------------------------------------------------+
int OnInit()
  {
//---
   int test = qqeState() == 1 ||qqeState() == 3 ? 2 : 1;
   Print(test);
   nextValidWAEDirection = test;
   
//---
   return(INIT_SUCCEEDED);
  }
//+------------------------------------------------------------------+
//| Expert deinitialization function                                 |
//+------------------------------------------------------------------+
void OnDeinit(const int reason)
  {
//---
   
  }
//+------------------------------------------------------------------+
//| Expert tick function                                             |
//+------------------------------------------------------------------+
void OnTick()
  {
//---
   //if(isAlarmTriggered) Alert("Trade trigger!");
   if(!IsNewCandle()) return;
   isPrimaryTriggered();   
   if(lookingForTrade > 0){
      //Alert("Looking for trade");
   }
  }
//+------------------------------------------------------------------+
//| Trade function                                                   |
//+------------------------------------------------------------------+
void OnTrade()
  {
//---
   
  }
//+------------------------------------------------------------------+
//| TradeTransaction function                                        |
//+------------------------------------------------------------------+
void OnTradeTransaction(const MqlTradeTransaction& trans,
                        const MqlTradeRequest& request,
                        const MqlTradeResult& result)
  {
//---
   //islostTrade();
  }
//+------------------------------------------------------------------+

bool IsNewCandle()
{
   if (newCandleTime == iTime(Symbol(), 0, 0)) return false;
         
   newCandleTime = iTime(Symbol(), 0, 0);     
   return true;   
}
void isLostTrade(){
   //Determin if the last trade was a loss and set nextValidWAEDirection to 0
}

void findNextSTPoint(){
   
}

bool isPrimaryTriggered(){
   int
      mHandle = iCustom(_Symbol,_Period,"Waddah Attar Explosion",
                     20,
                     40,
                     20,
                     2,
                     150,
                     400,
                     15,
                     150);
                     
   double 
      mValue[1],
      mSignal[1],
      mDeadZone[1],
      mColor[1];
   
   //Index: 0 = Bar Value 1 = Color; 2 = Signal Line; 3 = Deadzone
   //Bar Color: 1 = Green; 2 = Red
   CopyBuffer(mHandle, 0, 1, 1, mValue);
   ArraySetAsSeries(mValue, true);
   
   CopyBuffer(mHandle, 1, 1, 1, mColor);
   ArraySetAsSeries(mColor, true);
   
   CopyBuffer(mHandle, 2, 1, 1, mSignal);
   ArraySetAsSeries(mSignal, true);
   
   CopyBuffer(mHandle, 3, 1, 1, mDeadZone);
   ArraySetAsSeries(mDeadZone, true);
   
   //Determine if the last bar is valid. Meaning it's above the deadzone and is of the correct color based on the nextValidWAEDirection Var
   //nextValidWAEDirection Variable: 0 = Either; 1 = Long; 2 = Short
   //if (nextValidWAEDirection == 1 && qqeState() == (1 || 3)) || (nextValidWAEDirection == 2 && qqState() == (2 || 4))   
   if(mValue[0] < mDeadZone[0]){ return false; }
   if(nextValidWAEDirection == 0 || mColor[0] != nextValidWAEDirection){      
         nextValidWAEDirection = nextValidWAEDirection <= 1 ? 2 : 1;
         lookingForTrade = nextValidWAEDirection;
         isAlarmTriggered = true;
         //Print("Color: ", mColor[0]);
         Alert("Valid bar");
         Print("Next Valid Bar: " + nextValidWAEDirection == 1 ? "Up" : "Down");
         return true;      
   }   
   return true;
}

void lookForValidSAR(int mTradeDirection){
   //Is the current candle within 2 of a SAR point
   //If not, wait for the next SAR trend.
   int
      mHandle = iSAR(_Symbol, PERIOD_CURRENT, 0.03, 0.2);
   double
      mSARBuffer[1];
      
   CopyBuffer(mHandle, 0, 1, 1, mSARBuffer);
   ArraySetAsSeries(mSARBuffer, true);
   
   if(sarPosition(mSARBuffer[0], 1) == mTradeDirection){
      bool run = true;
      int i = 1;
      while(run){
         if(sarPosition(mSARBuffer[0], i) == mTradeDirection){ 
            i++;
         }else{
            run = false;
         }
      }//End of While
      if(i < 2){
         lookingForTrade = 0;
         //takePosition(); 
      }
   }   
}//End Function

int sarPosition(double mSar, int mBar){

   if(mSar > iHigh(_Symbol, PERIOD_CURRENT, mBar)){ return 2; }
   return 1;
   
}

int qqeState(){
   //Returns state: 1 = Up; 2 = Down; 3 = Upturn; 4 = Downturn;
   int   
      mHandle = iCustom(_Symbol,_Period,"MTF QQE",
                     _Period,
                     100,
                     20,
                     2.618,
                     4.236,
                     PRICE_CLOSE,
                     true);
   double      
      mSignalBuffer[1],
      mFastBuffer[1],
      mPrvSignalBuffer[1],
      mPrvFastBuffer[1];
      
      CopyBuffer(mHandle, 0, 1, 1, mFastBuffer);      
      ArraySetAsSeries(mFastBuffer,true);      
      
      CopyBuffer(mHandle, 2, 1, 1, mSignalBuffer);
      ArraySetAsSeries(mSignalBuffer, true);
      
      CopyBuffer(mHandle, 2, 2, 1, mPrvSignalBuffer);      
      ArraySetAsSeries(mPrvSignalBuffer,true);
      
      CopyBuffer(mHandle, 0, 2, 1, mPrvFastBuffer);
      ArraySetAsSeries(mPrvFastBuffer, true);      
      
      //Print(mSignalBuffer[0], " || ", mFastBuffer[0]);
      //Print(mPrvSignalBuffer[0], " || ", mPrvFastBuffer[0]);
            
      if(mSignalBuffer[0] > mFastBuffer[0]){//Down
         if(mSignalBuffer[0] < mPrvSignalBuffer[0]){ Print("Upturn"); return 3; }                 
         Print("Uptrend");
         return 1; 
      }else{//Up
         if(mSignalBuffer[0] > mPrvSignalBuffer[0]){ Print("Downturn"); return 4; }         
         Print("Downtrend");
         return 2;
      }      
}

void PlaceOrder( int nCurBias, double dbLots, int nStopLoss, int nSlippage, int nmaxSpread, int nmagicNumber )
{
   if( OrdersTotal() == 0 )
   {
      int nSpread = (int) SymbolInfoInteger( _Symbol, SYMBOL_SPREAD );
      
      if( nSpread < nmaxSpread )
      {
         bool bBuy = nCurBias == 0;
      
         double
            dbAtr             = GetATR( 1 ),
            dbSpread          = _Point * nSpread,
            dbStopLoss        = _Point * nStopLoss,
            dbStopsLevel      = _Point * SymbolInfoInteger( _Symbol, SYMBOL_TRADE_STOPS_LEVEL ),
            dbStopLossRange   = fmax( dbSpread + dbStopLoss, dbStopsLevel ),
            dbTakeProfitRange = fmax( dbSpread + dbAtr     , dbStopsLevel ),
            dbDirection       = bBuy ? +1.0 : -1.0,
            dbPrice           = bBuy ? Ask : Bid,
            dbStopLossPrice   = Round2Ticksize( dbPrice - dbDirection * dbStopLossRange   ),
            dbTakeProfitPrice = Round2Ticksize( dbPrice + dbDirection * dbTakeProfitRange );

         int nTicket = OrderSend( _Symbol, bBuy ? OP_BUY : OP_SELL,
            dbLots, dbPrice, nSlippage, dbStopLossPrice, dbTakeProfitPrice,
            "Primary Order", nmagicNumber, 0, bBuy ? clrBlue : clrMagenta );
            
         int nTicket2 = OrderSend( _Symbol, bBuy ? OP_BUY : OP_SELL,
            dbLots/2, dbPrice, nSlippage, dbStopLossPrice, dbTakeProfitPrice*2,
            "Trailing Order", nmagicNumber, 0, bBuy ? clrBlue : clrMagenta );
            
         if( nTicket > 0 && nTicket2 > 0)
         {
            Alert( "Order Fired!" );
         }
         else
         {
            Print("Error: ", GetLastError());
         };
      };
   };
}

Not all functions are in use yet. But, before I get too far into this project, I want to make sure I'm not ingraining any horrible practices into my coding. I would rather learn to do things right and move on from there.

That being said, please try to be kind. I'm new to this and am indeed putting a lot of effort into learning.

[William Roeder]

Tristen Shaw: 'm looking for input on things I need to root out of my habits in order to code better and need less help in the future.

1. int
      nextValidWAEDirection = 0,//0 = Either; 1 = Long; 2 = Short;
      lookingForTrade= 0;//0 = Not looking, 1 = Long, 2 = Short
   ⋮
       int test = qqeState() == 1 ||qqeState() == 3 ? 2 : 1;
       enumeration Direction{eEither, eLong, eShort};
   

   Don't use ints when you mean enumerations.It also makes your code self-documenting.
   

   enumeration Status{eIdle, eBuy, eSell};
   Direction nextValidWAEDirection = either;
   Statis    lookingForTrade= eEither;
   ⋮
      int test = qqeState();
      Print(test);
      nextValidWAEDirection = test == 1 || test == 3 ? eShort : eLong;

2. № 1: Don't cut-and-paste code. call the function once, use twice.

3. № 1: What does one (1) and three (3) mean? A different enumeration?

4.    if(!IsNewCandle()) return;

   I disagree with making a new bar function, because it can only be called once per tick. A variable can be tested multiple times.
             Running EA once at the start of each bar - MQL4 programming forum (2011)

5.    isPrimaryTriggered();

   That looks like it returns a boolean, but you don't check the value.

6. bool isPrimaryTriggered(){
      int
         mHandle = iCustom(_Symbol,_Period,"Waddah Attar Explosion",

   Perhaps you should read the manual, especially the examples.
      How To Ask Questions The Smart Way. (2004)
         How To Interpret Answers.
            RTFM and STFW: How To Tell You've Seriously Screwed Up.

   They all (including iCustom) return a handle (an int). You get that in OnInit. In OnTick/OnCalculate (after the indicator has updated its buffers), you use the handle, shift and count to get the data.
             Technical Indicators - Reference on algorithmic/automated trading language for MetaTrader 5
             Timeseries and Indicators Access / CopyBuffer - Reference on algorithmic/automated trading language for MetaTrader 5
             How to start with MQL5 - General - MQL5 programming forum - Page 3 #22 (2020)
             How to start with MQL5 - MetaTrader 5 - General - MQL5 programming forum - Page 7 #61 (2020)
             How to call indicators in MQL5 - MQL5 Articles (2010)

7. void PlaceOrder( int nCurBias, double dbLots, int nStopLoss, int nSlippage, int nmaxSpread, int nmagicNumber )
   {
      if( OrdersTotal() == 0 )

   Magic number only allows an EA to identify its trades from all others. Using OrdersTotal/OrdersHistoryTotal (MT4) or PositionsTotal (MT5), directly and/or no Magic number/symbol filtering on your OrderSelect / Position select loop means your is incompatible with every EA (including itself on other charts and manual trading.)
   

          Symbol Doesn't equal Ordersymbol when another currency is added to another seperate chart . - MQL4 programming forum (2013)
          PositionClose is not working - MQL5 programming forum (2020)
          MagicNumber: "Magic" Identifier of the Order - MQL4 Articles (2006)
          Orders, Positions and Deals in MetaTrader 5 - MQL5 Articles 2011

You need one Magic Number for each symbol/timeframe/strategy. Trade current timeframe, one strategy, and filter by symbol requires one MN.

8.          int nTicket = OrderSend( _Symbol, bBuy ? OP_BUY : OP_SELL,
               dbLots, dbPrice, nSlippage, dbStopLossPrice, dbTakeProfitPrice,
               "Primary Order", nmagicNumber, 0, bBuy ? clrBlue : clrMagenta );

   The above is MT4 call, but you posted in the MT5 general section and posted MT5 code.
   

[Tristen Shaw]

William Roeder #:

1. I disagree with making a new bar function, because it can only be called once per tick. A variable can be tested multiple times.
             Running EA once at the start of each bar - MQL4 programming forum (2011)

Ok, I'm surprised that was all to be honest. But thank you for the input, I'm working on putting it into practice and doing some follow up research.

As for the new bar function. I have the same issue with the one in your link as I do with the one I was using before. It's very common for things to go off on init even if they aren't in the init function. I've searched and searched for an explanation but can't find one anywhere.

//+------------------------------------------------------------------+
//| Variables                                                        |
//+------------------------------------------------------------------+
   static datetime lastBarOpenAt;

//+------------------------------------------------------------------+
//| Expert initialization function                                   |
//+------------------------------------------------------------------+
int OnInit()
  {

   
   return(INIT_SUCCEEDED);
  }
//+------------------------------------------------------------------+
//| Expert deinitialization function                                 |
//+------------------------------------------------------------------+
void OnDeinit(const int reason)
  {
//---   
   
  }
//+------------------------------------------------------------------+
//| Expert tick function                                             |
//+------------------------------------------------------------------+
void OnTick()
  {
      bool isNewBar = lastBarOpenAt != iTime(_Symbol, PERIOD_CURRENT, 0);
      if (isNewBar){ lastBarOpenAt = iTime(_Symbol, PERIOD_CURRENT, 0);
         Alert("New bar");  
      }
  }

[Dominik Egert]

For general coding styles, there are numerous tutorials on the web for clean and structured c/c++ coding habits.

I personally would suggest to use as much const declarations as possible for variables. Also I suggest to try to remove any if statements from loops, while and for.

Also I suggest using procedural approaches for execution logic and object oriented for data management.

You should always assign a value to a variable on declaration.

For speed, consider the amount of parameters for a function. The first three parameters (per Parameter 64 bit) go into registers of the CPU. All other will be pushed to stack.

Also you might want to design your variable declarations always from biggest to smallest within a function. While arrays are pointers and have a size of 64bit. Try to avoid misalignment.

Functions can cache values by utilizing static variables. This way you can save calculations which would be repeated, but are deterministic to the execution of the program. (Don't change).

I declare all variables at the top of a function.

I try to use ternary operator if possible "?" instead of if statements, especially in loops.

These are my basic guidelines I apply to my code.

Maybe I am missing some, due to unconsciousness...

[Dominik Egert]

Oh, one more comment on mql 5.

Print and printf will raise an error code if only fed one parameter. Try to get into the habit of using format specifiers to format your strings.

Use printf and Stringformat. Always have a format specifier in the first parameter. Even if it's just the "%s" to output a string.

And always clear the error code variable ResetLastError within a function that's using GetLastError.

Also you should check your return values from API functions like Copy buffer() > 0, or whatever you expect, so you can manage failed operations.

Don't assume, check.

Concerning boolean evaluations, try using brackets to clarify operation and result assignment.

bool eval = (bVar == 1);

Makes it more convenient to read.

Edit:

You should always make use of the _StopFlag in all of your loops.

This way you do not get "hung" executions in your terminal.

[Tristen Shaw]

Dominik Christian Egert #:

Oh, one more comment on mql 5.

Print and printf will raise an error code if only fed one parameter. Try to get into the habit of using format specifiers to format your strings.

Use printf and Stringformat. Always have a format specifier in the first parameter. Even if it's just the "%s" to output a string.

And always clear the error code variable ResetLastError within a function that's using GetLastError.

Also you should check your return values from API functions like Copy buffer() > 0, or whatever you expect, so you can manage failed operations.

Don't assume, check.

Concerning boolean evaluations, try using brackets to clarify operation and result assignment.

bool eval = (bVar == 1);

Makes it more convenient to read.

Edit:

You should always make use of the _StopFlag in all of your loops.

This way you do not get "hung" executions in your terminal.

Very informative tips! I appreciate your input!