KDE review for KWeatherCore

[Dominik Haumann]

Hi Han,

let's have this discussion public on kde-core-devel and kde-framworks,
so other can contribute.

On Sat, Jan 2, 2021 at 1:29 PM hanyoung <hanyoung at protonmail.com> wrote:
>
> Hi Dominik,
>
> Thank you for your time. This is my first time writing a library, I tried my best by looking
> at existing KDE frameworks source code and articles on KDE wiki. Obvious KWeatherCore
> is far from polished. However, there are still something I want to explain.


There is always a first time, so welcome :-)

>
> > - #pragma once: It's used in all header files.
> I've once encountered name conflict with one KDE frameworks and that took me half a hour
> to debug. I want to #pragma once to avoid this kind of issue to appear, since most modern
> compiler supports this feature.


Yes, I am aware that essentially all compilers support it. I was just
mentioning the status quo.

>
> > 1. The first sentence in doxygen is automatically he brief version.
> It's on QHC documentation, so I just copied it. I guess it's fine since it doesn't do any harm.


It does harm in the sense that it adds visual noise. That is why I
suggested to remove it.
Other example: Many developers (outside of KDE) use void parameters
such as in void foobar(void);
This (void) is not needed either, it just adds noise. It's the exact
same thing here with doxygen.
You don't need it. So why add it?

>
> > 2. Obviously, GeoTimezone is not a complete documentation :-)
> Can't argue with that :P
> Actually, some problems with this class are all partially related to this.
>
> > 5. Copyright:
> Didn't know about that, my apologies. Will definitely change that.
>
> > 6. File naming.
> Didn't notice that. My bad.
>
> > 7. Constructors.
> It has another constructor takes zero arguments. And this class provides all setters and getters.
> The reason this "one for all" constructor exists is for internal convenience use. And I thought
> export it does no harm.

Let me try to explain this a bit more in depth.

When back in the old days Trolltech released Qt4, they broke a lot of API.
One reason for breaking API was the focus on good and readable API.
In fact, Mathias Ettrich even held a talk to the KDE audience in Ludwigsburg,
Germany in 2004 why API design matters a lot. The link I mentioned previously
(https://doc.qt.io/archives/qq/qq13-apis.html) essentially contains
all the points
back from this talk. Qt tries to stick to these rules, and KDE does as well.

Therefore I'd like to emphasize this again: Constructors that take
e.g. 8 arguments
and most of them are of type double will lead to unreadable code.
Theses constructors
also will not work, once you extend the class with  function and
member. Then, you have
one missing member that is not included in the constructor. So this
approach is not open
to extensions. Or it short: that is bad API design :-)

Let me phase it differently: Can we find an API that is even better?
You have a default constructor - fair enough. But I believe it can be better,
that is why I suggested the version that takes only a QDate, since you
cannot misuse it,
and the API is rather explicit. And the most important property of a
forecast is the date,
since otherwise the information is useless.

> > 8. Wrong order of API documentation:
> My bad.
>
> > 9. Pass non primitive data types (pod) by const reference:
> I use std::move, so pass by value means one copy and one move for lvalue, one move for rvalue.
> Const reference costs one copy for everything. Pass by value saves me one rvalue reference overload function.
> Consider that I have so many setters, it's justifiable.

I saw and understood this, yes. But this approach still has its
drawbacks. Let me explain.
When you use std::move internally, then you don't have any performance
penalty for now,
since the version with const ref would also do a copy later.

But let's say you need to internally change the implementation such
that you don't
need a copy anymore. Instead you just read this parameter. Then, you
still have the
copy in the public API and you cannot change it anymore due to binary
compatibility.

Or in other words: Currently you force API design decisions on the
public API that
rely on some internal implementation details. That is always a bad idea.

I hope what I wrote is understandable :-)

> > 10. Mixup of documentation + unexport macro.
> I can definitely improve that.
>
> > I assume the weatherDescription is a possibly user visible string?
> Thank you, I just noticed that default value for weatherIcon and weatherDescription are reversed. My bad.
>
> Thanks again for taking time to look through my terrible code base.

It's not terrible, it just needs several rounds of reviews to reach
the quality to be
included in kde-frameworks.

> Happy new year!

Thanks, same to you!

Best regards
Dominik