Domotics control system

This is a project about a domotics control system based on arduino. Nothing great; it's only for testing myself and making stuff.

Before I started coding this, I searched on the net for some useful tips to use in my code, but I still remain a novice in programming.

The project is about 1600 lines long. I'm obligated to load it on an external site here.

DomoS.h

```
#ifndef DomoS_H

#define DomoS_H
#include

class DomoS
{
private:

static const byte STRINGMAXLEN = 64; //Maximum length for the command string
static const byte SUBSTRINGMAXLEN = 16; //Maximum length for a single command
static const byte MAXADDRESSPIN = 8; //Maximum number of adressing pin
static const byte MAXNAMELEN = 10; //Maximum length for a peripheral name
static const byte FILEVER = 0; //The version of the file type

/*
The DomoS setting file is made of two parts
1) The header of the file which contains all of the configuration parameters
of the DomoS module and the version of the file type
2) The body of the file which contains all the settings for the different
peripherals [for a explanation go to the declaration of the type]
These are stored in sequential order

With version 0 the different arduino EEPROM can contain up to:
ATmega168 and ATmega8 [512byte]: 45 peripherals
ATmega328 [1024byte]: 91 peripherals
ATmega1280 and ATmega2560 [4096byte]: 371 peripherals
*/
struct DomoSFileHeader //size 13byte
{
//The order here is also the order in the EEPROM
//RESPECT THIS ORDER
//The "EEPROM #" on the right of the variable name is the standard position in the EEPROM

//Version of the file type, in case of change through developement
byte fileVer; //EEPROM 2
//Copy of the configuration parameters
byte numPeripheral; //EEPROM 3
byte numAddressPin; //EEPROM 4
byte addressPin[MAXADDRESSPIN]; //EEPROM 5-12
byte outputPin; //EEPROM 13
byte writeToEeprom; //EEPROM 14
};

struct DomoSF

I am not familiar with Arduino, but as a general C++ developer, I've noticed the following issues (looking only at the code in the question, not the one behind links):

-
No const-correctness.

The biggest problem in the code, one which would cause me to return it back to the developer if I were reviewing this at work.

None of your member functions are classified as const, meaning they will not be callable on a const object of type DomoS, or such an object accessed through a const-qualified path (such as a const DomoS &).

Even more importantly, your functions which take a pointer or reference parameter do not use const on the destination type either, which again means const objects or, even worse, temporaries cannot be used as arguments for those parameters. This way, you also deprive yourself of a great opportunity for self-documenting code, creating space for confusion of the user (or future maintainer) of the class. Example:

byte GetCommand(char* command); //Gets the number of a command

Based on its signature, this function:

• returns a byte. Given the comment, I expect this to be a number of the command

• can write something into the string pointed to by command. As a user, I am confused. Will it do so? What if the buffer I pass in is not large enough for what the function will write? How do I determine what "large enough" is? Why can't I use a string literal as the argument (string literals are of type const char[], and thus passing one here would cast away constness)?

• can modify the DomoS object on which it's invoked. This is definitely not something I would expect from a function whose name starts with Get.

Instead, I would expect the function's signature to be:

byte GetCommand(const char* command) const; //Returns the number of a command

The same applies to many other functions in the class, of course. This was just an example.

Writing const-correct code also prevents some programmer errors, such as accidentally modifying an input-only parameter.

Last but not least, it is a real pain to integrate a non-const-correct module into otherwise const-correct code. You have to use const_casts everywhere, which is 1) tedious to write, 2) a great source of bugs and 3) a maintenance nightmare. I understand this is less of an issue here since almost all of the class's functions are private, but it's a general principle worth mentioning anyway. From the external interface at least, IsOn() should definitely be const.

A quick summary of "const-correctness":

• A member function which does not modify the observable state of the object it's invoked on should be marked as const, so that it can be called on const-qualified objects.

• A function taking a parameter by reference and not modifying the parameter should take the parameter by const &. This is particularly important because it allows temporary objects to be bound to the reference (temporaries can bind to const-references, but not to non-const references).

• A function taking a parameter by pointer and not modifying the object pointed to should declare the parameter as pointer to const.

Imagine this code:

void prettyPrint(const std::string &s) {
  std::cout << s.size() << ' ' << s << '\n';
}

std::string x = "Hello";
std::string y = " World!";

prettyPrint(x + y);

If there was no const in the signature of print(), this wouldn't compile, as x + y returns a temporary. If the purely observing member function size() of std::string wasn't marked as const, we couldn't call s.size() inside prettyPrint() because s is a const-reference.

-
Taking structures by value.

Some of your functions take parameters of type DomoSFileBody by value. This can be OK if the function does any internal modification on the arguments passed in: copying at call site can be more efficient than copying internally, as discussed in this great article.

However, if the function does not modify its argument, you can save a significant amount of copying by passing these arguments as const DomoSFileBody & instead.

-
Hand-crafted enumeration for the error code.

The error constants would be perfectly represented as an enumeration. This has a distinct advantage of creating a full-fledged type, so that overload resolution can be used and code is more self-documenting. Function signature byte GetError(); doesn't really tell me much, but ErrorCode GetError(); clearly says what values I can expect.

In C++11 (and many compilers support this even before that), you can explicitly specify the underlying type for an enumeration, so it could be a byte even so. The definition would then look like this:

```
//Errors constant
enum ErrorCode {
OK,
COMMANDSTRINGTOOLONG,
SUBCOMMANDSTRINGTOOLONG,
COMMANDNOTRECOGNIZED,
NOCOMMANDPARAMETERS,
SUBCOMMANDNOTRECOGNIZED,
NAMENOTDEFINED,
NAMETOOLONG,
ASNOTDEFINED,
BINARYNUMBERTOOLONG,
EEPROMISFULL,
PERIPHERALNOTFOUND,
STRANG

StackExchange Code Review Q#26811, answer score: 2