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