From 3fd79eef0c192eecf70f00e1f679e8ccb12c11e2 Mon Sep 17 00:00:00 2001 From: Maxim Pimenov Date: Wed, 12 Feb 2020 18:57:09 +0300 Subject: [PATCH] [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). --- coding/coding_tests/url_tests.cpp | 3 +- coding/url.cpp | 13 +- coding/url.hpp | 4 +- map/map_tests/mwm_url_tests.cpp | 10 +- map/mwm_url.cpp | 211 +++++++++++++++--------------- map/mwm_url.hpp | 16 ++- 6 files changed, 125 insertions(+), 132 deletions(-) diff --git a/coding/coding_tests/url_tests.cpp b/coding/coding_tests/url_tests.cpp index 0d573fdede..c0f18da886 100644 --- 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; diff --git a/coding/url.cpp b/coding/url.cpp index 9f3e09c767..6e7c361d9c 100644 --- 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) diff --git a/coding/url.hpp b/coding/url.hpp index 13c0d29ece..982a4595da 100644 --- a/coding/url.hpp +++ b/coding/url.hpp @@ -26,14 +26,14 @@ using Params = std::vector; class Url { public: - using Callback = std::function; + using Callback = std::function; 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); diff --git a/map/map_tests/mwm_url_tests.cpp b/map/map_tests/mwm_url_tests.cpp index a452cc8c4c..5b3efd4b17 100644 --- 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) { diff --git a/map/mwm_url.cpp b/map/mwm_url.cpp index 8a9c0ae273..9b147d50c8 100644 --- 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 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 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 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 & 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 & points) +void ParsedMapApi::ParseMapParam(url::Param const & param, vector & 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 & 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() diff --git a/map/mwm_url.hpp b/map/mwm_url.hpp index e801941041..bc4b4ff9eb 100644 --- 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 & points); - bool RouteKeyValue(std::string const & key, std::string const & value, std::vector & 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 & points, bool & correctOrder); + void ParseRouteParam(url::Param const & param, std::vector & 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 m_routePoints;