Refactor proposal

[mcarbonne]

Following my first PR (#19), I continued my digging and came with a proposal to refactor "state.cpp/h" files. Note: I started this PR from my previous one but if needed, I can rebase it on master. Only commit d5715791d3011233319ef2a1416462af63164497 is part of my proposal.

While browsing the source code, it was not straight forward to find which part of code was responsible for what.

My proposal is the following: rather than having everything public, encapsulate sections in structures, stored as private members + create a function (factor() const) giving read-only access + create the required functions when altering variables is required. Before:

class State {
public:
  // #### FACTORY ####
  String serial_number = "";  // len = 8
  String random_id = "";      // len = 6
  String model_name = "";     // 1 < len < 20
  String model_id = "";       // len = 2
  String hw_version = "";     // len = 3
  String unique_id = "";      // len = 20

After:

private:
  struct Factory {
    String serial_number = "";  // len = 8
    String random_id = "";      // len = 6
    String model_name = "";     // 1 < len < 20
    String model_id = "";       // len = 2
    String hw_version = "";     // len = 3
    String unique_id = "";      // len = 20
  } m_factory;

public:
  const Factory& factory() const { return m_factory; }
  void set_serial_number(const String& str) { m_factory.serial_number = str; }
  void set_random_id(const String& str) { m_factory.random_id = str; }
  void set_model_name(const String& str) { m_factory.model_name = str; }
  void set_model_id(const String& str) { m_factory.model_id = str; }
  void set_hw_version(const String& str) { m_factory.hw_version = str; }
  void set_unique_id(const String& str) { m_factory.unique_id = str; }

Now it is obvious to find write access versus read-only access.

Also, I think I found (and fixed) a bug, brought to light by this refactor : In factory.h

      } else if (cmd == "OK") {
        if (device_state.serial_number.length() > 0 &&
            device_state.random_id.length() > 0 &&
            device_state.model_name.length() > 0 &&
            device_state.model_id.length() > 0 &&
            device_state.hw_version.length() > 0) {
            device_state.unique_id = String("HBTNS-") + device_state.serial_number + "-" +
                               device_state.random_id;

With my proposal, it became

        if (device_state.factory().serial_number.length() > 0 &&
            device_state.factory().random_id.length() > 0 &&
            device_state.factory().model_name.length() > 0 &&
            device_state.factory().model_id.length() > 0 &&
            device_state.factory().hw_version.length() > 0) {
            device_state.factory().unique_id = String("HBTNS-") + device_state.factory().serial_number + "-" +
                               device_state.factory().random_id;

But thanks to constness (of factory method) it will refuse to compile. I'm pretty sure that = should have been == in the comparison with the unique id.

If you agree with my changes, I'll do the same for all the other sections found in state.h

[nplan]

I agree with your refactor proposal, it makes sense.

The "bug" is not actually a bug. The unique_id should be assigned at that point.

[mcarbonne]

Indeed, it wasn't a bug. I was fooled by the misleading indentation, fixed !

[mcarbonne]

@nplan I tried to keep only the original aim of my pull request: refactor of State. But I also:

• moved some functions declaration from headers to source files (factory.h)
• simplified get_btn_label and set_btn_label
• removed utils.h/cpp (there was only one function, I moved it directly in State::set_btn_label). I did a "breaking" change. Empty label is not replaced anymore by a "-" but I can rollback to the original behavior if needed.

After all those changes, it's not yet perfect and have some suggestions to improve it:

• create an MQTT class, with a few public methods and remove all mqtt-related stuffs from state. I would do something like that :

  
  #ifndef HOMEBUTTONS_MQTT_H
  #define HOMEBUTTONS_MQTT_H

include "state.h"

include "PubSubClient.h"

class Network; class WiFiClient;

class MQTT { public: MQTT(...); void publish_sensors(double temperature, double humidity); void send_autodiscovery(); //.... void update(); private: PubSubClient m_mqtt_client; struct Topics { String t_common = ""; // [...] } m_topics; };

endif // HOMEBUTTONS_MQTT_H

- try to avoid "public" variables without clear owner and create other classes to isolate independent functions.
- try to avoid static singleton (ie. `extern Toto toto`). I would rather do something like that:
```c++
class MyApplication
{
private:
    MQTT m_mqtt;
    AnotherModule m_anotherModule;
    //....
};

and in your main.cpp:

int main()
{
    MyApplication app;
    app.run();
}

When I have time, I'll submit other PR in this direction, If you agree.

[nplan]

There are now three different ways in which the variables in State are set:

1. Setter function

   set_device_name(const String& device_name)

2. Setter function for multiple variables

   set_ap_ssid_and_password(const String& ssid, const String& pwd)

3. Access by reference to struct

   device_state.sensors().charging = false;

Could you elaborate a bit what's the reason for this? To me it would make more sense to have a single way only for all the variables in State.

[mcarbonne]

I used setter whenever it was possible (without changing too much code). Sometimes, multiple variables are always set together (examples: set_mqtt_parameters or set_ap_ssid_and_password)

For some parts of the code (persisted, flags, sensors, topics...), it would have required too many changes. As I proposed for an MQTT class, splitting code in multiple modules (classes) with limited responsibility will improve code quality.