AmboVent-1690-108 / AmboVent

AmboVent 1690.108
The Unlicense
206 stars 69 forks source link

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

Open ElectricRCAircraftGuy opened 4 years ago

ElectricRCAircraftGuy commented 4 years ago

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 commented 4 years ago

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

nimrod46 commented 4 years ago

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

ElectricRCAircraftGuy commented 4 years ago

Fixed description.

ElectricRCAircraftGuy commented 4 years ago

@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. :)

nimrod46 commented 4 years ago

@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