Merge lp:~jpakkane/unity-scope-click/classness into lp:unity-scope-click

[Jussi Pakkanen]

Made structs more C++-like.

Made a few structs more C++-like. This includes moving comparison operators to class methods. This is the commonly accepted best practice. Also made PackageDetails JSON parsing work via a constructor.

[lp:~jpakkane/unity-scope-click/classness]

134. By Jussi Pakkanen on 2014-02-11

Removed redundant thises.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:134
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/256/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/154
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/154

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scope-click-ci/256/rebuild

review: Approve (continuous-integration)

[Thomas Voß (thomas-voss)]

I would argue against "classifying" the struct as it really is a value type, according to Meyers: Effective C++ Item 23 "Prefer non-member non-friend functions to member functions", symmetric operators should be non-member, non-friend functions (see http://stackoverflow.com/questions/4421706/operator-overloading/4421729#4421729 for a good discussion).

With that, I disapprove of the changes.

review: Disapprove

[Jussi Pakkanen (jpakkane)]

"Classify" was perhaps a poorly chosen name but I could not think of anything better. That being said:

This MR does _not_ change the semantics of the target in any way, it remains a struct with the exact same format. The only API change is that in addition to having a constructor that takes all elements you also get a constructor that takes a file name pointing to a json file. This follows the principle that it is better to initialize everything in the constructor rather than creating empty objects and then calling some type of an initializer function.

As far as member operators go, the main point in that style recommendation is that it increases encapsulation. In this particular case that point is moot, since the comparison function accesses struct members directly. There is no encapsulation at all.

The main argument for implementing comparison operators as member functions is information centralisation. This allows you to look at just the struct definition and immediately know what operations it supports. A plain function comparison operator is easy to overlook and can thus cause problems when new person starts looking through the code and can even be accidentally lost when refactoring.

[Thomas Voß (thomas-voss)]

> "Classify" was perhaps a poorly chosen name but I could not think of anything
> better. That being said:
>
> This MR does _not_ change the semantics of the target in any way, it remains a
> struct with the exact same format. The only API change is that in addition to
> having a constructor that takes all elements you also get a constructor that
> takes a file name pointing to a json file. This follows the principle that it
> is better to initialize everything in the constructor rather than creating
> empty objects and then calling some type of an initializer function.
>

Sure, two stage init is bad, but why not have a free function from_json? With that, you wouldn't need the c'tor of epic size.

> As far as member operators go, the main point in that style recommendation is
> that it increases encapsulation. In this particular case that point is moot,
> since the comparison function accesses struct members directly. There is no
> encapsulation at all.
>
> The main argument for implementing comparison operators as member functions is
> information centralisation. This allows you to look at just the struct
> definition and immediately know what operations it supports. A plain function
> comparison operator is easy to overlook and can thus cause problems when new
> person starts looking through the code and can even be accidentally lost when
> refactoring.

Hmmm, the compiler & tests capture if someone forgets about an operator== during refactorings, not sure how having it defined as a member of the struct really helps.

Unmerged revisions

134. By Jussi Pakkanen on 2014-02-11

Removed redundant thises.

133. By Jussi Pakkanen on 2014-02-11

Made Package and PackageDetails more classlike.

132. By Jussi Pakkanen on 2014-02-11

Moved comparison operators to member functions as is the recommended way.

=== modified file 'scope/click/index.cpp'
--- scope/click/index.cpp	2014-02-06 21:32:20 +0000
+++ scope/click/index.cpp	2014-02-11 13:41:35 +0000
@@ -36,30 +36,30 @@
 namespace click
 {
 
-bool operator==(const Package& lhs, const Package& rhs) {
-    return lhs.name == rhs.name &&
-            lhs.title == rhs.title &&
-            lhs.price == rhs.price &&
-            lhs.icon_url == rhs.icon_url &&
-            lhs.url == rhs.url;
+bool Package::operator==(const Package& rhs) const {
+    return name == rhs.name &&
+           title == rhs.title &&
+           price == rhs.price &&
+           icon_url == rhs.icon_url &&
+           url == rhs.url;
 }
 
-bool operator==(const PackageDetails& lhs, const PackageDetails& rhs) {
-    return lhs.name == rhs.name &&
-            lhs.title == rhs.title &&
-            lhs.icon_url == rhs.icon_url &&
-            lhs.description == rhs.description &&
-            lhs.download_url == rhs.download_url &&
-            lhs.rating == rhs.rating &&
-            lhs.keywords == rhs.keywords &&
-            lhs.terms_of_service == rhs.terms_of_service &&
-            lhs.license == rhs.license &&
-            lhs.publisher == rhs.publisher &&
-            lhs.main_screenshot_url == rhs.main_screenshot_url &&
-            lhs.more_screenshots_urls == rhs.more_screenshots_urls &&
-            lhs.binary_filesize == rhs.binary_filesize &&
-            lhs.version == rhs.version &&
-            lhs.framework == rhs.framework;
+bool PackageDetails::operator==(const PackageDetails& rhs) const {
+    return name == rhs.name &&
+           title == rhs.title &&
+           icon_url == rhs.icon_url &&
+           description == rhs.description &&
+           download_url == rhs.download_url &&
+           rating == rhs.rating &&
+           keywords == rhs.keywords &&
+           terms_of_service == rhs.terms_of_service &&
+           license == rhs.license &&
+           publisher == rhs.publisher &&
+           main_screenshot_url == rhs.main_screenshot_url &&
+           more_screenshots_urls == rhs.more_screenshots_urls &&
+           binary_filesize == rhs.binary_filesize &&
+           version == rhs.version &&
+           framework == rhs.framework;
 }
 
 PackageList package_list_from_json(const std::string& json)
@@ -86,7 +86,41 @@
     return pl;
 }
 
-void PackageDetails::loadJson(const std::string &json)
+
+PackageDetails::PackageDetails(const std::string &name,
+        const std::string &title,
+        const std::string &icon_url,
+        const std::string &description,
+        const std::string &download_url,
+        const std::string &rating,
+        const std::string &keywords,
+        const std::string &terms_of_service,
+        const std::string &license,
+        const std::string &publisher,
+        const std::string &main_screenshot_url,
+        const std::string &more_screenshots_urls,
+        const std::string &binary_filesize,
+        const std::string &version,
+        const std::string &framework) :
+                  name(name),
+                  title(title),
+                  icon_url(icon_url),
+                  description(description),
+                  download_url(download_url),
+                  rating(rating),
+                  keywords(keywords),
+                  terms_of_service(terms_of_service),
+                  license(license),
+                  publisher(publisher),
+                  main_screenshot_url(main_screenshot_url),
+                  more_screenshots_urls(more_screenshots_urls),
+                  binary_filesize(binary_filesize),
+                  version(version),
+                  framework(framework)
+{
+}
+
+PackageDetails::PackageDetails(const std::string &json)
 {
     std::istringstream is(json);
     boost::property_tree::ptree node;
@@ -130,9 +164,7 @@
     QSharedPointer<click::web::Response> response = service->call(click::DETAILS_PATH+package_name);
     QObject::connect(response.data(), &click::web::Response::finished, [=](QString reply){
         Q_UNUSED(response); // so it's still in scope
-        click::PackageDetails d;
-        d.loadJson(reply.toUtf8().constData());
-        callback(d);
+        callback(click::PackageDetails(reply.toUtf8().constData()));
     });
 }
 

=== modified file 'scope/click/index.h'
--- scope/click/index.h	2014-02-06 21:22:48 +0000
+++ scope/click/index.h	2014-02-11 13:41:35 +0000
@@ -54,6 +54,7 @@
     std::string icon_url;
     std::string url;
     void matches (std::string query, std::function<bool> callback);
+    bool operator==(const Package& rhs) const;
 };
 
 typedef std::list<Package> PackageList;
@@ -77,7 +78,25 @@
     std::string binary_filesize;
     std::string version;
     std::string framework;
-    void loadJson(const std::string &json);
+
+    PackageDetails(const std::string &name,
+    const std::string &title,
+    const std::string &icon_url,
+    const std::string &description,
+    const std::string &download_url,
+    const std::string &rating,
+    const std::string &keywords,
+    const std::string &terms_of_service,
+    const std::string &license,
+    const std::string &publisher,
+    const std::string &main_screenshot_url,
+    const std::string &more_screenshots_urls,
+    const std::string &binary_filesize,
+    const std::string &version,
+    const std::string &framework);
+    explicit PackageDetails(const std::string &json);
+
+    bool operator==(const PackageDetails& rhs) const;
 };
 
 class Index
@@ -91,9 +110,6 @@
     ~Index();
 };
 
-bool operator==(const Package& lhs, const Package& rhs);
-bool operator==(const PackageDetails& lhs, const PackageDetails& rhs);
-
 } // namespace click
 
 #endif // CLICKINDEX_H