[coding] [map] Refactored url's ForEachParam.

The bool value in the old version served two purposes at once: it was used both to control the execution flow and to indicate the parsing result. As such, the return value of true meant "the needed key was found and the callback returned true when executed with the corresponding value" and the return value of false meant "either the needed key was not found or the callback returned false even though the key was present". This is too loaded semantically and is a false optimization (since the parsed url params need to be stored in a vector of pairs by the time of this call, early exit won't help much).
2020-02-12 18:57:09 +03:00 · 2020-02-12 18:57:09 +03:00 · 3fd79eef0c
commit 3fd79eef0c
parent b6842d1e1e
6 changed files with 125 additions and 132 deletions
--- a/coding/coding_tests/url_tests.cpp
+++ b/coding/coding_tests/url_tests.cpp
@ -35,13 +35,12 @@ public:
  }

 private:
-  bool AddTestValue(url::Param const & param)
+  void AddTestValue(url::Param const & param)
  {
    TEST(!m_keyValuePairs.empty(), ("Failed for url = ", m_url, "Passed KV = ", param));
    TEST_EQUAL(m_keyValuePairs.front().first, param.m_name, ());
    TEST_EQUAL(m_keyValuePairs.front().second, param.m_value, ());
    m_keyValuePairs.pop();
-    return true;
  }

  string m_url;
--- a/coding/url.cpp
+++ b/coding/url.cpp
@ -224,19 +224,10 @@ bool Url::Parse(std::string const & url)
  return true;
 }

-bool Url::ForEachParam(Callback const & callback) const
+void Url::ForEachParam(Callback const & callback) const
 {
-  // todo(@m) Looks strange but old code worked this way.
-  if (m_params.empty())
-    return false;
-
  for (auto const & param : m_params)
-  {
-    if (!callback(param))
-      return false;
-  }
-
-  return true;
+    callback(param);
 }

 string Make(string const & baseUrl, Params const & params)
--- a/coding/url.hpp
+++ b/coding/url.hpp
@ -26,14 +26,14 @@ using Params = std::vector<Param>;
 class Url
 {
 public:
-  using Callback = std::function<bool(Param const & param)>;
+  using Callback = std::function<void(Param const & param)>;

  explicit Url(std::string const & url);

  std::string const & GetScheme() const { return m_scheme; }
  std::string const & GetPath() const { return m_path; }
  bool IsValid() const { return !m_scheme.empty(); }
-  bool ForEachParam(Callback const & callback) const;
+  void ForEachParam(Callback const & callback) const;

 private:
  bool Parse(std::string const & url);
--- a/map/map_tests/mwm_url_tests.cpp
+++ b/map/map_tests/mwm_url_tests.cpp
@ -39,9 +39,11 @@ class ApiTest
 {
 public:
  explicit ApiTest(string const & urlString)
-    : m_fm(kFrameworkParams)
+    : m_framework(kFrameworkParams)
  {
-    m_m = &m_fm.GetBookmarkManager();
+    df::VisualParams::Init(1.0, 1024);
+
+    m_m = &m_framework.GetBookmarkManager();
    m_api.SetBookmarkManager(m_m);

    auto const res = m_api.SetUrlAndParse(urlString);
@ -103,7 +105,7 @@ private:
  }

 private:
-  Framework m_fm;
+  Framework m_framework;
  ParsedMapApi m_api;
  m2::RectD m_viewportRect;
  BookmarkManager * m_m;
@ -118,7 +120,7 @@ bool IsValid(Framework & fm, string const & urlString)

  return api.IsValid();
 }
-}
+}  // namespace

 UNIT_TEST(MapApiSmoke)
 {
--- a/map/mwm_url.cpp
+++ b/map/mwm_url.cpp
@ -11,8 +11,6 @@
 #include "platform/marketing_service.hpp"
 #include "platform/settings.hpp"

-#include "coding/url.hpp"
-
 #include "base/logging.hpp"
 #include "base/string_utils.hpp"

@ -127,12 +125,12 @@ enum class ApiURLType

 std::array<std::string, 3> const kAvailableSchemes = {{"mapswithme", "mwm", "mapsme"}};

-ApiURLType URLType(url::Url const & uri)
+ApiURLType URLType(url::Url const & url)
 {
-  if (std::find(kAvailableSchemes.begin(), kAvailableSchemes.end(), uri.GetScheme()) == kAvailableSchemes.end())
+  if (std::find(kAvailableSchemes.begin(), kAvailableSchemes.end(), url.GetScheme()) == kAvailableSchemes.end())
    return ApiURLType::Incorrect;

-  auto const path = uri.GetPath();
+  auto const path = url.GetPath();
  if (path == "map")
    return ApiURLType::Map;
  if (path == "route")
@ -151,8 +149,11 @@ ApiURLType URLType(url::Url const & uri)
  return ApiURLType::Incorrect;
 }

-bool ParseLatLon(string const & key, string const & value, double & lat, double & lon)
+bool ParseLatLon(url::Param const & param, double & lat, double & lon)
 {
+  string const & key = param.m_name;
+  string const & value = param.m_value;
+
  size_t const firstComma = value.find(',');
  if (firstComma == string::npos)
  {
@ -196,20 +197,21 @@ ParsedMapApi::ParsingResult ParsedMapApi::SetUrlAndParse(string const & url)
  return res;
 }

-ParsedMapApi::ParsingResult ParsedMapApi::Parse(url::Url const & uri)
+ParsedMapApi::ParsingResult ParsedMapApi::Parse(url::Url const & url)
 {
-  switch (URLType(uri))
+  switch (URLType(url))
  {
    case ApiURLType::Incorrect:
      return ParsingResult::Incorrect;
    case ApiURLType::Map:
    {
      vector<ApiPoint> points;
-      auto const result = uri.ForEachParam([&points, this](url::Param const & param) {
-        return AddKeyValue(param.m_name, param.m_value, points);
+      bool correctOrder = true;
+      url.ForEachParam([&points, &correctOrder, this](url::Param const & param) {
+        ParseMapParam(param, points, correctOrder);
      });

-      if (!result || points.empty())
+      if (points.empty() || !correctOrder)
        return ParsingResult::Incorrect;

      ASSERT(m_bmManager != nullptr, ());
@ -230,11 +232,11 @@ ParsedMapApi::ParsingResult ParsedMapApi::Parse(url::Url const & uri)
      m_routePoints.clear();
      using namespace route;
      vector<string> pattern{kSourceLatLon, kSourceName, kDestLatLon, kDestName, kRouteType};
-      auto const result = uri.ForEachParam([&pattern, this](url::Param const & param) {
-        return RouteKeyValue(param.m_name, param.m_value, pattern);
+      url.ForEachParam([&pattern, this](url::Param const & param) {
+        ParseRouteParam(param, pattern);
      });

-      if (!result || pattern.size() != 0)
+      if (pattern.size() != 0)
        return ParsingResult::Incorrect;

      if (m_routePoints.size() != 2)
@ -248,23 +250,23 @@ ParsedMapApi::ParsingResult ParsedMapApi::Parse(url::Url const & uri)
    case ApiURLType::Search:
    {
      SearchRequest request;
-      auto const result = uri.ForEachParam([&request, this](url::Param const & param) {
-        return SearchKeyValue(param.m_name, param.m_value, request);
+      url.ForEachParam([&request, this](url::Param const & param) {
+        ParseSearchParam(param, request);
      });
-      if (!result)
+      if (request.m_query.empty())
        return ParsingResult::Incorrect;
      
      m_request = request;
-      return request.m_query.empty() ? ParsingResult::Incorrect : ParsingResult::Search;
+      return ParsingResult::Search;
    }
    case ApiURLType::Lead:
    {
      lead::CampaignDescription description;
-      auto result = uri.ForEachParam([&description, this](url::Param const & param) {
-        return LeadKeyValue(param.m_name, param.m_value, description);
+      url.ForEachParam([&description, this](url::Param const & param) {
+        ParseLeadParam(param, description);
      });

-      if (!result || !description.IsValid())
+      if (!description.IsValid())
        return ParsingResult::Incorrect;

      description.Write();
@ -273,11 +275,11 @@ ParsedMapApi::ParsingResult ParsedMapApi::Parse(url::Url const & uri)
    case ApiURLType::Catalogue:
    {
      Catalog item;
-      auto const result = uri.ForEachParam([&item, this](url::Param const & param) {
-        return CatalogKeyValue(param.m_name, param.m_value, item);
+      url.ForEachParam([&item, this](url::Param const & param) {
+        ParseCatalogParam(param, item);
      });

-      if (!result || item.m_id.empty())
+      if (item.m_id.empty())
        return ParsingResult::Incorrect;

      m_catalog = item;
@ -286,11 +288,11 @@ ParsedMapApi::ParsingResult ParsedMapApi::Parse(url::Url const & uri)
    case ApiURLType::CataloguePath:
    {
      CatalogPath item;
-      auto const result = uri.ForEachParam([&item, this](url::Param const & param) {
-        return CatalogPathKeyValue(param.m_name, param.m_value, item);
+      url.ForEachParam([&item, this](url::Param const & param) {
+        ParseCatalogPathParam(param, item);
      });

-      if (!result || item.m_url.empty())
+      if (item.m_url.empty())
        return ParsingResult::Incorrect;

      m_catalogPath = item;
@ -299,11 +301,11 @@ ParsedMapApi::ParsingResult ParsedMapApi::Parse(url::Url const & uri)
    case ApiURLType::Subscription:
    {
     Subscription item;
-     auto const result = uri.ForEachParam([&item, this](url::Param const & param) {
-       return SubscriptionKeyValue(param.m_name, param.m_value, item);
+     url.ForEachParam([&item, this](url::Param const & param) {
+       ParseSubscriptionParam(param, item);
     });

-     if (!result || item.m_groups.empty())
+     if (item.m_groups.empty())
       return ParsingResult::Incorrect;

     m_subscription = item;
@ -313,60 +315,19 @@ ParsedMapApi::ParsingResult ParsedMapApi::Parse(url::Url const & uri)
  UNREACHABLE();
 }

-bool ParsedMapApi::RouteKeyValue(string const & key, string const & value, vector<string> & pattern)
-{
-  using namespace route;
-
-  if (pattern.empty())
-    return true;
-
-  if (key != pattern.front())
-    return false;
-
-  if (key == kSourceLatLon || key == kDestLatLon)
-  {
-    double lat = 0.0;
-    double lon = 0.0;
-    if (!ParseLatLon(key, value, lat, lon))
-      return false;
-
-    RoutePoint p;
-    p.m_org = mercator::FromLatLon(lat, lon);
-    m_routePoints.push_back(p);
-  }
-  else if (key == kSourceName || key == kDestName)
-  {
-    m_routePoints.back().m_name = value;
-  }
-  else if (key == kRouteType)
-  {
-    string const lowerValue = strings::MakeLowerCase(value);
-    if (lowerValue == kRouteTypePedestrian || lowerValue == kRouteTypeVehicle ||
-        lowerValue == kRouteTypeBicycle || lowerValue == kRouteTypeTransit)
-    {
-      m_routingType = lowerValue;
-    }
-    else
-    {
-      LOG(LWARNING, ("Incorrect routing type:", value));
-      return false;
-    }
-  }
-
-  pattern.erase(pattern.begin());
-  return true;
-}
-
-bool ParsedMapApi::AddKeyValue(string const & key, string const & value, vector<ApiPoint> & points)
+void ParsedMapApi::ParseMapParam(url::Param const & param, vector<ApiPoint> & points, bool & correctOrder)
 {
  using namespace map;

+  string const & key = param.m_name;
+  string const & value = param.m_value;
+
  if (key == kLatLon)
  {
    double lat = 0.0;
    double lon = 0.0;
-    if (!ParseLatLon(key, value, lat, lon))
-      return false;
+    if (!ParseLatLon(param, lat, lon))
+      return;

    ApiPoint pt{.m_lat = lat, .m_lon = lon};
    points.push_back(pt);
@ -385,7 +346,8 @@ bool ParsedMapApi::AddKeyValue(string const & key, string const & value, vector<
    else
    {
      LOG(LWARNING, ("Map API: Point name with no point. 'll' should come first!"));
-      return false;
+      correctOrder = false;
+      return;
    }
  }
  else if (key == kId)
@ -397,7 +359,8 @@ bool ParsedMapApi::AddKeyValue(string const & key, string const & value, vector<
    else
    {
      LOG(LWARNING, ("Map API: Point url with no point. 'll' should come first!"));
-      return false;
+      correctOrder = false;
+      return;
    }
  }
  else if (key == kStyle)
@ -409,7 +372,7 @@ bool ParsedMapApi::AddKeyValue(string const & key, string const & value, vector<
    else
    {
      LOG(LWARNING, ("Map API: Point style with no point. 'll' should come first!"));
-      return false;
+      return;
    }
  }
  else if (key == kBackUrl)
@ -433,25 +396,67 @@ bool ParsedMapApi::AddKeyValue(string const & key, string const & value, vector<
  {
    m_goBackOnBalloonClick = true;
  }
-  return true;
 }

-bool ParsedMapApi::SearchKeyValue(string const & key, string const & value, SearchRequest & request) const
+void ParsedMapApi::ParseRouteParam(url::Param const & param, vector<string> & pattern)
+{
+  using namespace route;
+
+  string const & key = param.m_name;
+  string const & value = param.m_value;
+
+  if (pattern.empty() || key != pattern.front())
+    return;
+
+  if (key == kSourceLatLon || key == kDestLatLon)
+  {
+    double lat = 0.0;
+    double lon = 0.0;
+    if (!ParseLatLon(param, lat, lon))
+      return;
+
+    RoutePoint p;
+    p.m_org = mercator::FromLatLon(lat, lon);
+    m_routePoints.push_back(p);
+  }
+  else if (key == kSourceName || key == kDestName)
+  {
+    m_routePoints.back().m_name = value;
+  }
+  else if (key == kRouteType)
+  {
+    string const lowerValue = strings::MakeLowerCase(value);
+    if (lowerValue == kRouteTypePedestrian || lowerValue == kRouteTypeVehicle ||
+        lowerValue == kRouteTypeBicycle || lowerValue == kRouteTypeTransit)
+    {
+      m_routingType = lowerValue;
+    }
+    else
+    {
+      LOG(LWARNING, ("Incorrect routing type:", value));
+      return;
+    }
+  }
+
+  pattern.erase(pattern.begin());
+}
+
+void ParsedMapApi::ParseSearchParam(url::Param const & param, SearchRequest & request) const
 {
  using namespace search;

+  string const & key = param.m_name;
+  string const & value = param.m_value;
+
  if (key == kQuery)
  {
-    if (value.empty())
-      return false;
-
    request.m_query = value;
  }
  else if (key == kCenterLatLon)
  {
    double lat = 0.0;
    double lon = 0.0;
-    if (ParseLatLon(key, value, lat, lon))
+    if (ParseLatLon(param, lat, lon))
    {
      request.m_centerLat = lat;
      request.m_centerLon = lon;
@ -465,14 +470,15 @@ bool ParsedMapApi::SearchKeyValue(string const & key, string const & value, Sear
  {
    request.m_isSearchOnMap = true;
  }
-
-  return true;
 }

-bool ParsedMapApi::LeadKeyValue(string const & key, string const & value, lead::CampaignDescription & description) const
+void ParsedMapApi::ParseLeadParam(url::Param const & param, lead::CampaignDescription & description) const
 {
  using namespace lead;

+  string const & key = param.m_name;
+  string const & value = param.m_value;
+
  if (key == kFrom)
    description.m_from = value;
  else if (key == kType)
@ -483,38 +489,31 @@ bool ParsedMapApi::LeadKeyValue(string const & key, string const & value, lead::
    description.m_content = value;
  else if (key == kKeyword)
    description.m_keyword = value;
-  /*
-   We have to support parsing the uri which contains unregistred parameters.
-   */
-  return true;
 }

-bool ParsedMapApi::CatalogKeyValue(string const & key, string const & value, Catalog & item) const
+void ParsedMapApi::ParseCatalogParam(url::Param const & param, Catalog & item) const
 {
  using namespace catalogue;

+  string const & key = param.m_name;
+  string const & value = param.m_value;
+
  if (key == kName)
    item.m_name = value;
  else if (key == kId)
    item.m_id = value;
-
-  return true;
 }

-bool ParsedMapApi::CatalogPathKeyValue(string const & key, string const & value, CatalogPath & item) const
+void ParsedMapApi::ParseCatalogPathParam(url::Param const & param, CatalogPath & item) const
 {
-  if (key == catalogue_path::kUrl)
-    item.m_url = value;
-
-  return true;
+  if (param.m_name == catalogue_path::kUrl)
+    item.m_url = param.m_value;
 }

-bool ParsedMapApi::SubscriptionKeyValue(string const & key, string const & value, Subscription & item) const
+void ParsedMapApi::ParseSubscriptionParam(url::Param const & param, Subscription & item) const
 {
-  if (key == subscription::kGroups)
-    item.m_groups = value;
-
-  return true;
+  if (param.m_name == subscription::kGroups)
+    item.m_groups = param.m_value;
 }

 void ParsedMapApi::Reset()
--- a/map/mwm_url.hpp
+++ b/map/mwm_url.hpp
@ -1,5 +1,7 @@
 #pragma once

+#include "coding/url.hpp"
+
 #include "geometry/point2d.hpp"
 #include "geometry/rect2d.hpp"

@ -102,13 +104,13 @@ public:
  Subscription const & GetSubscription() const { return m_subscription; }
 private:
  ParsingResult Parse(url::Url const & url);
-  bool AddKeyValue(std::string const & key, std::string const & value, std::vector<ApiPoint> & points);
-  bool RouteKeyValue(std::string const & key, std::string const & value, std::vector<std::string> & pattern);
-  bool SearchKeyValue(std::string const & key, std::string const & value, SearchRequest & request) const;
-  bool LeadKeyValue(std::string const & key, std::string const & value, lead::CampaignDescription & description) const;
-  bool CatalogKeyValue(std::string const & key, std::string const & value, Catalog & item) const;
-  bool CatalogPathKeyValue(std::string const & key, std::string const & value, CatalogPath & item) const;
-  bool SubscriptionKeyValue(std::string const & key, std::string const & value, Subscription & item) const;
+  void ParseMapParam(url::Param const & param, std::vector<ApiPoint> & points, bool & correctOrder);
+  void ParseRouteParam(url::Param const & param, std::vector<std::string> & pattern);
+  void ParseSearchParam(url::Param const & param, SearchRequest & request) const;
+  void ParseLeadParam(url::Param const & param, lead::CampaignDescription & description) const;
+  void ParseCatalogParam(url::Param const & param, Catalog & item) const;
+  void ParseCatalogPathParam(url::Param const & param, CatalogPath & item) const;
+  void ParseSubscriptionParam(url::Param const & param, Subscription & item) const;

  BookmarkManager * m_bmManager = nullptr;
  std::vector<RoutePoint> m_routePoints;