D27166: Refactor converter runner

[Harald Sitter]

sitter added a comment.


  Looks so much better already!
  
  I'd really love a unit test for this though. plasma-workspace's servicesrunner should have an example one

INLINE COMMENTS

> converterrunner.cpp:19
>  
> +#define CONVERSION_CHAR QLatin1Char( '>' )
>  #include "converterrunner.h"

Please move this after the includes. If any header in the future decides to want to use the name as well we're in for an awkward debugging session.

> converterrunner.cpp:57
> +    const QString valueGroup = QStringLiteral("([0-9,./]+)");
> +    const QString unitGroup = QStringLiteral("([a-zA-Z/\"'^0-9$£¥€]+)");
> +    const QString separatorsGroup = QStringLiteral("(?: in | to | as | ?> ?)");

I am sure we can do better than hardcoding currency symbols. Perhaps iterate QLocales and get the  currencySymbol for each?
(needs checking that performance doesn't degrade)

> converterrunner.cpp:59
> +    const QString separatorsGroup = QStringLiteral("(?: in | to | as | ?> ?)");
> +    matchRegex = QRegularExpression(QStringLiteral("^%1 ?%2(?:%3%2)?$").arg(valueGroup, unitGroup, separatorsGroup));
>  }

I'd really prefer named capture groups. Popping data out of the captures by index is fairly terrible to read and also easier to mess up in the future.

> converterrunner.cpp:68
>  {
> -    const QString term = context.query();
> -    if (term.size() < 2) {
> +    const QString &term = context.query();
> +    if (term.size() < 2 || !context.isValid()) {

unnecessary ref.

> converterrunner.cpp:78
> +    const QString value = regexMatch.captured(1);
> +    const QString unit1 = regexMatch.captured(2);
> +    const QString unit2 = regexMatch.lastCapturedIndex() == 3 ? regexMatch.captured(3) : QString();

unit1 and unit2 should probably be unitString1 and unitString2 so the actual units can be unit1 and unit2. same for value I suppose.

> converterrunner.cpp:82
> +    KUnitConversion::UnitCategory category = converter.categoryForUnit(unit1);
> +    const KUnitConversion::Unit u1 = category.unit(unit1);
> +    const QList<KUnitConversion::Unit> &units = createResultUnits(unit2, category);

Please use more descriptive names than u1, such as unit1.
In fact, isn't unit1 "inputUnit" and unit2 "outputUnit"? If so I'd name them thusly.

> converterrunner.cpp:83
> +    const KUnitConversion::Unit u1 = category.unit(unit1);
> +    const QList<KUnitConversion::Unit> &units = createResultUnits(unit2, category);
> +    const auto numberDataPair = getValidatedNumberValue(value);

unnecessary ref.

> converterrunner.cpp:92
> +    QList<Plasma::QueryMatch> matches;
> +    for (const KUnitConversion::Unit &u: units) {
> +        const KUnitConversion::Value &v = category.convert(KUnitConversion::Value(numberValue, u1), u);

`s/u/unit`

> converterrunner.cpp:93
> +    for (const KUnitConversion::Unit &u: units) {
> +        const KUnitConversion::Value &v = category.convert(KUnitConversion::Value(numberValue, u1), u);
> +        if (!v.isValid() || u1 == u) {

`s/v/value`

> converterrunner.cpp:114
> +    QGuiApplication::clipboard()->setText(match.data().toString());
> +}
> +double ConverterRunner::stringToDouble(const QStringRef &value, bool *ok)

newline missing it seems

> converterrunner.cpp:124
> +
> +QPair<bool, double> ConverterRunner::getValidatedNumberValue(const QString &value)
> +{

in all return statements you can use list initialization

`return { ok, output };`

> converterrunner.h:50
> +
> +    double stringToDouble(const QStringRef &value, bool *ok);
> +    QPair<bool, double> getValidatedNumberValue(const QString &value);

stringToDouble and getValidatedNumberValue seem to have conflicting ideas of api design. either both should use the pair return or both should use the ok pointer.

> converterrunner.h:52
> +    QPair<bool, double> getValidatedNumberValue(const QString &value);
> +    QList<KUnitConversion::Unit> createResultUnits(const QString &unit2, const KUnitConversion::UnitCategory &category);
>  };

Please use multiple lines to declare multiple members, even when they have the same type. Same line declaration is unnecessarily increasing reading complexity.

REPOSITORY
  R114 Plasma Addons

REVISION DETAIL
  https://phabricator.kde.org/D27166

To: alex, broulik, ngraham, #plasma, sitter
Cc: sitter, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200210/8065f665/attachment-0001.html>