From c011846c948a230ba27ffd0362ed8d8a785af20c Mon Sep 17 00:00:00 2001 From: Arsentiy Milchakov Date: Mon, 13 Nov 2017 15:38:51 +0300 Subject: [PATCH] [booking] review fixes --- .../generator_tests_support/test_feature.cpp | 24 +++- .../generator_tests_support/test_feature.hpp | 7 +- .../test_mwm_environment.hpp | 2 +- map/booking_filter.cpp | 103 ++++++++++++------ map/booking_filter.hpp | 7 +- map/booking_filter_cache.cpp | 7 +- map/booking_filter_cache.hpp | 10 +- map/framework.cpp | 8 +- map/framework.hpp | 4 +- .../booking_availability_cache_test.cpp | 10 +- map/map_tests/booking_filter_test.cpp | 29 ++--- map/search_api.cpp | 4 +- map/search_api.hpp | 4 +- 13 files changed, 134 insertions(+), 85 deletions(-) diff --git a/generator/generator_tests_support/test_feature.cpp b/generator/generator_tests_support/test_feature.cpp index 85116b43b5..972849381c 100644 --- a/generator/generator_tests_support/test_feature.cpp +++ b/generator/generator_tests_support/test_feature.cpp @@ -40,11 +40,13 @@ uint64_t GenUniqueId() TestFeature::TestFeature(string const & name, string const & lang) : m_id(GenUniqueId()), m_center(0, 0), m_type(Type::Unknown), m_name(name), m_lang(lang) { + Init(); } TestFeature::TestFeature(m2::PointD const & center, string const & name, string const & lang) : m_id(GenUniqueId()), m_center(center), m_type(Type::Point), m_name(name), m_lang(lang) { + Init(); } TestFeature::TestFeature(vector const & boundary, string const & name, @@ -52,6 +54,12 @@ TestFeature::TestFeature(vector const & boundary, string const & nam : m_id(GenUniqueId()), m_boundary(boundary), m_type(Type::Area), m_name(name), m_lang(lang) { ASSERT(!m_boundary.empty(), ()); + Init(); +} + +void TestFeature::Init() +{ + m_metadata.Set(feature::Metadata::FMD_TEST_ID, strings::to_string(m_id)); } bool TestFeature::Matches(FeatureType const & feature) const @@ -64,8 +72,18 @@ bool TestFeature::Matches(FeatureType const & feature) const void TestFeature::Serialize(FeatureBuilder1 & fb) const { - auto & metadata = fb.GetMetadataForTesting(); - metadata.Set(feature::Metadata::FMD_TEST_ID, strings::to_string(m_id)); + using feature::Metadata; + size_t i = static_cast(Metadata::EType::FMD_CUISINE); + size_t const count = static_cast(Metadata::EType::FMD_COUNT); + for (; i < count; ++i) + { + auto const type = static_cast(i); + if (m_metadata.Has(type)) + { + auto const value = m_metadata.Get(type); + fb.GetMetadataForTesting().Set(type, value); + } + } switch (m_type) { @@ -231,8 +249,6 @@ void TestPOI::Serialize(FeatureBuilder1 & fb) const fb.AddHouseNumber(m_houseNumber); if (!m_streetName.empty()) fb.AddStreet(m_streetName); - if (!m_metadata.Empty()) - fb.GetMetadataForTesting() = m_metadata; } string TestPOI::ToString() const diff --git a/generator/generator_tests_support/test_feature.hpp b/generator/generator_tests_support/test_feature.hpp index 77d08dc85e..28fca0e48d 100644 --- a/generator/generator_tests_support/test_feature.hpp +++ b/generator/generator_tests_support/test_feature.hpp @@ -32,6 +32,7 @@ public: inline void SetPostcode(std::string const & postcode) { m_postcode = postcode; } inline uint64_t GetId() const { return m_id; } inline std::string const & GetName() const { return m_name; } + inline feature::Metadata & GetMetadata() { return m_metadata; } virtual void Serialize(FeatureBuilder1 & fb) const; virtual std::string ToString() const = 0; @@ -56,6 +57,10 @@ protected: std::string const m_name; std::string const m_lang; std::string m_postcode; + feature::Metadata m_metadata; + +private: + void Init(); }; class TestCountry : public TestFeature @@ -125,13 +130,11 @@ public: inline void SetHouseNumber(std::string const & houseNumber) { m_houseNumber = houseNumber; } inline void SetStreet(TestStreet const & street) { m_streetName = street.GetName(); } inline void SetTypes(std::vector> const & types) { m_types = types; } - inline void SetMetadata(feature::Metadata const & metadata) { m_metadata = metadata; } private: std::string m_houseNumber; std::string m_streetName; std::vector> m_types; - feature::Metadata m_metadata; }; class TestBuilding : public TestFeature diff --git a/indexer/indexer_tests_support/test_mwm_environment.hpp b/indexer/indexer_tests_support/test_mwm_environment.hpp index 9d0e439f53..272c26a824 100644 --- a/indexer/indexer_tests_support/test_mwm_environment.hpp +++ b/indexer/indexer_tests_support/test_mwm_environment.hpp @@ -80,4 +80,4 @@ private: MwmList m_mwmFiles; }; } // namespace tests_support -} // namespace indexer \ No newline at end of file +} // namespace indexer diff --git a/map/booking_filter.cpp b/map/booking_filter.cpp index a38e894f89..e230604207 100644 --- a/map/booking_filter.cpp +++ b/map/booking_filter.cpp @@ -16,23 +16,21 @@ namespace { struct HotelToResult { - HotelToResult(std::string const & id, search::Result const & result, - availability::Cache::HotelStatus status) - : m_hotelId(id), m_result(result), m_cacheStatus(status) - { - } + HotelToResult(search::Result const & result) : m_result(result) {} - std::string m_hotelId; search::Result m_result; + std::string m_hotelId; availability::Cache::HotelStatus m_cacheStatus; }; +using HotelToResults = std::vector; + void FillAvailability(HotelToResult & hotelToResult, std::vector const & hotelIds, availability::Cache & cache, search::Results & results) { using availability::Cache; - if (hotelToResult.m_cacheStatus == Cache::HotelStatus::UnAvailable) + if (hotelToResult.m_cacheStatus == Cache::HotelStatus::Unavailable) return; if (hotelToResult.m_cacheStatus == Cache::HotelStatus::Available) @@ -62,10 +60,69 @@ void FillAvailability(HotelToResult & hotelToResult, std::vector co } else { - cache.Insert(hotelToResult.m_hotelId, Cache::HotelStatus::UnAvailable); + cache.Insert(hotelToResult.m_hotelId, Cache::HotelStatus::Unavailable); } } } + +void PrepareData(Index const & index, search::Results const & results, + HotelToResults & hotelToResults, availability::Cache & cache, + booking::AvailabilityParams & p) +{ + std::vector features; + + for (auto const & r : results) + { + if (!r.m_metadata.m_isSponsoredHotel || + r.GetResultType() != search::Result::ResultType::RESULT_FEATURE) + { + continue; + } + + features.push_back(r.GetFeatureID()); + hotelToResults.emplace_back(r); + } + + std::sort(features.begin(), features.end()); + + MwmSet::MwmId currentMwmId; + std::unique_ptr guard; + for (auto const & featureId : features) + { + if (currentMwmId != featureId.m_mwmId) + { + guard = my::make_unique(index, featureId.m_mwmId); + currentMwmId = featureId.m_mwmId; + } + + auto it = std::find_if(hotelToResults.begin(), hotelToResults.end(), + [&featureId](HotelToResult const & item) + { + return item.m_result.GetFeatureID() == featureId; + }); + ASSERT(it != hotelToResults.cend(), ()); + + FeatureType ft; + if (!guard->GetFeatureByIndex(featureId.m_index, ft)) + { + hotelToResults.erase(it); + LOG(LERROR, ("Feature can't be loaded:", featureId)); + continue; + } + + std::string hotelId = ft.GetMetadata().Get(feature::Metadata::FMD_SPONSORED_ID); + auto const status = cache.Get(hotelId); + + it->m_hotelId = hotelId; + it->m_cacheStatus = status; + + if (status != availability::Cache::HotelStatus::Absent) + continue; + + cache.Reserve(hotelId); + p.m_hotelIds.push_back(std::move(hotelId)); + } +} } // namespace namespace booking @@ -75,40 +132,16 @@ namespace filter Filter::Filter(Index const & index, booking::Api const & api) : m_index(index), m_api(api) {} void Filter::Availability(search::Results const & results, - availability::internal::Params const & params) const + availability::internal::Params && params) const { auto & p = params.m_params; auto const & cb = params.m_callback; ASSERT(p.m_hotelIds.empty(), ()); - std::vector hotelToResults; - // Fill hotel ids. - for (auto const & r : results) - { - if (!r.m_metadata.m_isSponsoredHotel || - r.GetResultType() != search::Result::ResultType::RESULT_FEATURE) - continue; + HotelToResults hotelToResults; - auto const & id = r.GetFeatureID(); - Index::FeaturesLoaderGuard const guard(m_index, id.m_mwmId); - FeatureType ft; - if (!guard.GetFeatureByIndex(id.m_index, ft)) - { - LOG(LERROR, ("Feature can't be loaded:", id)); - continue; - } - - std::string hotelId = ft.GetMetadata().Get(feature::Metadata::FMD_SPONSORED_ID); - auto const status = m_availabilityCache.Get(hotelId); - hotelToResults.emplace_back(hotelId, r, status); - - if (status != availability::Cache::HotelStatus::Absent) - continue; - - m_availabilityCache.Reserve(hotelId); - p.m_hotelIds.push_back(std::move(hotelId)); - } + PrepareData(m_index, results, hotelToResults, m_availabilityCache, p); auto const apiCallback = [this, cb, hotelToResults](std::vector hotelIds) mutable { diff --git a/map/booking_filter.hpp b/map/booking_filter.hpp index a74de37991..c699a85925 100644 --- a/map/booking_filter.hpp +++ b/map/booking_filter.hpp @@ -3,13 +3,13 @@ #include "map/booking_filter_availability_params.hpp" #include "map/booking_filter_cache.hpp" +class Index; + namespace search { class Results; } -class Index; - namespace booking { class Api; @@ -20,8 +20,9 @@ class Filter { public: Filter(Index const & index, booking::Api const & api); + void Availability(search::Results const & results, - availability::internal::Params const & params) const; + availability::internal::Params && params) const; private: Index const & m_index; diff --git a/map/booking_filter_cache.cpp b/map/booking_filter_cache.cpp index b39fbe8589..15bbe1b8b3 100644 --- a/map/booking_filter_cache.cpp +++ b/map/booking_filter_cache.cpp @@ -29,8 +29,7 @@ void Cache::Reserve(std::string const & hotelId) { std::lock_guard lock(m_mutex); - Item item; - m_hotelToResult.emplace(hotelId, std::move(item)); + m_hotelToResult.emplace(hotelId, HotelStatus::NotReady); } void Cache::Insert(std::string const & hotelId, HotelStatus const s) @@ -43,9 +42,7 @@ void Cache::Insert(std::string const & hotelId, HotelStatus const s) m_hotelToResult[hotelId] = std::move(item); if (!m_agingInProgress) - { RemoveOutdated(); - } } void Cache::RemoveOutdated() @@ -95,7 +92,7 @@ std::string DebugPrint(Cache::HotelStatus status) { case Cache::HotelStatus::Absent: return "Absent"; case Cache::HotelStatus::NotReady: return "NotReady"; - case Cache::HotelStatus::UnAvailable: return "UnAvailable"; + case Cache::HotelStatus::Unavailable: return "Unavailable"; case Cache::HotelStatus::Available: return "Available"; } } diff --git a/map/booking_filter_cache.hpp b/map/booking_filter_cache.hpp index d5a4e19a6d..faa669a700 100644 --- a/map/booking_filter_cache.hpp +++ b/map/booking_filter_cache.hpp @@ -17,9 +17,13 @@ class Cache public: enum class HotelStatus { + // The hotel is absent in cache. Absent, + // Information about the hotel was requested, but request is not ready yet. NotReady, - UnAvailable, + // The hotel is unavailable for booking. + Unavailable, + // The hotel is available for booking. Available, }; @@ -40,8 +44,6 @@ public: void Reserve(std::string const & hotelId); void Insert(std::string const & hotelId, HotelStatus const s); - DISALLOW_COPY_AND_MOVE(Cache); - private: void RemoveOutdated(); void RemoveExtra(); @@ -54,6 +56,8 @@ private: size_t const m_maxCount = 1000; // Aging process is disabled when |m_expiryPeriodSeconds| is equal to zero. size_t const m_expiryPeriodSeconds = 60; + + DISALLOW_COPY_AND_MOVE(Cache); }; std::string DebugPrint(Cache::HotelStatus status); diff --git a/map/framework.cpp b/map/framework.cpp index 471d4694f7..49e974fa92 100644 --- a/map/framework.cpp +++ b/map/framework.cpp @@ -3306,13 +3306,13 @@ ugc::Reviews Framework::FilterUGCReviews(ugc::Reviews const & reviews) const return result; } -void Framework::FilterSearchResults(booking::filter::availability::Params const & params, - search::Results const & results, bool inViewport) +void Framework::FilterSearchResultsOnBooking(booking::filter::availability::Params const & params, + search::Results const & results, bool inViewport) { using namespace booking::filter; auto const & cb = params.m_callback; - availability::internal::Params const paramsInternal + availability::internal::Params paramsInternal { params.m_params, [this, cb, inViewport](search::Results const & results) @@ -3340,5 +3340,5 @@ void Framework::FilterSearchResults(booking::filter::availability::Params const } }; - m_bookingFilter.Availability(results, paramsInternal); + m_bookingFilter.Availability(results, std::move(paramsInternal)); } diff --git a/map/framework.hpp b/map/framework.hpp index b801e65c7f..2d7afc67f6 100644 --- a/map/framework.hpp +++ b/map/framework.hpp @@ -825,6 +825,6 @@ private: ugc::Reviews FilterUGCReviews(ugc::Reviews const & reviews) const; public: - void FilterSearchResults(booking::filter::availability::Params const & params, - search::Results const & results, bool inViewport) override; + void FilterSearchResultsOnBooking(booking::filter::availability::Params const & params, + search::Results const & results, bool inViewport) override; }; diff --git a/map/map_tests/booking_availability_cache_test.cpp b/map/map_tests/booking_availability_cache_test.cpp index 8c4357953b..30dbd33b3b 100644 --- a/map/map_tests/booking_availability_cache_test.cpp +++ b/map/map_tests/booking_availability_cache_test.cpp @@ -12,7 +12,7 @@ namespace { UNIT_TEST(AvailabilityCache_Smoke) { - Cache cache(2, 0); + Cache cache(2 /* maxCount */, 0 /* expiryPeriodSeconds */); std::string kHotelId = "0"; @@ -26,14 +26,14 @@ UNIT_TEST(AvailabilityCache_Smoke) TEST_EQUAL(cache.Get(kHotelId), Cache::HotelStatus::Available, ()); - cache.Insert(kHotelId, Cache::HotelStatus::UnAvailable); + cache.Insert(kHotelId, Cache::HotelStatus::Unavailable); - TEST_EQUAL(cache.Get(kHotelId), Cache::HotelStatus::UnAvailable, ()); + TEST_EQUAL(cache.Get(kHotelId), Cache::HotelStatus::Unavailable, ()); } UNIT_TEST(AvailabilityCache_RemoveExtra) { - Cache cache(3, 0); + Cache cache(3 /* maxCount */, 0 /* expiryPeriodSeconds */); std::vector const kHotelIds = {"1", "2", "3"}; for (auto const & id : kHotelIds) @@ -52,4 +52,4 @@ UNIT_TEST(AvailabilityCache_RemoveExtra) TEST_EQUAL(cache.Get("4"), Cache::HotelStatus::Available, ()); } -} // namespace \ No newline at end of file +} // namespace diff --git a/map/map_tests/booking_filter_test.cpp b/map/map_tests/booking_filter_test.cpp index 1cf0811a99..2a989956f9 100644 --- a/map/map_tests/booking_filter_test.cpp +++ b/map/map_tests/booking_filter_test.cpp @@ -33,39 +33,34 @@ UNIT_TEST(BookingFilter_AvailabilitySmoke) env.BuildMwm("TestMwm", [&kHotelIds](TestMwmBuilder & builder) { - feature::Metadata metadata; - metadata.Set(feature::Metadata::FMD_SPONSORED_ID, kHotelIds[0]); TestPOI hotel1(m2::PointD(1.0, 1.0), "hotel 1", "en"); - hotel1.SetMetadata(metadata); + hotel1.GetMetadata().Set(feature::Metadata::FMD_SPONSORED_ID, kHotelIds[0]); builder.Add(hotel1); TestPOI hotel2(m2::PointD(1.1, 1.1), "hotel 2", "en"); - metadata.Set(feature::Metadata::FMD_SPONSORED_ID, kHotelIds[1]); - hotel2.SetMetadata(metadata); + hotel2.GetMetadata().Set(feature::Metadata::FMD_SPONSORED_ID, kHotelIds[1]); builder.Add(hotel2); TestPOI hotel3(m2::PointD(0.9, 0.9), "hotel 3", "en"); - metadata.Set(feature::Metadata::FMD_SPONSORED_ID, kHotelIds[2]); - hotel3.SetMetadata(metadata); + hotel3.GetMetadata().Set(feature::Metadata::FMD_SPONSORED_ID, kHotelIds[2]); builder.Add(hotel3); }); - m2::RectD const rect = MercatorBounds::RectByCenterXYAndSizeInMeters( - m2::PointD(1.0, 1.0), 2 / MercatorBounds::degreeInMetres /* rect width */); + m2::RectD const rect(m2::PointD(0.5, 0.5), m2::PointD(1.5, 1.5)); search::Results results; results.AddResult({"suggest for testing", "suggest for testing"}); - search::Results expctedResults; + search::Results expectedResults; env.GetIndex().ForEachInRect( - [&results, &expctedResults](FeatureType & ft) { + [&results, &expectedResults](FeatureType & ft) { search::Result::Metadata metadata; metadata.m_isSponsoredHotel = true; search::Result result(ft.GetID(), ft.GetCenter(), "", "", "", 0, metadata); auto copy = result; results.AddResult(std::move(result)); - expctedResults.AddResult(std::move(copy)); + expectedResults.AddResult(std::move(copy)); }, rect, scales::GetUpperScale()); - availability::ParamsInternal params; + availability::internal::Params params; search::Results filteredResults; params.m_callback = [&filteredResults](search::Results const & results) { @@ -73,16 +68,16 @@ UNIT_TEST(BookingFilter_AvailabilitySmoke) testing::Notify(); }; - filter.Availability(results, params); + filter.Availability(results, std::move(params)); testing::Wait(); TEST_NOT_EQUAL(filteredResults.GetCount(), 0, ()); - TEST_EQUAL(filteredResults.GetCount(), expctedResults.GetCount(), ()); + TEST_EQUAL(filteredResults.GetCount(), expectedResults.GetCount(), ()); for (size_t i = 0; i < filteredResults.GetCount(); ++i) { - TEST_EQUAL(filteredResults[i].GetFeatureID(), expctedResults[i].GetFeatureID(), ()); + TEST_EQUAL(filteredResults[i].GetFeatureID(), expectedResults[i].GetFeatureID(), ()); } } -} // namespace \ No newline at end of file +} // namespace diff --git a/map/search_api.cpp b/map/search_api.cpp index 4ddb750658..2b4b10ef15 100644 --- a/map/search_api.cpp +++ b/map/search_api.cpp @@ -99,7 +99,7 @@ bool SearchAPI::SearchEverywhere(EverywhereSearchParams const & params) if (results.IsEndMarker() && results.IsEndedNormal() && !params.m_bookingFilterParams.IsEmpty()) { - m_delegate.FilterSearchResults(params.m_bookingFilterParams, results, false); + m_delegate.FilterSearchResultsOnBooking(params.m_bookingFilterParams, results, false); } }); @@ -137,7 +137,7 @@ bool SearchAPI::SearchInViewport(ViewportSearchParams const & params) if (results.IsEndMarker() && results.IsEndedNormal() && !params.m_bookingFilterParams.IsEmpty()) { - m_delegate.FilterSearchResults(params.m_bookingFilterParams, results, true); + m_delegate.FilterSearchResultsOnBooking(params.m_bookingFilterParams, results, true); } }); diff --git a/map/search_api.hpp b/map/search_api.hpp index 3a9b77072c..ec9ed364e6 100644 --- a/map/search_api.hpp +++ b/map/search_api.hpp @@ -69,8 +69,8 @@ public: virtual bool IsLocalAdsCustomer(search::Result const & /* result */) const { return false; } - virtual void FilterSearchResults(booking::filter::availability::Params const & params, - search::Results const & results, bool inViewport) + virtual void FilterSearchResultsOnBooking(booking::filter::availability::Params const & params, + search::Results const & results, bool inViewport) { } };