diff options
author | Sergey Magidovich <mgsergio@mapswithme.com> | 2016-05-24 12:50:41 +0300 |
---|---|---|
committer | Sergey Magidovich <mgsergio@mapswithme.com> | 2016-05-25 14:22:58 +0300 |
commit | ec26af0c03be006e89729b7f8cd84a675372cfc2 (patch) | |
tree | 0f596570beb2ecd23fecd5d5d65eaf44340865c9 | |
parent | 6c3124edee85535e6baa7259e810a3dabd2d39e8 (diff) |
Code review.
-rw-r--r-- | indexer/editable_map_object.cpp | 32 | ||||
-rw-r--r-- | indexer/editable_map_object.hpp | 1 | ||||
-rw-r--r-- | indexer/indexer_tests/editable_map_object_test.cpp | 12 | ||||
-rw-r--r-- | indexer/postcodes_matcher.cpp | 12 | ||||
-rw-r--r-- | indexer/postcodes_matcher.hpp | 8 | ||||
-rw-r--r-- | indexer/string_slice.hpp | 1 | ||||
-rw-r--r-- | search/processor.cpp | 2 | ||||
-rw-r--r-- | search/v2/geocoder.cpp | 3 | ||||
-rw-r--r-- | search/v2/token_slice.hpp | 4 |
9 files changed, 35 insertions, 40 deletions
diff --git a/indexer/editable_map_object.cpp b/indexer/editable_map_object.cpp index 9c63a55091..5d63d3e89b 100644 --- a/indexer/editable_map_object.cpp +++ b/indexer/editable_map_object.cpp @@ -195,7 +195,7 @@ void EditableMapObject::SetPointType() { m_geomType = feature::EGeomType::GEOM_P // static bool EditableMapObject::ValidateBuildingLevels(string const & buildingLevels) { - if (buildingLevels.size() > log10(kMaximumLevelsEditableByUsers) + 1) + if (buildingLevels.size() > 18 /* max number of digits in uint_64 */) return false; uint64_t levels; @@ -205,6 +205,8 @@ bool EditableMapObject::ValidateBuildingLevels(string const & buildingLevels) // static bool EditableMapObject::ValidateHouseNumber(string const & houseNumber) { + // TODO(mgsergio): Make a better validation, use real samples for example. + if (houseNumber.empty()) return true; @@ -219,7 +221,7 @@ bool EditableMapObject::ValidateHouseNumber(string const & houseNumber) for (auto const c : us) { // Valid house numbers contain at least one digit. - if (c >= '0' && c <= '9') + if (strings::IsASCIIDigit(c)) return true; } return false; @@ -249,7 +251,7 @@ bool EditableMapObject::ValidateFlats(string const & flats) // static bool EditableMapObject::ValidatePostCode(string const & postCode) { - return search::LooksLikePostcode(postCode, false /* check whole matching */); + return search::LooksLikePostcode(postCode, false /* IsPrefix */); } // static @@ -258,24 +260,24 @@ bool EditableMapObject::ValidatePhone(string const & phone) if (phone.empty()) return true; - auto start = begin(phone); - auto const stop = end(phone); + auto curr = begin(phone); + auto const last = end(phone); auto const kMaxNumberLen = 15; auto const kMinNumberLen = 5; - if (*start == '+') - ++start; + if (*curr == '+') + ++curr; auto digitsCount = 0; - for (; start != stop; ++start) + for (; curr != last; ++curr) { - auto const isCharValid = isdigit(*start) || *start == '(' || - *start == ')' || *start == ' ' || *start == '-'; + auto const isCharValid = isdigit(*curr) || *curr == '(' || + *curr == ')' || *curr == ' ' || *curr == '-'; if (!isCharValid) return false; - if (isdigit(*start)) + if (isdigit(*curr)) ++digitsCount; } @@ -283,19 +285,13 @@ bool EditableMapObject::ValidatePhone(string const & phone) } // static -bool EditableMapObject::ValidateFax(string const & fax) -{ - return ValidatePhone(fax); -} - -// static bool EditableMapObject::ValidateWebsite(string const & site) { if (site.empty()) return true; auto const dotPos = find(begin(site), end(site), '.'); - // Site should contain at least one dot but not at the begining and not at the and. + // Site should contain at least one dot but not at the begining/and. if (dotPos == end(site) || site.front() == '.' || site.back() == '.') return false; diff --git a/indexer/editable_map_object.hpp b/indexer/editable_map_object.hpp index ac7b0db100..747082bf0d 100644 --- a/indexer/editable_map_object.hpp +++ b/indexer/editable_map_object.hpp @@ -111,7 +111,6 @@ public: static bool ValidateFlats(string const & flats); static bool ValidatePostCode(string const & postCode); static bool ValidatePhone(string const & phone); - static bool ValidateFax(string const & fax); static bool ValidateWebsite(string const & site); static bool ValidateEmail(string const & email); diff --git a/indexer/indexer_tests/editable_map_object_test.cpp b/indexer/indexer_tests/editable_map_object_test.cpp index 2f808d84ae..5857a8f8ce 100644 --- a/indexer/indexer_tests/editable_map_object_test.cpp +++ b/indexer/indexer_tests/editable_map_object_test.cpp @@ -2,6 +2,8 @@ #include "indexer/editable_map_object.hpp" +namespace +{ using osm::EditableMapObject; UNIT_TEST(EditableMapObject_SetWebsite) @@ -10,7 +12,7 @@ UNIT_TEST(EditableMapObject_SetWebsite) emo.SetWebsite("https://some.thing.org"); TEST_EQUAL(emo.GetWebsite(), "https://some.thing.org", ()); - emo.SetWebsite("some.thing.org"); + emo.SetWebsite("http://some.thing.org"); TEST_EQUAL(emo.GetWebsite(), "http://some.thing.org", ()); emo.SetWebsite("some.thing.org"); @@ -18,7 +20,6 @@ UNIT_TEST(EditableMapObject_SetWebsite) emo.SetWebsite(""); TEST_EQUAL(emo.GetWebsite(), "", ()); - } UNIT_TEST(EditableMapObject_ValidateBuildingLevels) @@ -54,6 +55,7 @@ UNIT_TEST(EditableMapObject_ValidateFlats) TEST(EditableMapObject::ValidateFlats("123-456;a-e"), ()); TEST(EditableMapObject::ValidateFlats("123-456"), ()); TEST(EditableMapObject::ValidateFlats("123-456; 43-45"), ()); + TEST(!EditableMapObject::ValidateFlats("123-456, 43-45"), ()); TEST(!EditableMapObject::ValidateFlats("234-234 124"), ()); TEST(!EditableMapObject::ValidateFlats("123-345-567"), ()); TEST(!EditableMapObject::ValidateFlats("234-234;234("), ()); @@ -84,11 +86,6 @@ UNIT_TEST(EditableMapObject_ValidatePhone) TEST(!EditableMapObject::ValidatePhone("000 000 00b"), ()); } -// See validate phone. -// UNIT_TEST(EditableMapObject_ValidateFax) -// { -// } - UNIT_TEST(EditableMapObject_ValidateWebsite) { TEST(EditableMapObject::ValidateWebsite(""), ()); @@ -111,3 +108,4 @@ UNIT_TEST(EditableMapObject_ValidateEmail) TEST(!EditableMapObject::ValidateEmail("e@ma@i.l"), ()); TEST(!EditableMapObject::ValidateEmail("e@mail"), ()); } +} // namespace diff --git a/indexer/postcodes_matcher.cpp b/indexer/postcodes_matcher.cpp index 9c7889d793..143b462216 100644 --- a/indexer/postcodes_matcher.cpp +++ b/indexer/postcodes_matcher.cpp @@ -102,7 +102,7 @@ public: // patterns. // // Complexity: O(total length of tokens in |slice|). - bool HasString(StringSliceBase const & slice, bool handleAsPrefix) const + bool HasString(StringSliceBase const & slice, bool isPrefix) const { if (slice.Size() == 0) return m_root.m_isLeaf; @@ -120,7 +120,7 @@ public: if (!cur) return false; - if (handleAsPrefix) + if (isPrefix) return true; return cur->m_isLeaf; @@ -159,18 +159,18 @@ PostcodesMatcher const & GetPostcodesMatcher() } } // namespace -bool LooksLikePostcode(StringSliceBase const & slice, bool handleAsPrefix) +bool LooksLikePostcode(StringSliceBase const & slice, bool isPrefix) { - return GetPostcodesMatcher().HasString(slice, handleAsPrefix); + return GetPostcodesMatcher().HasString(slice, isPrefix); } -bool LooksLikePostcode(string const & s, bool handleAsPrefix) +bool LooksLikePostcode(string const & s, bool isPrefix) { vector<UniString> tokens; bool const lastTokenIsPrefix = TokenizeStringAndCheckIfLastTokenIsPrefix(s, tokens, search::Delimiters()); - return LooksLikePostcode(NoPrefixStringSlice(tokens), handleAsPrefix && lastTokenIsPrefix); + return LooksLikePostcode(NoPrefixStringSlice(tokens), isPrefix && lastTokenIsPrefix); } size_t GetMaxNumTokensInPostcode() { return GetPostcodesMatcher().GetMaxNumTokensInPostcode(); } diff --git a/indexer/postcodes_matcher.hpp b/indexer/postcodes_matcher.hpp index 3d0ab900a9..d7af92e984 100644 --- a/indexer/postcodes_matcher.hpp +++ b/indexer/postcodes_matcher.hpp @@ -7,11 +7,11 @@ namespace search { -bool LooksLikePostcode(StringSliceBase const & slice, bool handleAsPrefix); +/// If isPrefix is true returns true if some postcode starts with s. +/// If isPrefix is false returns true if s equals to some postcode. +bool LooksLikePostcode(StringSliceBase const & slice, bool isPrefix); /// Splits s into tokens and call LooksLikePostcode(TokenSlice) on the result. -/// If handleAsPrefix is true returns true if some postcode starts with s. -/// If handleAsPrefix is false returns true if s equals to some postcode. -bool LooksLikePostcode(string const & s, bool handleAsPrefix); +bool LooksLikePostcode(string const & s, bool isPrefix); size_t GetMaxNumTokensInPostcode(); } // namespace search diff --git a/indexer/string_slice.hpp b/indexer/string_slice.hpp index 4bf3eb6a13..510cdbe8cb 100644 --- a/indexer/string_slice.hpp +++ b/indexer/string_slice.hpp @@ -2,6 +2,7 @@ #include "base/string_utils.hpp" +#include "std/cstdint.hpp" #include "std/string.hpp" #include "std/vector.hpp" diff --git a/search/processor.cpp b/search/processor.cpp index 0cc1fb9d26..c81926d3c3 100644 --- a/search/processor.cpp +++ b/search/processor.cpp @@ -611,7 +611,7 @@ class PreResult2Maker feature::TypesHolder holder(ft); vector<pair<size_t, size_t>> matched(slice.Size()); - m_processor.ForEachCategoryTypes(v2::QuerySliceOnTokens(slice), [&](size_t i, uint32_t t) + m_processor.ForEachCategoryTypes(v2::QuerySlice(slice), [&](size_t i, uint32_t t) { ++matched[i].second; if (holder.Has(t)) diff --git a/search/v2/geocoder.cpp b/search/v2/geocoder.cpp index 8bc16e49eb..30981bd454 100644 --- a/search/v2/geocoder.cpp +++ b/search/v2/geocoder.cpp @@ -1011,7 +1011,8 @@ void Geocoder::WithPostcodes(TFn && fn) break; TokenSlice slice(m_params, startToken, startToken + n); - if (LooksLikePostcode(QuerySliceOnTokens(slice), true /*handleAsPrefix*/)) + auto const isPrefix = startToken + n == m_numTokens; + if (LooksLikePostcode(QuerySlice(slice), isPrefix)) endToken = startToken + n; } if (startToken == endToken) diff --git a/search/v2/token_slice.hpp b/search/v2/token_slice.hpp index d6cc2bc02d..725ea98355 100644 --- a/search/v2/token_slice.hpp +++ b/search/v2/token_slice.hpp @@ -69,10 +69,10 @@ private: vector<size_t> m_indexes; }; -class QuerySliceOnTokens : public StringSliceBase +class QuerySlice : public StringSliceBase { public: - QuerySliceOnTokens(TokenSlice const & slice) : m_slice(slice) {} + QuerySlice(TokenSlice const & slice) : m_slice(slice) {} // QuerySlice overrides: QueryParams::TString const & Get(size_t i) const override { return m_slice.Get(i).front(); } |