General question about 'clean' code

glb_sq · October 20, 2024, 3:46pm

Question about the readability of code.

Say for example I have a sketch that uses switches and LEDs. My code has enums, structs, global declarations, and function prototypes that are specific to each, all in the main sketch file.

Which of the two examples below are the more common and/or proper way to arrange the sketch for readability? I have my code arranged like method 1, which I feel puts everything in the right place, but I feel like method 2 gives a more organized look into how the code works.

If my code was bigger, I would put the things into separate files, but I was wondering what's the best practice for when it's desired to have everything in one file.

Just wondering and looking to learn something today.

method 1:

switch defines
LED defines

switch enums
LED enums

switch declarations
LED Decarations

switch functions
LED functions

method 2:

switch defines
switch enums
switch declarations
switch functions

LED defines
LED enums
LED declarations
LED functions

sonofcy · October 20, 2024, 4:18pm

There are on-line documents on coding style even including how to name variables. We will never get agreement. Arduino itself eschews the use of defines and I agree.

LarryD · October 20, 2024, 4:19pm

That a very subjective subject.
Style is a personal thing unless your boss says do it this way.
Here is a style I’ve adopted.


//
//
//  WEB link
//
//
//  Version    YY/MM/DD    Comments
//  =======    ========    ====================================================================
//  1.00       --/--/--    Running code
//
//
//
 
//********************************************^************************************************
//For diagnostics on an UNO, ProMini or Nano
//
//PINB5 = 13, PINB4 = 12, PINB3 = 11, PINB2 = 10, PINB1 = 9, PINB0 = 8
//
//Macro definition for generating a 63ns pulse on UNO pin 13 i.e. PINB5
//
//add this line to setup()
//pinMode(13,OUTPUT);
 
#define PULSE62_13       cli(); PINB = bit(PINB5); PINB = bit(PINB5); sei()
 
 
//********************************************^************************************************
//                       S e r i a l   O R   P a r a l l e l   L C D   ?
//********************************************^************************************************
 
//uncomment the next line if you have a serial LCD                                 <-----<<<<<
#define serialLCDisBeingUsed
 
#ifdef  serialLCDisBeingUsed
 
#include <Wire.h>
 
//Use I2C library:     https://github.com/duinoWitchery/hd44780
//LCD Reference:       https://www.arduino.cc/en/Reference/LiquidCrystal
 
#include <hd44780.h>   //main hd44780 header
 
//NOTE:
//hd44780_I2Cexp control LCD using I2C I/O expander backpack (PCF8574 or MCP23008)
//hd44780_I2Clcd control LCD with native I2C interface (PCF2116, PCF2119x, etc...)
 
#include <hd44780ioClass/hd44780_I2Cexp.h> //I2C expander i/o class header
 
//If you do not know what your I2C address is, first run the 'I2C_Scanner' sketch
//OR
//run the "I2CexpDiag" sketch that comes with the hd44780 library
//hd44780_I2Cexp lcd(0x3F);
hd44780_I2Cexp lcd(0x27);
 
//********************************************^************************************************
#else
 
#include <LiquidCrystal.h>
 
// LCD pin         4   6  11  12  13  14
//                RS  EN  D4  D5  D6  D7
LiquidCrystal lcd( 4,  5,  6,  7,  8,  9);
 
#endif
 
//                                       M a c r o s
//********************************************^************************************************
#define RESET              0
 
#define VALID              true
#define INVALID            false
 
#define EXPIRED            true
#define stillTIMING        false
 
#define ENABLED            true
#define DISABLED           false
 
#define LEDon              HIGH    //+5V---[220R]---[LED]---PIN
#define LEDoff             LOW
 
#define OPENED             HIGH    //+5V---[Internal 50k]---PIN---[switch]---GND 
#define CLOSED             LOW
 
//#define OPENED             LOW   //+5V---[switch]---PIN---[10k]---GND
//#define CLOSED             HIGH
 
#define PRESSED            HIGH    //+5V---[Internal 50k]---PIN---[switch]---GND
#define RELEASED           LOW
 
 
//                                S w i t c h   s t u f f
//********************************************^************************************************
//a Structure that creates "switch" objects.
//if we read 3 same level changes in a row, we have a valid switch operation.
//if we sample the switches every 5ms, we can catch input changes ~15ms and longer in duration
 
struct defineSwitch
{
#define VALID             true
#define INVALID           false
 
  const byte              switchPin;
  byte                    switchState;
 
  byte                    debounceFlag;
  unsigned long           timeClosed;
 
  //****************************************
  //Function to check if there is a valid "switch" operation  (OPENED or CLOSED)
  bool checkSwitch()
  {
    byte currentState = digitalRead(switchPin);
    //****************************************
    //was there a change in state ?
    if (switchState != currentState)
    {
      //**************************
      //have we seen this change 3 times in a row ?
      if (debounceFlag++ >= 3)
      {
        //get ready for the next 3 readings
        debounceFlag = 0;
 
        //update to the new state
        switchState = currentState;
 
        //has the switch closed ?
        if (switchState == CLOSED)
        {
          //save the time the switch closed
          timeClosed = millis();
        }
 
        //the switch has opened
        else
        {
          //calculate how long was the switch closed
          timeClosed = millis() - timeClosed;
        }
 
        return VALID;
      }
    }
 
    //****************************************
    //there was no change in state detected
    else
    {
      debounceFlag = 0;
    }
 
    return INVALID;
  }
 
}; //END of   struct defineSwitch
 
//********************************************^************************************************
//create you switch objects
 
//****************************************
defineSwitch userSwitch =
{
  //switchPin, switchState (at power up time)
  2, OPENED
};
 
 
//                                  T I M E R   s t u f f
//********************************************^************************************************
//a Structure that creates TIMER objects
 
struct makeTimer
{
#define MILLIS        1
#define MICROS        1000  //we can use this value to divide into a variable to get milliseconds
 
#define ENABLED       true
#define DISABLED      false
 
#define YES           true
#define NO            false
 
  //timerType       = what kind of TIMER is this, MILLIS or MICROS
  //enableFlag      = tells us if this TIMER is ENABLED or DISABLED
  //restart         = do we start this TIMER again and again, YES or NO
  //interval        = delay time (ms/us) which we are looking for
 
  //intervalDefault = if used, this is the powerup default value for "interval"
  //intervalNew     = if used, this is the value we want to change "interval" to
 
  //
  //****************************************
  //You need a minimum number of member values to create TIMER object.
  //Example:
  //give this TIMER a name "myTimer"
  //   makeTimer myTimer =
  //   {
  //assign values to the members that make up this TIMER
  //timerType (MILLIS or MICROS), enableFlag, restart, interval, intervalDefault, intervalNew
  //     MICROS, ENABLED, YES, 200ul, 200UL, 200ul
  //   };
  //
  // You have access to:
  // Variables: myTimer.timerType, myTimer.enableFlag, myTimer.restart, myTimer.interval,
  //            myTimer.intervalDefault, myTimer.intervalNew
  //
  // Functions: myTimer.checkTime(), myTimer.enableTimer(), myTimer.disableTimer(),
  //            myTimer.expireTimer(), myTimer.restartTimer()
  //****************************************
 
  //these are the bare minimum "members" needed when defining a TIMER
  int           timerType;
  bool          enableFlag;
  bool          restart;
  unsigned long interval;
 
  //when you do not define the following members,
  //these will get initialized to 0 when a TIMER is instantiated
  unsigned long intervalDefault;
  unsigned long intervalNew;
  unsigned long previousTime;
  unsigned long currentTime;
 
  //****************************************
  //Function to check if this TIMER has expired ex: myTimer.checkTime();
  bool checkTime()
  {
    currentTime = getTime();
 
    //has this TIMER expired ?
    if (currentTime - previousTime >= interval)
      //Note: with delays of < 2 millis, use micros() and adjust "interval" as needed
    {
      //should this TIMER start again?
      if (restart == ENABLED)
      {
        //get ready for the next iteration
        previousTime = currentTime;
      }
 
      //this TIMER has expired
      return true;
    }
 
    //this TIMER has not expired
    return false;
 
  } //END of   checkTime()
 
  //****************************************
  //Function to enable and reset this TIMER, ex: myTimer.enableTimer();
 void enableTimer()
  {
    enableFlag = ENABLED;
 
    //initialize previousTime to current millis() or micros()
    previousTime = getTime();
 
  } //END of   enableTimer()
 
  //****************************************
  //Function to disable this TIMER, ex: myTimer.disableTimer();
  void disableTimer()
  {
    enableFlag = DISABLED;
 
  } //END of   disableTimer()
 
  //****************************************
  //Function to force this TIMER to expire ex: myTimer.expireTimer();
  void expireTimer()
  {
    //force this TIMER to expire
    previousTime = getTime() - interval;
 
  } //END of   expireTimer()
 
  //****************************************
  //Function to restart this TIMER ex: myTimer.restartTimer();
  void restartTimer()
  {
    //reset this TIMER
    previousTime = getTime();
 
  } //END of   restartTimer()
 
  //****************************************
  //Function to return the current time
  unsigned long getTime()
  {
    //return the time             i.e. millis() or micros()
    if (timerType == MILLIS)
    {
      return millis();
    }
 
    else
    {
      return micros();
    }
 
  }; //END of   getTime()
 
}; //END of   structure “makeTimer”
 
//********************************************^************************************************
//example:
//
//create and initialize a "test" TIMER object
//****************************************
//makeTimer test =
//{
//timerType (MILLIS or MICROS), enableFlag, restart, interval, intervalDefault, intervalNew
//          MILLIS,              DISABLED,     NO,    2000ul,      2000ul,          0ul
//this is a millis() based TIMER, it is disabled, it does not restart,
//its interval is 2 seconds, its default is 2 seconds, new value is 0ul
//};
 
//****************************************
makeTimer heartbeat =
{
  //timerType (MILLIS or MICROS), enableFlag, restart, interval, intervalDefault, intervalNew
  MILLIS, ENABLED, YES, 500ul
};
 
//****************************************
makeTimer switches =
{
  //timerType (MILLIS or MICROS), enableFlag, restart, interval, intervalDefault, intervalNew
  MILLIS, ENABLED, YES, 5ul
};
 
//****************************************
makeTimer stateMachine =
{
  //timerType (MILLIS or MICROS), enableFlag, restart, interval, intervalDefault, intervalNew
  MILLIS, ENABLED, YES, 50ul
};
 
//****************************************
makeTimer common =
{
  //timerType (MILLIS or MICROS), enableFlag, restart, interval, intervalDefault, intervalNew
  MILLIS, DISABLED, YES, 1000ul
};
 
 
//                           S t a t e   M a c h i n e   St u f f
//********************************************^************************************************
 
enum SM {Startup, State1, State2, State3};
 
SM machineState = Startup;
 
 
//                                    G P I O   P i n s
//********************************************^************************************************
 
//userSwitch                            2
const byte testLED                    = 3;
//used for a parallel LCD               4, 5, 6, 7, 8, 9
const byte heartbeatLED               = 13;
 
 
//                                       s e t u p ( )
//********************************************^************************************************
void setup()
{
  Serial.begin(115200);
 
  pinMode(userSwitch.switchPin, INPUT_PULLUP);
 
  pinMode(testLED, OUTPUT);
  pinMode(heartbeatLED, OUTPUT);
  //used for diagnostics                                                         <-------<<<<<
  //pinMode(13, OUTPUT);  //in this case pin 13 is the heartbeat LED
 
  //****************************************
  //LCD stuff
  lcd.begin(16, 2);
  lcd.clear();
 
  lcd.setCursor(0, 0);
  //                   111111
  //         0123456789012345
  //             Skeleton
  lcd.print("    Skeleton    ");
 
  lcd.setCursor(0, 1);
  //                   111111
  //         0123456789012345
  //              Sketch
  lcd.print("     Sketch     ");
 
} //END of   setup()
 
 
//                                        l o o p ( )
//********************************************^************************************************
void loop()
{
  //PULSE62_13;
 
  //****************************************                      h e a r t b e a t   T I M E R
  //is it time to toggle the heartbeatLED ?
  if (heartbeat.checkTime() == EXPIRED)
  {
    //Toggle the heartbeatLED
    digitalWrite(heartbeatLED, !digitalRead(heartbeatLED));
  }
 
  //****************************************                      s w i t c h e s   T I M E R
  //is it time to check the switches ?
  if (switches.checkTime() == EXPIRED)
  {
    checkSwitches();
  }
 
  //****************************************                      s t a t e M a c h i n e   T I M E R
  //is it time to check the the State Machine ?
  if (stateMachine.checkTime() == EXPIRED)
  {
    checkMachine();
  }
 
  //========================================
  //Other non blocking code goes here
  //========================================
 
} //END of   loop()
 
 
//                               c h e c k S w i t c h e s ( )
//********************************************^************************************************
void checkSwitches()
{
  //****************************************                      u s e r S w i t c h
  //is there a valid change in switch state ?
  if (userSwitch.checkSwitch() == VALID)
  {
    //************                    CLOSED
    //did the switch closed ?
    if (userSwitch.switchState == CLOSED)
    {
      //do something
 
      //toggle the testLED
      digitalWrite(testLED, !digitalRead(testLED));
    }
 
    //************                    OPENED
    //the switch opened
    else
    {
      //do something
 
      Serial.println(userSwitch.timeClosed);
    }
 
  } //END of   if (userSwitch.checkSwitch() == VALID)
 
  //****************************************                      end of this switch
 
 
  //========================================
  //next switch code
  //========================================
 
} //END of   checkSwitches()
 
 
//                                c h e c k M a c h i n e ( )
//********************************************^************************************************
//service the State Machine
void checkMachine()
{
  switch (machineState)
  {
    //****************************************
    case Startup:
      {
        //do startup stuff
 
        //set the common TIMER interval to 500ms
        common.interval = 500ul;
 
        common.enableTimer();
 
        //next state
        machineState = State1;
      }
      break;                //END of   case:
 
    //****************************************
    case State1:
      {
        //has the common TIMER expired ?
        if (common.checkTime() == EXPIRED)
        {
          //do something
 
          //as needed
          //common.disableTimer();
        }
      }
      break;                //END of   case:
 
    //****************************************
    case State2:
      {
        //do something
      }
      break;                //END of   case:
 
    //****************************************
    case State3:
      {
        //do something
      }
      break;                //END of   case:
 
  } //END of  switch/case
 
} //END of   checkMachine()
 
 
//********************************************^************************************************
//                                E N D   O f   S k e t c h
//********************************************^************************************************

gcjr · October 20, 2024, 4:20pm

may be worth considering what you would put in a separate .cpp and .h file and this may help with organizing within a single file.

the definitions for the functions, and any variables would be in the ,cpp. the .h would include and declarations for functions and that would eb called from some other files. If the enums are needs as arguments or return values from those functions, they would be in the .h file.

declarations wouldn't normally be needed in a single file containing the definition.

i believe most people expect to find definition/declarations and enums near the top of a file

"nothing worse than no comments are wrong comments.

sonofcy · October 20, 2024, 4:36pm

I much prefer NO comments when I am debugging someone's code. When listings were paper I would cover them up, today I copy the file and delete them, then reformat the code to my style. Often after that the problem solves itself.
When I write code my loop variables are NEVER i, they are real world names like numOfCyls.

Dave_Lowther · October 20, 2024, 5:26pm

As others have said, you rarely find agreement on coding style.
The most important thing is that you understand it now, and if / when you come back to look at it in the future.
If someone else is going to read your code, then it's sensible to follow some standard that others will find readable.
One of the things that bugs me, about the single ino files, that some people post when asking for help, is having variables and constants declared as globals, when they are only used by one function.
If variables or constants are only used in one function, then declare them in that function. If a variable is required to keep its value between calls to that function, then declare it using the 'static' keyword.
#defines that can be replaced with constants should be, particularly if one wants to localise them to a single function.

xfpd · October 20, 2024, 5:49pm

Programming as an art.
Programming as a science.

If you write for yourself, structure for ease of use or reading, using the art of programming.

If you write for the rest of the world, structure code by standards of the science side of programming. Standard structure will allow "AI" to get beyond "almost guessed it," with only one, standard way to print "Hello, World!"

alto777 · October 20, 2024, 6:42pm

Method 2.

Put all the stuff for each separate thing in its own place. This goes beyond LEDs and switches to include displays and sensors and things that go together functionally., like enum sand variables supporting a FSM.

All at the top where ppl will see it on their way to your setup and loop.

a7

PaulRB · October 20, 2024, 7:06pm

Did you mean forward declarations?

If so, these should not be used in a single .ino file in my opinion. They are 99.999% never required anyway. With a single source code file, the only time you need forward declarations is where you have two mutually recursive functions (function A calls function B and function B calls function A) and that's extremely rare! Avoid forward declarations simply be defining your functions before that are called.

gcjr · October 20, 2024, 7:43pm

probably the best way to learn is to debug someone elses code

system · April 18, 2025, 7:43pm

This topic was automatically closed 180 days after the last reply. New replies are no longer allowed.

Topic		Replies	Views
Suggestions for the Arduino style guide General Discussion	3	2423	May 6, 2021
Keep track of code/snippets Programming	23	3962	May 5, 2021
Proper Code writing style Programming	14	4063	May 5, 2021
Best practices and general questions Programming	18	406	December 31, 2024
Style guide? Syntax & Programs	2	1139	May 6, 2021

General question about 'clean' code

Related topics