Wrong use of Object Oriented Programming or memory issue

Hi,

As you will see in my code, I’m a programming noob so please bare with me :confused:
The goal of my project is to fetch weather data and display it on an LCD screen. Here I have removed code that is not needed to show my problem.
I am aware that my project can be solved in many different ways but I’m doing it this way to learn OOP in Arduino. I’ve created the following classes:

class WeatherData - Purpose: To hold weather data to reuse data when needed. A struct could probably do it but wants to learn Arduino OOP.
class OpenWeatherMap - Purpose: To connect to Open Weather Map API and fetch weather data. This does not has be a class but again I’m using this to learn how to use Arduino OOP.

As you will see, The OpenWeatherMap class is creating an instance of the WeatherData class, fetching data from the API and storing it in the WeatherData instance, I’m in my sketch I’m creating an instance of the OpenWeatherMap class getting the data and storing it two different instances of the WeatherData class.
If I only create one instance everything works just fine but if I create two objects the data in the second object is not correct, seems like the same memory is used and the data is overwritten in the second object.
Project can be found here: https://github.com/datamann/OopBestPractices

/OopBestPractices.ino/

#include <SPI.h>
#define DEBUG
#include "openweathermap.h"
#include "wifi.h"

String apikey = "***";
OpenWeatherMap owp;
OpenWeatherMap owp1;
WeatherData Ilagan;
WeatherData Drammen;

void setup() {
Serial.begin(9600);
/*Code removed: maximum allowed length (9000 char).*/
}
#ifdef DEBUG
Serial.print(F("\n\nWiFi connected: "));
Serial.println(WiFi.localIP().toString());
#endif
Ilagan = owp.fetchWeatherData("1711146", apikey);
Drammen = owp1.fetchWeatherData("3159016", apikey);
/*Code removed: maximum allowed length (9000 char).*/
}

void loop() {
}

/WeatherData.h/

#ifndef WEATHERDATA_h
#define WEATHERDATA_h
class WeatherData {
public:
WeatherData();
void setLocation(const char* location);
const char* getLocation();
void setTemperature(float temperature);
float getTemperature();
void setFeelslike(float feelsLike);
float getFeelslike();
void setTemp_min(float temp_min);
float getTemp_min();
void setTemp_max(float temp_max);
float getTemp_max();
void setPressure(int pressure);
int getPressure();
void setHumidity(int humidity);
int getHumidity();
void setWeather(const char* weather);
const char* getWeather();
void setDescription(const char* description);
const char* getDescription();
void setWindspeed(float windSpeed);
float getWindspeed();
void setWinddirection(int windDirection);
int getWinddirection();
void setWindcompassdirection(const char* windCompassDirection);
const char* getWindcompassdirection();
void setWeatherid(int weatherID);
int getWeatherid();
void setTimes(const char* timeS);
const char* getTimes();
private:
const char* _location;
float _temperature;
float _feelsLike;
float _temp_min;
float _temp_max;
int _pressure;
int _humidity;
const char* _weather;
const char* _description;
float _windSpeed;
int _windDirection;
const char* _windCompassDirection;
int _weatherID;
const char* _timeS;
};
#endif

/WeatherData.cpp/

#include "WeatherData.h"

WeatherData::WeatherData() {
_location = "--";
_temperature = 0.00;
_feelsLike = 0.00;
_temp_min = 0.00;
_temp_max = 0.00;
_pressure = 0;
_humidity = 0;
_windSpeed = 0.00;
_windDirection = 0;
_weatherID = 0;
};
void WeatherData::setLocation(const char* location) {
_location = location;
};
const char* WeatherData::getLocation() {
return _location;
};
void WeatherData::setTemperature(float temperature) {
_temperature = temperature;
};
float WeatherData::getTemperature() {
return _temperature;
};
void WeatherData::setFeelslike(float feelsLike) {
_feelsLike = feelsLike;
};
float WeatherData::getFeelslike() {
return _feelsLike;
};
void WeatherData::setTemp_min(float temp_min) {
_temp_min = temp_min;
};
float WeatherData::getTemp_min() {
return _temp_min;
};
void WeatherData::setTemp_max(float temp_max) {
_temp_max = temp_max;
};
float WeatherData::getTemp_max() {
return _temp_max;
};
void WeatherData::setPressure(int pressure) {
_pressure = pressure;
};
int WeatherData::getPressure() {
return _pressure;
};
void WeatherData::setHumidity(int humidity) {
_humidity = humidity;
};
int WeatherData::getHumidity() {
return _humidity;
};
void WeatherData::setWeather(const char* weather) {
_weather = weather;
};
const char* WeatherData::getWeather() {
return _weather;
};
void WeatherData::setDescription(const char* description) {
_description = description;
};
const char* WeatherData::getDescription() {
return _description;
};
void WeatherData::setWindspeed(float windSpeed) {
_windSpeed = windSpeed;
};
float WeatherData::getWindspeed() {
return _windSpeed;
};
void WeatherData::setWindcompassdirection(const char* windCompassDirection) {
_windCompassDirection = windCompassDirection;
};
const char* WeatherData::getWindcompassdirection() {
return _windCompassDirection;
};
void WeatherData::setWinddirection(int windDirection) {
_windDirection = windDirection;
};
int WeatherData::getWinddirection() {
return _windDirection;
};
void WeatherData::setWeatherid(int weatherID) {
_weatherID = weatherID;
};
int WeatherData::getWeatherid() {
return _weatherID;
};
void WeatherData::setTimes(const char* timeS) {
_timeS = timeS;
};
const char* WeatherData::getTimes() {
return _timeS;
};

/openweathermap.h/

#ifndef OPENWEATHERMAP_h
#define OPENWEATHERMAP_h
#include "WeatherData.h"
class OpenWeatherMap {
public:
OpenWeatherMap();
WeatherData fetchWeatherData(String cityid, String apikey);  
private:
const char* getWindDirection(int deg);
bool inRange(float val, float minimum, float maximum);
char* _servername = "api.openweathermap.org";
WeatherData *wd;
};
#endif

/openweathermap.cpp/

#include "openweathermap.h"
#include "WeatherData.h"
#include <ArduinoJson.h>
#include <WiFi.h>
OpenWeatherMap::OpenWeatherMap() {
wd = new WeatherData();
};
WeatherData OpenWeatherMap::fetchWeatherData(String cityid, String apikey)
{
String result = "";
WiFiClient client;
const int httpPort = 80;
if (!client.connect(_servername, httpPort)) {
#ifdef DEBUG
Serial.print(F("Client not connected! "));
Serial.println(client);
#endif
}
String url = "/data/2.5/forecast?id="+ cityid +"&units=metric&cnt=1&APPID="+ apikey;
client.print(String("GET ") + url + " HTTP/1.1\r\n" +
"Host: " + _servername + "\r\n" +
"Connection: close\r\n\r\n");
unsigned long timeout = millis();
while (client.available() == 0) {
if (millis() - timeout > 10000) {
client.stop();
#ifdef DEBUG
Serial.print(F("Client timed out! "));
Serial.println(client);
#endif
}
}
while(client.available()) {
result = client.readStringUntil('\r');
}
result.replace('[', ' ');
result.replace(']', ' ');
char jsonArray [result.length()+1];
result.toCharArray(jsonArray,sizeof(jsonArray));
jsonArray[result.length() + 1] = '\0';
StaticJsonDocument<1024> doc;
DeserializationError error = deserializeJson(doc, jsonArray);
if (error) {
#ifdef DEBUG
Serial.print(F("deserializeJson() failed with code "));
Serial.println(error.c_str());
#endif
}
const char* location = doc["city"]["name"];
wd->setLocation(location);
float temperature = doc["list"]["main"]["temp"];
wd->setTemperature(temperature);
float feelsLike = doc["list"]["main"]["feels_like"];
wd->setFeelslike(feelsLike);
float temp_min = doc["list"]["main"]["temp_min"];
wd->setTemp_min(temp_min);
float temp_max = doc["list"]["main"]["temp_max"];
wd->setTemp_max(temp_max);
int pressure = doc["list"]["main"]["pressure"];
wd->setPressure(pressure);
int humidity = doc["list"]["main"]["humidity"];
wd->setHumidity(humidity);
const char* weather = doc["list"]["weather"]["main"];
wd->setWeather(weather);
const char* description = doc["list"]["weather"]["description"];
wd->setDescription(description);
float windSpeed = doc["list"]["wind"]["speed"];
wd->setWindspeed(windSpeed);
int windDirection = doc["list"]["wind"]["deg"];
wd->setWinddirection(windDirection);
wd->setWindcompassdirection(getWindDirection(wd->getWinddirection()));
int weatherID = doc["list"]["weather"]["id"];
wd->setWeatherid(weatherID);
return *wd;
};
const char* OpenWeatherMap::getWindDirection(int deg)
{
float wind_deg = float(deg);
const char* direction = "---";
if(OpenWeatherMap::inRange(wind_deg, 348.750, 360.999)){direction = "N";return direction;}
/*Code removed: maximum allowed length (9000 char).*/
};
bool OpenWeatherMap::inRange(float val, float minimum, float maximum)
{
return ((minimum <= val) && (val <= maximum));
};

like many C++ programs, it's not obvious what your code is doing (especially without any comments)

at a talk on object oriented programming at bell labs (1985) the speaker said within the first few sentences that you can write OOPs in almost any language, including assembler, but not languages like Fortran or Basic. And you're not writing OOPs just because your using C++!

Tom Cargill's book goes thru examples of C++ programs that are poorly written, explains the issues and fixes them. (similar to Kernighan's Elements of Programming Style)

in the 3rd edition of his book, Bjarne Stroustrup said learned a lot about using C++ while updating the book. In an interview, he said he felt it takes 10 years to become proficient in C++.

Ken Thompson, an early user of C++ and co-developer of UNIX, didn't think much of C++. He and Rob Pike developed the "Go" programming language, a more pure OOP language at Google

i've always struggled with seeing the beauty of C++. I think I never saw a good example (which I believe exists). But other's i've worked with were tortured by layers of abstraction hiding how relatively simple code works.

In his 3rd edition, Stroustrup warned users to only use the features of the language that are necessary and not use them frivolously. This is similar to the Bell Labs writing course recommendation to use a simple a term as possible.

many C++ classes can be implemented as a struct with functions.

Your string handling is flawed: you have several strings in your weather data struct e.g. _weather that have no memory allocated, they're just pointers.

When you set them, they take the value of a pointer they were passed. Those point to something in your doc variable which is a temporary variable and when it goes out of scope that memory can be reused. In consequence, your data is likely to be corrupted.

You need to allocate memory in your class for your strings i.e. an array of char and strcpy (better strncpy) data into them when setting. Similarly, on getting, the caller should provide a place to put the data and you can copy it there. It's not good practice to return a pointer to a private variable.

@gcjr I'm sorry for the late reply. You're absolutely right I have to write comments for my code. Thank you for the book suggestions, I have taken a look at some and will read them. So it takes a long time to be good programmer, well then it will take ages for me since I'm only a hobby programmer and sometimes it can take weeks in between coding sessions. It often feels like I've need to re-learn every time...

@wildbill I'm sorry for the late reply. Thank you so much for help! I was able to solve my issue (not right away though) because of your comment :slight_smile: