Differential D6513 Diff 37047 src/attica/atticaprovider.cpp

Changeset View

Standalone View

View Options

src/attica/atticaprovider.cpp

Context not available.
18#include "atticaprovider_p.h" 18​#include "atticaprovider_p.h"
19 19​
20#include "question.h" 20​#include "question.h"
21​#include "tagsfilterchecker.h"
21 22​
22#include <QCollator> 23​#include <QCollator>
23#include <knewstuffcore_debug.h> 24​#include <knewstuffcore_debug.h>
Context not available.
269 Content::List contents = listJob->itemList(); 270​ Content::List contents = listJob->itemList();
270 271​
271 EntryInternal::List entries; 272​ EntryInternal::List entries;
273​ TagsFilterChecker checker(mCurrentRequest.tagFilter);
274​ TagsFilterChecker downloadschecker(mCurrentRequest.downloadTagFilter);
272 Q_FOREACH (const Content &content, contents) { 275​ Q_FOREACH (const Content &content, contents) {
273 mCachedContent.insert(content.id(), content); 276​ bool filterAcceptsDownloads = content.downloads() == 0 ? true : false;
274 entries.append(entryFromAtticaContent(content)); 277​ foreach(const Attica::DownloadDescription & dli, content.downloadUrlDescriptions()) {
278​ filterAcceptsDownloads = downloadschecker.filterAccepts(dli.tags());
279​ if(filterAcceptsDownloads) {
280​ break;
cfeckUnsubmitted
Done

Space after foreach. No space after &.

cfeck: Space after `foreach`. No space after `&`.
ngrahamUnsubmitted
Done

Also isn't Qt foreach deprecated anyway? https://www.kdab.com/goodbye-q_foreach/

ngraham: Also isn't Qt `foreach` deprecated anyway? https://www.kdab.com/goodbye-q_foreach/
cfeckUnsubmitted
Done

Ah, yes. Q_FOREACH, or C++11 for (x : y) if you see it does not cause unneeded detaches.

cfeck: Ah, yes. `Q_FOREACH`, or C++11 `for (x : y)` if you see it does not cause unneeded detaches.
281​ }
282​ }
283​ if(filterAcceptsDownloads && checker.filterAccepts(content.tags())) {
ahiemstraUnsubmitted
Done

Wouldn't it be simpler to first check if the content should be considered at all, and then check if one of the downloads is valid? So something like:

if(!checker.filterAccepts(content.tags()) {
  continue
}

foreach download {
 if(downloadsChecker.filterAccepts(download.tags) {
   add download
   break
 }
}
ahiemstra: Wouldn't it be simpler to first check if the content should be considered at all, and then…
leinirAuthorUnsubmitted
Done

Hmm... It's not quite so simple as that (it's just just a straight foreach, there's also the preselection logic, which is what that ternary is for), but the fact that wasn't clear to you while reading does suggest it was too convoluted. I've remodelled it somewhat, and i think the new logic is perhaps more easily readable as well, which is not a bad thing. Thanks :)

leinir: Hmm... It's not quite so simple as that (it's just just a straight foreach, there's also the…
284​ mCachedContent.insert(content.id(), content);
285​ entries.append(entryFromAtticaContent(content));
286​ }
287​ else {
288​ qCDebug(KNEWSTUFFCORE) << "Filter has excluded" << content.name() << "on entry filter" << mCurrentRequest.tagFilter << "and download filter" << mCurrentRequest.downloadTagFilter;
289​ }
cfeckUnsubmitted
Done

Coding style: Space after if (multiple occurences).

cfeck: Coding style: Space after `if` (multiple occurences).
275 } 290​ }
276 291​
cfeckUnsubmitted
Done

Frameworks coding style:

if (...) {
    ...
} else {
    ...
}
cfeck: Frameworks coding style: if (...) { ... } else { ... }
277 qCDebug(KNEWSTUFFCORE) << "loaded: " << mCurrentRequest.hashForRequest() << " count: " << entries.size(); 292​ qCDebug(KNEWSTUFFCORE) << "loaded: " << mCurrentRequest.hashForRequest() << " count: " << entries.size();
cfeckUnsubmitted
Done

Missing Space after if

cfeck: Missing Space after `if`
cfeckUnsubmitted
Done

Coding style:

if (...) {
    ...
} else {
    ...
}
cfeck: Coding style: if (...) { ... } else { ... }
Context not available.
481 entry.setSummary(content.description()); 496​ entry.setSummary(content.description());
482 entry.setShortSummary(content.summary()); 497​ entry.setShortSummary(content.summary());
483 entry.setChangelog(content.changelog()); 498​ entry.setChangelog(content.changelog());
499​ entry.setTags(content.tags());
484 500​
485 entry.clearDownloadLinkInformation(); 501​ entry.clearDownloadLinkInformation();
486 QList<Attica::DownloadDescription> descs = content.downloadUrlDescriptions(); 502​ QList<Attica::DownloadDescription> descs = content.downloadUrlDescriptions();
Context not available.
493 info.id = desc.id(); 509​ info.id = desc.id();
494 info.size = desc.size(); 510​ info.size = desc.size();
495 info.isDownloadtypeLink = desc.type() == Attica::DownloadDescription::LinkDownload; 511​ info.isDownloadtypeLink = desc.type() == Attica::DownloadDescription::LinkDownload;
512​ info.tags = desc.tags();
496 entry.appendDownloadLinkInformation(info); 513​ entry.appendDownloadLinkInformation(info);
497 } 514​ }
498 515​
Context not available.