Functions that return key/value data pairs should not return them as single strings

[kcranky]

Is your feature request related to a problem? Please describe. Much of the data requested from the Toggl API exists in multiples of key/value pairs. In particular is the workspaces list and projects list. Currently, the library returns this data as a string and leaves it for the user to process. This isn't necessarily terrible, but formatting the data types to indicate the way the data should be worked with is valuable.

Describe the solution you'd like Ideally, we'd like to return a std::arr type from these hypothetical getWorkspaces() and getProjects(). However, as the Arduino library doesn't support the std::arr, we need to implement a custom solution.

Structs are our friend in this scenario. A struct of the following form could capture both workspaces and projects:

struct KVPair {
    String name;
    int id;
};

There are a few problems with this though. 1) We need to return an array of structs, and in this form we can't get the number of returned workspaces/projects 2) An HTTP error can't intuitively gleamed from an empty array

There are 2 approaches we could use which will address both raised issues 1) Get a pointer to an integer in the function call. The function updates this value with the amount of workspace/project entries in successful, or returns a NULL in the array of structs and sets this value to be the HTTP code upon failure. 2) Wrap the array of workspace/project structs in a struct which contains a return integer value, something along the lines of

struct KVPairReturn {
    int returnCode;
    KVPair* dataPairs;
}

and return it as follows (in the case of a successful request)

KVPair* projects = new KVPair[arr.size()];
KVReturnPair  returnData {
    arr.size(),
    projects
};

In case of a unsuccessful request, the projects array can be set to NULL and the returnCode value set to the HTTP return code.

In both cases the user can check if the data is valid by first checking if the returned array is NULL, and then if so check the HTTP code, if not use the updated integer value to know how many workspaces or projects they are working with. The data also represents the tie between the id and the name

Describe alternatives you've considered Memory on an embedded system is scary. Giving strings definitely is easier to ensure things are safe, but I don't feel it's a good approach in this case as the data returned is non-intuitive and doesn't necessarily cater for more general use cases I've tested the option KVPairReturn option, and it works rather nicely. It is also one less pointer than option 1, though we'd still need to make it clear to users to free up the KVPair array memory once they're done.

Additional context Writing data structures to indicate the way the data is presented (and hence expected) is good practice. Returning strings is confusing to library users.

[JoeyStrandnes]

Thanks for your comprehensive explanation!

I am aware of the memory inefficiencies around Strings (something my code is quite reliant on).

I originally used strings to keep things simple and safe. The code grew and I never got around to improve that aspect...

I do like your KVPairReturn option and see no inherent downside over using Strings. You could create a PR so I could look through it since you have already written the code.

The Arduino platform is mostly aimed towards beginners. So I tried to keep things simple/dumbed down so that anybody could just use and modify this library without much hassle. I do realize that I might just have made things to inefficient and non intuitive, in my quest to keep things simple (I'm also not a programmer...).

[kcranky]

I have made a proof of concept, see the "RemoveStrings" branch I've created at https://github.com/kcranky/Arduino-Toggl-API/tree/RemoveStrings. Sorry it's taken a while.

What I did 1) Created two new structs, KVPair and KVReturn. KVPair constains two members, id (int) and name (String). KVReturn contains 3 members - HTTPCode (int), pairCount (int) and KVPairs (KVPair *).

2) Removed getProject and getWorkspace, and created getProjects and getWorkspaces. The title is now more accurate and will ensure that anyone who upgrades the library will move to the new methods.

3) Added a new private method, void getKVPairs(String const URL, KVReturn &data); This is the method that is called by getProjects and getWorkspaces. Both of those features perform the same behavior, so I combined them to prevent code duplication.

Testing Here's a snippet that works with the ESP8266. (See "Problems" below for ESP32 details). This code will fetch and print all workspaces, and then fetch and print all projects for the first found workspace.

KVReturn workspaces = toggl.getWorkSpaces();
  if (workspaces.HTTPCode >= 200 && workspaces.HTTPCode <= 226){
    Serial.println("There are " + String(workspaces.pairCount) + " workspaces");

    for (int i = 0; i < workspaces.pairCount; i++){
      Serial.println(String(workspaces.KVPairs[i].id) + ": " + workspaces.KVPairs[i].name);
    }
  }
  else {
    Serial.println("Failed to get workspaces with HTTPCode " + String(workspaces.HTTPCode));
  }

  int ourWorkspace = workspaces.KVPairs[0].id;

  delete[] workspaces.KVPairs;

  KVReturn projects = toggl.getProjects(ourWorkspace);
  Serial.println("Workspace = " + String(ourWorkspace));
  if (projects.HTTPCode >= 200 && projects.HTTPCode <= 226){
    Serial.println("There are " + String(projects.pairCount) + " projects");

    for (int i = 0; i < projects.pairCount; i++){
      Serial.println(String(projects.KVPairs[i].id) + ": " + projects.KVPairs[i].name);
    }
  }
  else {
    Serial.println("Failed to get projects with HTTPCode " + String(projects.HTTPCode));
  }

  delete[] projects.KVPairs;

Problems I'm unable to test the ESP32 code, but I've raised that as a separate issue, which I've raised here: https://github.com/JoeyStrandnes/Arduino-Toggl-API/issues/9

[kcranky]

With Issue https://github.com/JoeyStrandnes/Arduino-Toggl-API/issues/9 resolved, I think this is working out well. Let me know what you think when you get a gap. I added details to the readme, and added an example to show how the methods work, too.

[JoeyStrandnes]

Great, feel free to make a pull request for the changes if you feel that it is "release ready". It makes it easier to test the new features!

Side note:

I also remembered why I returned the data as a "pretty string", I just intended it to be used to quickly determine the workspaces for the user through the terminal. Your implementation is definably better since it could be used to print to terminal but also used inside a menu system in an embedded display project.