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
