<div dir="ltr"><div>Hi Han,</div><div><br></div><div>just having a quick glance at the code, and I feel like there is a lot of polishing</div><div> missing. I'll give some examples, but this is by no means a complete review,</div><div>and I'll not comment on technical aspects at all, since this is not the domain</div><div>of my expertise.<br></div><div><br></div><div>- #pragma once: It's used in all header files. I am not sure whether we have</div><div>any policy here, but indeed it's almost not used at all in KDE Frameworks.</div><div>Maybe someone else can clarify.</div><div><br></div><div>- API documentation largely incomplete: For instance, looking at geotimezone.h,</div><div>we see </div><div><br></div><div> /**<br> * @brief GeoTimezone<br> * @param lat latitude<br> * @param lon longitude<br> * @param parent<br> */<br> GeoTimezone(double lat, double lon, QObject *parent = nullptr);<br></div><div><br></div><div>1. The first sentence in doxygen is automatically he brief version. You can omit @brief</div><div>everywhere. Its just noise.</div><div><br></div><div>2. Obviously, GeoTimezone is not a complete documentation :-)</div><div><br></div><div>3. Try to avoid abbreviations. That is: Name the parameters latitude and longitude</div><div>instead of lat and lon. Explaining a parameter with the name itself is also no good</div><div>practice. Better would be: @param latitude The latitude used in geographical timezone. or so...</div><div><br>Q_SIGNALS:<br> /**<br> * @brief finished<br> * @param timezone<br> */<br> void finished(const QString &timezone);</div><div><br></div><div>4. What is finished? It seems to me that this API is not intuitive :-)</div><div>And again: the API documentation is essentially missing.<br></div><div><br></div><div> /**<br> * @brief networkError encounted network error<br> */<br> void networkError();<br></div><div><br></div><div>Better would be networkErrorOccured() or so, to stress the fact that this is a signal.</div><div><br></div><div>5. Copyright:</div><div> * Copyright 2020 Han Young <<a href="mailto:hanyoung@protonmail.com">hanyoung@protonmail.com</a>><br> * Copyright 2020 Devin Lin <<a href="mailto:espidev@gmail.com">espidev@gmail.com</a>><br> * SPDX-License-Identifier: LGPL-2.0-or-later</div><div><br></div><div>I think nowadays we encourange using SPDX-FileCopyrightText to list the author.</div><div>This can appear multiple times, from what I understand.</div><div><a href="https://community.kde.org/Policies/Licensing_Policy#SPDX_Statements">https://community.kde.org/Policies/Licensing_Policy#SPDX_Statements</a></div><div><br></div><div><br></div><div>6. File naming.</div><div>We have dailyforecast.h, but the class is called DailyWeatherForecast.</div><div>It's good practice to name the header files exactly like the class names for public API.</div><div><br></div><div>7. Constructors. Let's have a look at this:</div><div> DailyWeatherForecast(double maxTemp,<br> double minTemp,<br> double precipitation,<br> double uvIndex,<br> double humidity,<br> double pressure,<br> QString weatherIcon,<br> QString weatherDescription,<br> QDate date);</div><div><br></div><div>Using this looks like this: DailyWheaterFoecast forecast(0, 10, 35, 80, 1023, "sun", "Sunny Weather", ...);</div><div><br></div><div>Please read "The Convenience Trap" here: <a href="https://doc.qt.io/archives/qq/qq13-apis.html">https://doc.qt.io/archives/qq/qq13-apis.html</a></div><div>In other words: Try to avoid constructors that take many many arguments to define the object in one go.</div><div>What is much more readable is to have 3-4 lines more code, but at least it can be understood:</div><div>DailyWheterhForecast forecast(QDate(2020, 12, 31));</div><div>forecast->setIcon("sun");</div><div>forecast->setDescription("Nice beach weather");</div><div>forecast->setTemperatureRange(2, 27);<br></div><div>...</div><div><br></div><div><br></div><div>8. Wrong order of API documentation:<br></div><div> /**<br> * Return a QJsonObject that can be converted back with<br> * DailyWeatherForecast::fromJson<br> */<br> ~DailyWeatherForecast();<br> QJsonObject toJson();<br></div><div><br></div><div>Here, the virtual destructor will get the API documentation of toJson(). Obviously not what is wanted.<br></div><div><br></div><div><br></div><div>9. Pass non primitive data types (pod) by const reference:</div><div> void setWeatherIcon(QString icon);<br> void setWeatherDescription(QString description);<br> void setDate(QDate date);<br></div><div><br></div><div>const QString &</div><div>const QDate &<br></div><div><br></div><div>10. Mixup of documentation + unexport macro.<br></div><div> /**<br> * @brief return maximum temperature<br> * @return maximum temperature, this value is initialized to the smallest<br> * value double can hold<br> */<br> KWEATHERCORE_NO_EXPORT void setJsDate(const QDateTime &date);<br> double maxTemp() const;<br></div><div><br></div><div>Here, setJsDate() will get the API documentation of maxTemp(). That is not what you want.</div><div>Typically, having to explicitly hide a symbol (_NO_EXPORT) is showing design issues.</div><div>I'd recommend to move this function to the pimpl class, and if you need internally access to</div><div>this function, then move your pimpl class to e.g. dailyweatherforecast_p.h.</div><div><br></div><div>This is just by looking at the first two header files.</div><div><br></div><div>Looking at WeatherForecast.cpp:</div><div><br></div><div> double maxTemp = std::numeric_limits<double>::min();<br> double minTemp = std::numeric_limits<double>::max();</div><div><br></div><div>Initializing the temperature to 0, 0, sounds a bit more sane to me, but that is disputable I guess.</div><div><br></div><div> QDate date = QDate();</div><div>The default constructor will be called automatically, better would be simply:</div><div> QDate date;</div><div><br></div><div>bool isNull(); The semantics of isNull() is a bit weird. Typically, the naming of bool isValid() is</div><div>used in Qt and therefore better.</div><div><br></div><div>I assume the weatherDescription is a possibly user visible string? Currently, this is initialized</div><div>to QString weatherDescription = QStringLiteral("weather-none-available"); So possibly</div><div>the string "weather-none-available" will show up in the UI. I suggest to add a translatable</div><div>string set to "No weather data set" or similar.</div><div><br></div><div><br></div><div>I'll stop at this point. I think someone else with more know-how in this area needs to do a</div><div>thorough review of this code. In general, I believe it's worth to iterate the code until this</div><div>framework is good enough, since there was quite some interest in the mail thread so far.</div><div><br></div><div>Best regards & have a nice day :-)</div><div>Dominik<br></div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Dec 31, 2020 at 8:38 AM hanyoung <<a href="mailto:hanyoung@protonmail.com">hanyoung@protonmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I've made some changes to KWeatherCore according to the feedback. Namely:<br>
* More Private Classes<br>
* Removed inline functions<br>
* Lowered coordinates resolution<br>
* Listed API services in use on README<br>
* Switched to LGPL<br>
<br>
Regards,<br>
Han<br>
</blockquote></div>