KDE review for KWeatherCore

[Dominik Haumann]

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 at protonmail.com>
 * Copyright 2020 Devin Lin <espidev at 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 at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20210102/b6d16bee/attachment.htm>