Embedded TRUE/FALSE options - move on to next function?

New comment
 

Hello,


I am in need of some help regarding an EA I have programmed that has indicators which can be set to true or false, currently I use an else statement to call another function that will run if the existing function is set to false.

The problem with this is that I end up with a huge list of functions, one for each indicator! This brings up errors in the journal when testing.

I need to have one function that will check if the technical analysis is set to true/false (properties) then run it if it is and go onto the next one or not run it if it is set to false and go onto the next one.

My apologies on how messy (bad) my code is, that is why I am posting here to look how I can improve it. Thank you for your time.

void MACD()
{
if (MACD1==1)
 {
if ((iMACD(NULL, NULL,12,26,9,PRICE_CLOSE,MODE_MAIN,0) > iMACD(NULL, NULL,12,26,9,PRICE_CLOSE,MODE_MAIN,1))&&
    (iMACD(NULL, NULL,12,26,9,PRICE_CLOSE,MODE_MAIN,1) > iMACD(NULL, NULL,12,26,9,PRICE_CLOSE,MODE_MAIN,2))
)
    {
        MACD2();
        
    }
    }
   else
    SMA();
}


void MACD2()
{
if (MACD2==1)
 {
if ((iMACD(NULL, NULL,12,26,9,PRICE_CLOSE,MODE_MAIN,2) > iMACD(NULL, NULL,12,26,9,PRICE_CLOSE,MODE_MAIN,3))&&
    (iMACD(NULL, NULL,12,26,9,PRICE_CLOSE,MODE_MAIN,3) > iMACD(NULL, NULL,12,26,9,PRICE_CLOSE,MODE_MAIN,4))
)
    {
        SMA();
        
    }
 }
   else
    SMA();
}

void SMA()
{
if (SMA1==1)
 {
if  ((iMA(NULL, NULL,14,0,MODE_SMA,PRICE_CLOSE,0) > iMA(NULL, NULL,21,0,MODE_SMA,PRICE_CLOSE,0)) && 
    (iMA(NULL, NULL,14,0,MODE_SMA,PRICE_CLOSE,1) > iMA(NULL, NULL,21,0,MODE_SMA,PRICE_CLOSE,1)))
    {
        SMA2();
    }
 }
   else
    RSI();
}

void SMA2()
{
if (SMA2==1)
 {
if  ((iMA(NULL, NULL,14,0,MODE_SMA,PRICE_CLOSE,2) > iMA(NULL, NULL,21,0,MODE_SMA,PRICE_CLOSE,2)) && 
    (iMA(NULL, NULL,14,0,MODE_SMA,PRICE_CLOSE,3) > iMA(NULL, NULL,21,0,MODE_SMA,PRICE_CLOSE,3)))
    {
        RSI();
    }
 }
   else
    RSI();
}
 
gangsta1:

Hello,


I am in need of some help regarding an EA I have programmed that has indicators which can be set to true or false, currently I use an else statement to call another function that will run if the existing function is set to false.

The problem with this is that I end up with a huge list of functions, one for each indicator! This brings up errors in the journal when testing.

I need to have one function that will check if the technical analysis is set to true/false (properties) then run it if it is and go onto the next one or not run it if it is set to false and go onto the next one.

Put it in one function that returns a bool,  if the first test of the MA is no good return false and the function ends,  if the MA test is OK go onto the next test,  if this fails return false and the function ends,  if not go on to the next etc, etc.
 
Simplify your code and you'll start to see. I'll start with one function:
Original Code:
void MACD2()
{
if (MACD2==1)
 {
if ((iMACD(NULL, NULL,12,26,9,PRICE_CLOSE,MODE_MAIN,2) > iMACD(NULL, NULL,12,26,9,PRICE_CLOSE,MODE_MAIN,3))&&
    (iMACD(NULL, NULL,12,26,9,PRICE_CLOSE,MODE_MAIN,3) > iMACD(NULL, NULL,12,26,9,PRICE_CLOSE,MODE_MAIN,4))
)
    {
        SMA();
        
    }
 }
   else
    SMA();
}
Standardize your indenting. Factor common code. The second argument to iMACD is an int not a string.
double getMACD(int shift){
   return( iMACD(NULL,0,12,26,9,PRICE_CLOSE,MODE_MAIN,shift) );
}
void MACD2()
{
   if (MACD2==1)
   {
      if ((getMACD(2) > getMACD(3))&&
          (getMACD(3) > getMACD(4))
      ){
         SMA();
      }
   }
   else
      SMA();
}
Was does MACD2==1 mean? Write self-docu- menting code.
#define MACD2_DISABLE   0         // \  This form of enumeration
#define MACD2_ENABLE    1         //  > is when you have multiple 
extern int MACD2 = MACD2_ENABLE;  // /  options (more than two.
void MACD2(){
   if (MACD2==MACD2_ENABLE){
      if (getMACD(2) > getMACD(3) && getMACD(3) > getMACD(4)){
         SMA();
      }
   }
   else
      SMA();
}
Use booleans when there are only two options.
extern bool useMACD2 = true;
void MACD2(){
   if (useMACD2){
      if (getMACD(2) > getMACD(3) && getMACD(3) > getMACD(4)){
         SMA();
      }
   }
   else
      SMA();
}
Simplify code when possible.
void MACD2(){
   if (!useMACD2
   || (getMACD(2) > getMACD(3) && getMACD(3) > getMACD(4))
   )     SMA();
}
Don't embed code. MACD2 shouldn't call SMA. It should return something to the caller.
bool MACD2(){
   if (!useMACD2) return(true);
   if (getMACD(2) > getMACD(3) && getMACD(3) > getMACD(4) ) return(true);
   return(false)
}
// Caller should now be
   if (!MACD2()) return;
   SMA();
Simplify booleans. If(con) return(true) else return(false) is simply return(con)
bool MACD2(){
   return( !useMACD2 || (getMACD(2) > getMACD(3) && getMACD(3) > getMACD(4)) );
}
The renaming of MACD2() to something better like isMacd2Ok() is left as an exercise.
New comment