Refactor Firebase class, improve api, change style

[ed7coyne]

I think we should refactor the Firebase class, while it is young and small seems the best time to do it.

I am personally against to the current paradigm of "do an action" then "explicitly check for error()". While this is exposed in professional apis (win32 comes to mind) it is usually not well regarded as those interfaces age. It is error prone as it is very easy to not explicitly check for the error than use the result as if it succeeded. It also has thread safety implications.

I propose that instead we return a FirebaseResult class that contains the status code and an optional result body and exposes helpers like is_error().

I also think we should take this time to standardize style guidelines for this class. Personally I propose using the Google C Style guide (https://google.github.io/styleguide/cppguide.html) as much as they apply to wirinig in its c-likeness. I think there is value in uniformity of the code as it develops, I am open to other styles as well but I think we should try to standardize on one if we can.

I will put together a pull request reflecting this change if no one has any objections.

[proppy]

I am personally against to the current paradigm of "do an action" then "explicitly check for error()".

The rational behind this was to make it easy to get the response content from the method call, without having to exposing to much types to the user. I noticed that Arduino library tend to be simpler than other C++ codebase and not push OO in the face of the user. I'd love to hear from you if you share similar observations.

I go a little bit further that road in #43, where we only return the data when the user explicitly ask for it: fbase.push().data() or fbase.push().json(), that's could be useful in the common case where a sketch just want to push data but don't care about buffering and parsing the result.

While this is exposed in professional apis (win32 comes to mind)

http://man7.org/linux/man-pages/man3/errno.3.html also come to mind.

It is error prone as it is very easy to not explicitly check for the error than use the result as if it succeeded.

My goal was actually the opposite: to make it really easy to check for error whatever the context: just test Firebase.error(). I but I agree the implementation might be a little bit too naive.

It also has thread safety implications.

Do you think that's a concern for arduino devices? So far I only encountered single core MCUs.

I was not too concerned by maintaining the last error (or the last result in #43) as a mutable state in the Firebase object, since the limited amount of example sketches I encountered usually do 1 request at a time (and the Firebase object only has 1 HttpClient instance). The idea was that if you need to compose multiple request concurrently (which I think would be rare) you'd use multiple Firebase object.

I propose that instead we return a FirebaseResult class that contains the status code and an optional result body and exposes helpers like is_error().

I will post comments on the codereview, I'd like use to converge on a way that make error checking more obvious while keeping the API simple.

I also think we should take this time to standardize style guidelines for this class. Personally I propose using the Google C Style guide (https://google.github.io/styleguide/cppguide.html) as much as they apply to wirinig in its c-likeness. I think there is value in uniformity of the code as it develops, I am open to other styles as well but I think we should try to standardize on one if we can.

+1 for this and enforcing it with travis (see #39), something else we need to take into account is having an API surface close to the Arduino style as well (i.e: user of the code shouldn't have to deal with inheritance, usually no exception handling and libraries tend to expose native type instead of introducing new types). Maybe this deserve a separate pull request, but it's also interesting to discuss the API change in the context of readability.

[proppy]

A few more elements from an offline discussion with @ed7coyne and @mimming.

We could have one result type by API method w/ specific results member and common methods for error() handling.

• FirebaseGetResult with a JsonObject result.
• FirebasePushResult with a String result for the name
• FirebaseStreamResult with aread()method that yieldFirebaseEvent`
• etc ..

The first implementation could consume the result payload so that the main http client could be reused right away. Later a lazy flag could be exposed to have more concurrent behavior with ephemeral http clients. The stream implementation could use it's own http client once the backend url redirection is fully negociated.

Some of this work could happen in #43 and #42

[proppy]

The stream implementation could use it's own http client once the backend url redirection is fully negociated.

Like @mimming pointed out that would also allow bi-directional I/O with push() and stream() with only 1 Firebase object.

[proppy]

Fixed with #42

[tabvn]

Problem still exit is stream and push or set together

Error screenshot: http://cloud.tabvn.com/b2ldqi.png

#include <ESP8266WiFi.h>
#include <FirebaseArduino.h>

// Set these to run example.
#define FIREBASE_HOST "abc.firebaseio.com"
#define FIREBASE_AUTH "abc"
#define WIFI_SSID "name"
#define WIFI_PASSWORD "pass"
#define UID "abc"
#define url "/users/abc/devices"

FirebaseArduino FirebaseStream;

void setup() {
Serial.begin(9600);

// connect to wifi.
WiFi.begin(WIFI_SSID, WIFI_PASSWORD);
Serial.print("Connecting to wifi");
while (WiFi.status() != WL_CONNECTED) {
Serial.print(".");
delay(500);
}
Serial.println();
Serial.print("connected: ");
Serial.println(WiFi.localIP());

FirebaseStream.begin(FIREBASE_HOST, FIREBASE_AUTH);
Firebase.begin(FIREBASE_HOST, FIREBASE_AUTH);
FirebaseStream.stream(url);

}

int n;
void loop() {

Serial.println("loop");
Firebase.setInt("data", n++);

if (FirebaseStream.failed()) {
Serial.println("streaming error");
Serial.println(FirebaseStream.error());
}

if (FirebaseStream.available()) {
FirebaseObject event = FirebaseStream.readEvent();
String eventType = event.getString("type");
eventType.toLowerCase();

 if (eventType == "put") {
 //  message = state ? "Device now is On": "Devive now is off";
   JsonVariant json = event.getJsonVariant();

   String path = json["path"].asString();
   if (path != "/"){
    // first loading seem it load all the devices. So we dont need it 
     Serial.println("");
     json.printTo(Serial);

     // if data: TRUE this mean device status change in this case we only take care when device status change.


     Serial.println(event.getBool("data"));
     String message = event.getBool("data") ? "Device is on now" : "Devive is off now";

     Serial.println("Sending message to server.");

     Firebase.setString("message", "hello world");
     delay(1000);

    // If data: Null this mean device just deleted see screenshot: http://cloud.tabvn.com/rax3i8.png 

    // Else String or object  (this could be adding new item or update device title .... 

   }


 }

}

}