request weather data and send them to the wirst #29

StefWe · 2020-06-30T19:46:01Z

Added function to receive coordinates from the qml part, request the weather data from owm and send the results to the wrist.

…weather data from owm and send the results to the wrist.

FlorentRevest · 2020-07-03T21:40:05Z

asteroidsyncserviced/dbusinterface.h


 private slots:
    void onTimeServiceReady();
    void onNotifyServiceReady();
    void onScreenshotServiceReady();
    void onWeatherServiceReady();
    void onDisconnected();
+    void onReplyFinished();


Given that this class does a lot, it would be helpful to name this into something more specific, maybe onOwmReplyFinished?

or maybe you could use a lambda and inline the slot in owmRequest. I think that's what the cool kids would do these days

FlorentRevest · 2020-07-03T21:45:03Z

asteroidsyncserviced/openweathermapparser.h

+#include <QJsonArray>
+#include <QDateTime>
+
+class OpenWeatherMapParser : public QObject


Since the methods are all stateless you don't really need a class here (an object of type OpenWeatherMapParser doesn't carry any ownership/lifetime semantic)
Or you could do the parsing once in the constructor (because all methods take the same arguments and do the same loop over and over again) and then store the results of the parsing in private members that you could return from getWeatherId(), getTempMin() etc...

FlorentRevest · 2020-07-03T21:46:01Z

asteroidsyncserviced/openweathermapparser.cpp

+    QList<short> id;
+    QDateTime timestamp;
+
+    for(int i = 0; i < 5; i++) {


I recommend using list.size() like in the other methods or you risk accessing undefined values.

I think range-based loops should work on QJsonArray https://en.wikipedia.org/wiki/C%2B%2B11#Range-based_for_loop

FlorentRevest · 2020-07-03T21:49:21Z

asteroidsyncserviced/openweathermapparser.cpp

+    for(int i = 0; i < 5; i++) {
+        QJsonObject dayInfo = list.takeAt(i).toObject();
+        QJsonArray dayWeather = dayInfo.value("weather").toArray();
+        QJsonObject firstEntry = dayWeather.takeAt(0).toObject();


QJsonArray has a first() method that is nicer than takeAt(0)

FlorentRevest · 2020-07-03T21:52:05Z

asteroidsyncserviced/openweathermapparser.cpp

+        dayInfo = list.at(i).toObject();
+        timestamp.setTime_t(dayInfo.value("dt").toInt());
+        dayMain = dayInfo.value("main").toObject();
+        if(actDate == timestamp.toString("dd.MM.yyyy") && dayMain.value("temp_min").toDouble() < tmp) {


a string comparison is always quite an expensive operation (and also a bit risky as a simple space or something is always a risk), it's usually the wrong way to approach a problem. What if you'd compare QDates instead https://doc.qt.io/qt-5/qdatetime.html#date ?

…weather data from owm and send the results to the wrist.

StefWe · 2020-08-02T22:39:31Z

i made a second try :)

FlorentRevest · 2020-08-06T21:40:56Z

asteroidsyncserviced/dbusinterface.cpp

+
+    m_wmp->prepareData(rootObj);
+    m_weatherService->setCity(m_wmp->getCity());
+    qDebug() << "WeatherID" << m_wmp->getWeatherId();


Does it help to keep those qDebug in the app ?

FlorentRevest · 2020-08-06T21:42:14Z

asteroidsyncserviced/dbusinterface.cpp

+        return;
+    }
+
+    m_wmp->prepareData(rootObj);


I don't understand why you have one m_wmp that you keep updating. Could you just create a new wmp here and give rootObj to its constructor?

You also wouldn't have to clear fields like you do in your prepareData method

FlorentRevest · 2020-08-06T21:48:23Z

asteroidsyncserviced/openweathermapparser.cpp

+void OpenWeatherMapParser::setMaxTemp(const QJsonObject obj, const QDate date)
+{
+    if(m_tempMax.isEmpty()) {
+        m_tempMax << obj.value("temp_max").toDouble();


You often extract this "obj.value("temp_max").toDouble()" could you make it a variable that you only extract once to make the code easier to read ?

FlorentRevest · 2020-08-06T22:07:59Z

asteroidsyncserviced/openweathermapparser.cpp

+        QJsonObject dayInfo = element.toObject();
+        timestamp.setTime_t(dayInfo.value("dt").toInt());
+
+        setWeatherId(dayInfo.value("weather").toArray(), timestamp.time());


I see that every method has a different way of determining "what day it is" but ultimately it should be the same day for every method.

setWeatherId relies on the assumption that all previous days had a weatherId somewhere between 11 and 13. What if the measurements contain a value at 11:00:00 and 13:00:00 ? You would suddenly inject 2 values in your array and then every following day would be shifted.

setMaxTemp/setMinTemp both do the same calculation keeping "m_dateTempMin" and "m_dateTempMax" which are the actually the same. Here too, you're making many assumptions, for example about the order in which events are received, if two events are swapped (maybe by the parser or for some other random reason?) your logic becomes broken. You use this to access "m_tempMax.size() - 1" but this creates special cases for when the array is empty etc... By reducing your number of assumptions you can make the code much more simple and generic.

I propose that you extract here in this loop once for each element the "day index" which is going to be the index at which any value will be inserted (whether that is a weather id or a max/min temperature in their respective arrays). You could calculate this offset quite easily with https://doc.qt.io/qt-5/qdate.html#daysTo by comparing the date to the first date in the json document. If you provide this day index to all your methods, you should find that your code becomes greatly simplified. Especially your "setM**Temp" functions should have less special cases for empty arrays/new dates etc... They could just insert their value at the right index. This also provides you stronger guarantees that the "weatherId"/"maxTemp"/"minTemp" will be aligned.

FlorentRevest · 2020-08-06T22:09:24Z

asteroidsyncserviced/openweathermapparser.cpp

+    }
+
+    if(m_dateTempMax == date && obj.value("temp_max").toDouble() > m_tempMax[m_tempMax.size() - 1]) {
+        m_tempMax.replace(m_tempMax.size() - 1, obj.value("temp_max").toDouble());


Something along the line of m_tempMax[dayIndex] = qMax(m_tempMax[dayIndex], obj.value("temp_max").toDouble()) is an option to make it easier to understand that you're keeping the max value.

FlorentRevest · 2020-08-06T22:10:45Z

asteroidsyncserviced/openweathermapparser.cpp

+    if(m_dateTempMax == date && obj.value("temp_max").toDouble() > m_tempMax[m_tempMax.size() - 1]) {
+        m_tempMax.replace(m_tempMax.size() - 1, obj.value("temp_max").toDouble());
+    } else if(m_dateTempMax != date) {
+        if(m_tempMax.size() >= 5) return;


Why 5? If this is a requirement of the protocol, I'd recommend that you enforce this in the "weatherservice" rather than in the parser. Ideally, the parser shouldn't have to care about limitations of the protocol that will transfer its data.

For example, it is easy for setWeatherId to ignore this restriction.

Added function to receive coordinates from the qml part, request the …

61e6a23

…weather data from owm and send the results to the wrist.

FlorentRevest reviewed Jul 3, 2020

View reviewed changes

Added function to receive coordinates from the qml part, request the …

9d09563

…weather data from owm and send the results to the wrist.

FlorentRevest requested changes Aug 6, 2020

View reviewed changes

beroset mentioned this pull request Apr 7, 2022

Expand weather api #51

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

request weather data and send them to the wirst #29

request weather data and send them to the wirst #29

StefWe commented Jun 30, 2020

FlorentRevest Jul 3, 2020

FlorentRevest Jul 3, 2020

FlorentRevest Jul 3, 2020

FlorentRevest Jul 3, 2020

FlorentRevest Jul 3, 2020

FlorentRevest Jul 3, 2020

FlorentRevest Jul 3, 2020

StefWe commented Aug 2, 2020

FlorentRevest Aug 6, 2020

FlorentRevest Aug 6, 2020

FlorentRevest Aug 6, 2020

FlorentRevest Aug 6, 2020

FlorentRevest Aug 6, 2020

FlorentRevest Aug 6, 2020

FlorentRevest Aug 6, 2020

FlorentRevest Aug 6, 2020

request weather data and send them to the wirst #29

Are you sure you want to change the base?

request weather data and send them to the wirst #29

Conversation

StefWe commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefWe commented Aug 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment