AmboVent icon indicating copy to clipboard operation
AmboVent copied to clipboard

Code Formatting: Use enums where able; C-style namespace them properly

Open ElectricRCAircraftGuy opened this issue 4 years ago • 5 comments

This is a sub-issue of #11. Don't close #11 until all sub-issues are resolved.

Use enums wherever they make sense instead of constants or #defines. Also, namespace them by putting their categorical name before their specific name, separated by an underscore.

Instead of:

enum main_states : byte
{
  STBY_STATE,
  BREATH_STATE,
  MENU_STATE
};

Do:

Use 3 slashes /// to signify "doxygen" style comments above members (good practice even if you don't generate doxygen). See more examples in my file here: https://github.com/ElectricRCAircraftGuy/eRCaGuy_dotfiles/blob/master/git%20%26%20Linux%20cmds%2C%20help%2C%20tips%20%26%20tricks%20-%20Gabriel.txt

enum main_state : byte
{
  // Use explicit name, and explicitly set 1st element to 0
  /// some useful description
  MAIN_STATE_STANDBY = 0, 
  /// some useful description
  MAIN_STATE_BREATH,
  /// some useful description
  MAIN_STATE_MENU, 
  /// Not a state! Use this if you need the total number of enums in this type
  MAIN_STATE_COUNT, // trailing comma on last elment is just fine
};

ElectricRCAircraftGuy avatar Apr 13 '20 08:04 ElectricRCAircraftGuy

I will help with this. Just want to teach the principle too is all.

ElectricRCAircraftGuy avatar Apr 13 '20 08:04 ElectricRCAircraftGuy

Just so you know, enums are type definition, not a list, so the name should be main_state for instance

nimrod46 avatar Apr 13 '20 13:04 nimrod46

Fixed description.

ElectricRCAircraftGuy avatar Apr 13 '20 16:04 ElectricRCAircraftGuy

@nimrod46, I'm also accustomed to using typedefs when writing C, and many organizations require enum classes when using C++, but I think the C enum is just fine, but since Arduino is technically C++, it allows a simplified syntax without typedef.

In C I'd add _t to the end of the enum type. Ex:

typedef enum main_state_e
{
  // Use explicit name, and explicitly set 1st element to 0
  /// some useful description
  MAIN_STATE_STANDBY = 0, 
  /// some useful description
  MAIN_STATE_BREATH,
  /// some useful description
  MAIN_STATE_MENU, 
  /// Not a state! Use this if you need the total number of enums in this type
  MAIN_STATE_COUNT, // trailing comma on last elment is just fine
} main_state_t;

Then

main_state_t main_state = MAIN_STATE_STANDBY;

I think we should add the _t if too to specify it's a type, but since we don't need the typedef in C++, I'm fine without it too. @nimrod46 , let me know if you have a preference. Otherwise, I'll plan on going with the _t as well so we can do this:

main_state_t main_state = MAIN_STATE_STANDBY;

...instead of this:

main_state my_main_state = MAIN_STATE_STANDBY;

Notice the problem of having to figure out what to name the name_state variable now in the latter case, since the variable name can't be the same as the type name. Adding _t to the end of the type solves this silly little dilemma. :)

ElectricRCAircraftGuy avatar Apr 13 '20 17:04 ElectricRCAircraftGuy

@ElectricRCAircraftGuy I prefer without the _t the current main state should be current_main_state . I think its the best practice to name it like that anyway

nimrod46 avatar Apr 13 '20 18:04 nimrod46