Help me make the code better

General
1

Hello Community.
Im in the process of learning to program and im wondering what best practise i should use.

I have this code. it will in a future when im done check value of water and report if its bad.

    #include "clickButton/clickButton.h"
    const int pinRelay_RedLed = D0; // Relay and RedLed is on same, if relay turns on the water is bad. 
    const int pinResetButton = D1; //the pin where we connect the reset button
    const int pinOsmosis_Stage2 = D2; //the pin where we connect the Osmosis EC meter stage 2 cable
    const int pinOsmosis_Stage3 = D3; //the pin where we connect the Osmosis EC meter stage 3 cable
    const int pinGreenLed = D6; //the pin where we connect the Green led
    const int pinYellowLed = D7;  //the pin where we connect the Yellow led
    //------------------------------------------------------------------------
    //---- Here i declare the state check for the inputs and also activate the pin --------------------
    ClickButton stateResetButton(pinResetButton, HIGH, true)
    ClickButton stateOsmosis_Stage2(pinOsmosis_Stage2, HIGH, true);
    ClickButton stateOsmosis_Stage3(pinOsmosis_Stage3, HIGH, true);
    //-----------------------------------------------------------------------
    //---- Herer i declare the function i will use ---------------------------
    void TurnOnYellowLed();
    void TurnOnRelay_RedLed();
    void PressResetButton();
    //-----------------------------------------------------------------------
        void setup() {
        //---- We declare respective digital point as outputs and inputs here ----
          pinMode(pinRelay_RedLed, OUTPUT); // Its an output since it need to send a voltage to Relay and Red Light if   water is bad
          pinMode(pinGreenLed, OUTPUT); // Its an output since it need to send a voltage Green led
          pinMode(pinYellowLed, OUTPUT); // Its an output since it need to send a voltage to Yellow led
        //----------------------------------------------------------------------
        //---- We declare the state of each variable when the units gets powered on ----
          digitalWrite(pinGreenLed, HIGH); // The GreenLed should always be on if water is good, if water is going bad This one will be LOW
        //----------------------------------------------------------------------  
        //---- For troubleshooting ---------------------------------------------
          Serial.begin(9600); // Open a serial port
        //----------------------------------------------------------------------
        //---- Here is some criterias for the inputs ----
          // Setup button timers (all in milliseconds / ms)
          // (These are default if not set, but changeable for convenience)
          stateResetButton.debounceTime   = 20;   // Debounce timer in ms
          stateResetButton.multiclickTime = 250;  // Time limit for multi clicks
          stateResetButton.longClickTime  = 1000; // time until "held-down clicks" register
          stateOsmosis_Stage2.debounceTime   = 20;   // Debounce timer in ms
          stateOsmosis_Stage2.multiclickTime = 250;  // Time limit for multi clicks
          stateOsmosis_Stage2.longClickTime  = 5000; // time until "held-down clicks" register
          stateOsmosis_Stage3.debounceTime   = 20;   // Debounce timer in ms
          stateOsmosis_Stage3.multiclickTime = 250;  // Time limit for multi clicks
          stateOsmosis_Stage3.longClickTime  = 5000; // time until "held-down clicks" register
        //----------------------------------------------------------------------
        }
        void loop() {
        //---- Here we update the status of all Input pin to see if they are 1 or 2,3,-1,-2,-3-----
          stateResetButton.Update();
          stateOsmosis_Stage2.Update();
          stateOsmosis_Stage3.Update();
        //----------------------------------------------------  
        //---- Here we tell the controller what to do if the status of the inputs meet certain criterias ----
          switch(stateResetButton.clicks != 0){ // Checks all possible values and as long its not "0" call function
            case 0:
              // do nothing for 0
              break;
            //case x:
            //  do whatever for x
            //  break;  
            default:  // all non explicitly dealt cases, The function should be called whatever value
              PressResetButton();
              break;
          }
          if(stateOsmosis_Stage3.clicks == -1) TurnOnRelay_RedLed(); // If HIGH for 60 seconds call function, -1 is waiting for a long press (60 sec)
          if (stateOsmosis_Stage3 == LOW){
            if(stateOsmosis_Stage2.clicks == -1) TurnOnYellowLed(); // If HIGH for 60 seconds call function, -1 is waiting for a long press (60 sec)
          }
        //-----------------------------------------------------
        }
        //-----------------------------------------------------------------------------
        //---- Here we write down all functions thats gets called under the loop() ----
        void TurnOnYellowLed(){ // This function is called when EC meters stage 2 is HIGH
              digitalWrite(pinGreenLed, LOW); // Turn off green led
              digitalWrite(pinYellowLed, HIGH); // Turn on yellow led 
              Serial.println("please arrange for a Osmosis filter change");
              Particle.publish("Osmosis filter soon bad", NULL, 60, PRIVATE);
        }
        
        
        void TurnOnRelay_RedLed(){ // This function is called when EC meter stage 3 is HIGH
             digitalWrite(pinGreenLed, LOW); // Turn off green led 
             digitalWrite(pinYellowLed, LOW); // Turn off yellow led
             digitalWrite(pinRelay_RedLed, HIGH); // Turn on relay and red led
             Serial.println("Please check/change Osmosis filter");
             Particle.publish("System shut down", NULL, 60, PRIVATE);
        }
        
        void PressResetButton(){ // This function is called when reset button is pressed
             digitalWrite(pinGreenLed, HIGH); // Turn on green led
             digitalWrite(pinYellowLed, LOW); // Turn off yellow led
             digitalWrite(pinRelay_RedLed, LOW); // Turn off relay and red led
             Serial.println("All values are set to default");
             Particle.publish("Reset press detected", NULL, 60, PRIVATE);
        }
        //-----------------------------------------------------------------------------
  1. I have seen on some references that everything in the loop is very clean and everything i have here should be called under functions instead, is there any pro/con for this?.
  2. i have some if statements, im not sure if this is the best way to go, What i want to happen is that if red led is already active there is no use to activate yellow led, however the yellow led should have been activated before the red led, its like a traffic light. Water is good = green led, water is going bad = yellow led, Water is bad = red led
  3. Any obvious way i can improve the code or make it more readable?

Many thanx
/Dimi

2

For readability, add some blank lines between blocks of code that create a functional entity.

Cut on comments that don’t add information or just state the obvious (e.g. use // --------------------- very sparingly, blank lines might do and make reading easier)

Have one consistent indentation depth throughout your code.

Where possible;
Use arrays over individual variables.
Don’t copy code blocks, rather go for loops or functions - where sensible and possible.

Using functions is considered better practice than “marathon code”, but one-line functions are not helping much either - use common sense.

Use expressive but not overly long var names. Var name meanings can be explained in the comments at their declaration. This keeps the code more readable too.

My personal preference is to have inline comments to be vertically aligned - where possible and sensible
Also if I have longer blocks of assignments, I align the assignment operator (=) vertically.
For functional if branches, I’d not use the one-line format

  // instead of
  if(stateOsmosis_Stage3.clicks == -1) TurnOnRelay_RedLed(); // If HIGH for 60 seconds call function, -1 is waiting for a long press (60 sec)
  // I'd prefer this
  if(stateOsmosis_Stage3.clicks == -1) // If HIGH for 60 seconds call function, -1 is waiting for a long press (60 sec)
    TurnOnRelay_RedLed(); 

  // exception "non-functional" 
  if (somCondition) return;

BTW: I guess I gave you some dodgy code in the other thread here

    switch(stateResetButton.clicks != 0){ // Checks all possible values and as long its not "0" call function
      ...
    }

This should be

    switch(stateResetButton.clicks){ // Checks all possible values and as long its not "0" call function
      ...
    }

I’ve corrected it there too :blush:

4 Likes
3

Haha thank you :slight_smile:

You are correct, i removed all //----- iand it made it easier to read, i thought first it was the opposite and it would make me understand better what is happening. I get your point that it should only be where i need to explain the not so obvious.

Im happy with the code so far, it will be a bigger sketch but i dont have all hardware i need yet.
meanwhile i will start to learn the cloud part.

Again ScruffR, thank you so much.
Its worth a lot to know that even for my low level questions someone will take the time to answer them.

BR
Dimi

1 Like