review fixes

6 files changed, 118 insertions, 68 deletions

diff --git a/local_ads/campaign.hpp b/local_ads/campaign.hpp
index 5eb795ef7f..52815a6429 100644
--- a/local_ads/campaign.hpp
+++ b/local_ads/campaign.hpp
@@ -29,15 +29,22 @@ struct Campaign
   std::string GetIconName() const { return IconsInfo::Instance().GetIcon(m_iconId); }
   uint32_t m_featureId = 0;
   uint16_t m_iconId = 0;
+  // Supported values range: 0-255. In case when accurate value is more than 255, we expect updated
+  // data will be received during this time interval. For ex. accurate value is 365, in this case
+  // first 110 days this field will store value 255.
   uint8_t m_daysBeforeExpired = 0;
-  uint8_t m_minZoomLevel = 16;  // supported values range: 10-17
-  uint8_t m_priority = 0;       // supported values range: 0-7
+  // Supported values range: 10-17.
+  uint8_t m_minZoomLevel = 16;
+  // Supported values range: 0-7
+  uint8_t m_priority = 0;
 };
 
 inline bool operator==(Campaign const & a, Campaign const & b)
 {
-  return a.m_featureId == b.m_featureId && a.m_iconId == b.m_iconId &&
-         a.m_daysBeforeExpired == b.m_daysBeforeExpired && a.m_minZoomLevel == b.m_minZoomLevel &&
+  return a.m_featureId == b.m_featureId &&
+         a.m_iconId == b.m_iconId &&
+         a.m_daysBeforeExpired == b.m_daysBeforeExpired &&
+         a.m_minZoomLevel == b.m_minZoomLevel &&
          a.m_priority == b.m_priority;
 }
 }  // namespace local_ads
diff --git a/local_ads/campaign_serialization.cpp b/local_ads/campaign_serialization.cpp
index 4b64e1a87d..7c20bf7042 100644
--- a/local_ads/campaign_serialization.cpp
+++ b/local_ads/campaign_serialization.cpp
@@ -13,8 +13,9 @@ namespace
 {
 using namespace local_ads;
 
-auto const kHalfByteShift = 0x4;
-auto const kLowerMask = 0xF;
+auto const kHalfByteShift = CHAR_BIT / 2;
+auto const kHalfByteMaxValue = 15;
+auto const kLowerMask = 0x0F;
 auto const kUpperMask = 0xF0;
 auto const kMinZoomLevel = 10;
 auto const kMaxZoomLevel = 17;
@@ -61,10 +62,27 @@ std::vector<Integral> ReadData(ByteStream & s, size_t chunksNumber)
 
   return result;
 }
+
+std::vector<uint8_t> SerializeV1(std::vector<Campaign> const & campaigns)
+{
+  std::vector<uint8_t> buff;
+  PushBackByteSink<decltype(buff)> dst(buff);
+  Write(dst, Version::V1);
+  Write(dst, campaigns.size());
+  for (auto const & c : campaigns)
+    WriteVarUint(dst, c.m_featureId);
+  for (auto const & c : campaigns)
+    WriteVarUint(dst, c.m_iconId);
+  for (auto const & c : campaigns)
+    Write(dst, c.m_daysBeforeExpired);
+
+  return buff;
+}
+
 std::vector<Campaign> DeserializeV1(std::vector<uint8_t> const & bytes)
 {
   ArrayByteSource src(bytes.data());
-  CHECK_EQUAL(Read<Version>(src), Version::v1, ());
+  CHECK_EQUAL(Read<Version>(src), Version::V1, ());
   auto const chunksNumber = Read<uint64_t>(src);
 
   auto const featureIds = ReadData<uint32_t>(src, chunksNumber);
@@ -88,10 +106,52 @@ uint8_t ZoomIndex(uint8_t zoomValue) { return zoomValue - kMinZoomLevel; }
 
 uint8_t ZoomValue(uint8_t zoomIndex) { return zoomIndex + kMinZoomLevel; }
 
+uint8_t PackZoomAndPriority(uint8_t minZoomLevel, uint8_t priority)
+{
+  ASSERT_GREATER_OR_EQUAL(minZoomLevel, kMinZoomLevel, ("Unsupported zoom level"));
+  ASSERT_LESS_OR_EQUAL(minZoomLevel, kMaxZoomLevel, ("Unsupported zoom level"));
+  ASSERT_LESS_OR_EQUAL(priority, kMaxPriority, ("Unsupported priority value"));
+
+  auto const zoomIndex = ZoomIndex(minZoomLevel);
+  ASSERT_LESS_OR_EQUAL(zoomIndex, kHalfByteMaxValue, ());
+  ASSERT_LESS_OR_EQUAL(priority, kHalfByteMaxValue, ());
+
+  // Pack zoom and priority into single byte.
+  return (zoomIndex & kLowerMask) | ((priority << kHalfByteShift) & kUpperMask);
+}
+
+uint8_t UnpackZoom(uint8_t src)
+{
+  return ZoomValue(src & kLowerMask);
+}
+
+uint8_t UnpackPriority(uint8_t src)
+{
+  return (src >> kHalfByteShift) & kLowerMask;
+}
+
+std::vector<uint8_t> SerializeV2(std::vector<Campaign> const & campaigns)
+{
+  std::vector<uint8_t> buff;
+  PushBackByteSink<decltype(buff)> dst(buff);
+  Write(dst, Version::V2);
+  Write(dst, campaigns.size());
+  for (auto const & c : campaigns)
+    WriteVarUint(dst, c.m_featureId);
+  for (auto const & c : campaigns)
+    WriteVarUint(dst, c.m_iconId);
+  for (auto const & c : campaigns)
+    Write(dst, c.m_daysBeforeExpired);
+  for (auto const & c : campaigns)
+    Write(dst, PackZoomAndPriority(c.m_minZoomLevel, c.m_priority));
+
+  return buff;
+}
+
 std::vector<Campaign> DeserializeV2(std::vector<uint8_t> const & bytes)
 {
   ArrayByteSource src(bytes.data());
-  CHECK_EQUAL(Read<Version>(src), Version::v2, ());
+  CHECK_EQUAL(Read<Version>(src), Version::V2, ());
   auto const chunksNumber = Read<uint64_t>(src);
 
   auto const featureIds = ReadData<uint32_t>(src, chunksNumber);
@@ -109,8 +169,8 @@ std::vector<Campaign> DeserializeV2(std::vector<uint8_t> const & bytes)
   for (size_t i = 0; i < chunksNumber; ++i)
   {
     campaigns.emplace_back(featureIds[i], icons[i], expirations[i],
-                           ZoomValue(zoomAndPriority[i] & kLowerMask),
-                           (zoomAndPriority[i] >> kHalfByteShift) & kLowerMask);
+                           UnpackZoom(zoomAndPriority[i]),
+                           UnpackPriority(zoomAndPriority[i]));
 
     ASSERT_GREATER_OR_EQUAL(campaigns.back().m_minZoomLevel, kMinZoomLevel,
                             ("Unsupported zoom level"));
@@ -126,32 +186,19 @@ namespace local_ads
 {
 std::vector<uint8_t> Serialize(std::vector<Campaign> const & campaigns, Version const version)
 {
-  std::vector<uint8_t> buff;
-  PushBackByteSink<decltype(buff)> dst(buff);
-  Write(dst, version);
-  Write(dst, campaigns.size());
-  for (auto const & c : campaigns)
-    WriteVarUint(dst, c.m_featureId);
-  for (auto const & c : campaigns)
-    WriteVarUint(dst, c.m_iconId);
-  for (auto const & c : campaigns)
-    Write(dst, c.m_daysBeforeExpired);
-  for (auto const & c : campaigns)
+  switch (version)
   {
-    ASSERT_GREATER_OR_EQUAL(c.m_minZoomLevel, kMinZoomLevel, ("Unsupported zoom level"));
-    ASSERT_LESS_OR_EQUAL(c.m_minZoomLevel, kMaxZoomLevel, ("Unsupported zoom level"));
-    ASSERT_LESS_OR_EQUAL(c.m_priority, kMaxPriority, ("Unsupported priority value"));
-
-    Write(dst, static_cast<uint8_t>((ZoomIndex(c.m_minZoomLevel) & kLowerMask) |
-                                    ((c.m_priority << kHalfByteShift) & kUpperMask)));
+  case Version::V1: return SerializeV1(campaigns);
+  case Version::V2: return SerializeV2(campaigns);
+  default: ASSERT(false, ("Unknown version"));
   }
 
-  return buff;
+  return {};
 }
 
 std::vector<uint8_t> Serialize(std::vector<Campaign> const & campaigns)
 {
-  return Serialize(campaigns, Version::latest);
+  return Serialize(campaigns, Version::Latest);
 }
 
 std::vector<Campaign> Deserialize(std::vector<uint8_t> const & bytes)
@@ -161,9 +208,9 @@ std::vector<Campaign> Deserialize(std::vector<uint8_t> const & bytes)
 
   switch (version)
   {
-  case Version::v1: return DeserializeV1(bytes);
-  case Version::v2: return DeserializeV2(bytes);
-  default: ASSERT(false, ("Unknown version type"));
+  case Version::V1: return DeserializeV1(bytes);
+  case Version::V2: return DeserializeV2(bytes);
+  default: ASSERT(false, ("Unknown version"));
   }
 
   return {};
@@ -175,9 +222,10 @@ std::string DebugPrint(local_ads::Version version)
 
   switch (version)
   {
-  case Version::unknown: return "Unknown";
-  case Version::v1: return "Version 1";
-  case Version::v2: return "Version 2";
+  case Version::Unknown: return "Unknown";
+  case Version::V1: return "Version 1";
+  case Version::V2: return "Version 2";
+  default: ASSERT(false, ("Unknown version"));
   }
 }
 }  // namespace local_ads
diff --git a/local_ads/campaign_serialization.hpp b/local_ads/campaign_serialization.hpp
index 167c0259a0..2434db8367 100644
--- a/local_ads/campaign_serialization.hpp
+++ b/local_ads/campaign_serialization.hpp
@@ -9,14 +9,14 @@ namespace local_ads
 {
 enum class Version
 {
-  unknown = -1,
+  Unknown = -1,
   // March 2017 (store feature ids and icon ids as varints, use one byte for days before
   // expiration).
-  v1 = 0,
+  V1 = 0,
   // August 2017 (store zoom level and priority as 0-7 values in one byte).
-  v2 = 1,
+  V2 = 1,
 
-  latest = v2
+  Latest = V2
 };
 std::vector<uint8_t> Serialize(std::vector<Campaign> const & campaigns, Version const version);
 std::vector<uint8_t> Serialize(std::vector<Campaign> const & campaigns);
diff --git a/local_ads/local_ads_tests/campaign_serialization_test.cpp b/local_ads/local_ads_tests/campaign_serialization_test.cpp
index 40a69017bd..4cec8f9ab0 100644
--- a/local_ads/local_ads_tests/campaign_serialization_test.cpp
+++ b/local_ads/local_ads_tests/campaign_serialization_test.cpp
@@ -36,8 +36,8 @@ std::vector<Campaign> GenerateCampaignsV1(size_t number)
 
 std::vector<Campaign> GenerateCampaignsV2(size_t number)
 {
-  std::random_device rd;
-  std::mt19937 gen(rd());
+  int kSeed = 42;
+  std::mt19937 gen(kSeed);
   std::uniform_int_distribution<> featureIds(1, 600000);
   std::uniform_int_distribution<> icons(1, 4096);
   std::uniform_int_distribution<> expirationDays(1, 30);
@@ -64,19 +64,19 @@ UNIT_TEST(Serialization_Smoke)
         {10, 10, 10},
         {1000, 100, 20},
         {120003, 456, 15}
-      }, Version::v1), ());
+      }, Version::V1), ());
 
   TEST(TestSerialization({
         {10, 10, 10, 10, 0},
         {1000, 100, 20, 17, 7},
         {120003, 456, 15, 13, 6}
-      }, Version::v2), ());
+      }, Version::V2), ());
 
-  TEST(TestSerialization(GenerateCampaignsV1(100), Version::v1), ());
-  TEST(TestSerialization(GenerateCampaignsV1(1000), Version::v1), ());
-  TEST(TestSerialization(GenerateCampaignsV1(10000), Version::v1), ());
+  TEST(TestSerialization(GenerateCampaignsV1(100), Version::V1), ());
+  TEST(TestSerialization(GenerateCampaignsV1(1000), Version::V1), ());
+  TEST(TestSerialization(GenerateCampaignsV1(10000), Version::V1), ());
 
-  TEST(TestSerialization(GenerateCampaignsV2(100), Version::v2), ());
-  TEST(TestSerialization(GenerateCampaignsV2(1000), Version::v2), ());
-  TEST(TestSerialization(GenerateCampaignsV2(10000), Version::v2), ());
+  TEST(TestSerialization(GenerateCampaignsV2(100), Version::V2), ());
+  TEST(TestSerialization(GenerateCampaignsV2(1000), Version::V2), ());
+  TEST(TestSerialization(GenerateCampaignsV2(10000), Version::V2), ());
 }
diff --git a/local_ads/pylocal_ads/bindings_test.py b/local_ads/pylocal_ads/bindings_test.py
index 4decdc314a..f183a99120 100644
--- a/local_ads/pylocal_ads/bindings_test.py
+++ b/local_ads/pylocal_ads/bindings_test.py
@@ -1,27 +1,22 @@
-from pylocal_ads import (Campaign, serialize, deserialize)
+import unittest
 
+from pylocal_ads import (Campaign, serialize, deserialize)
 
-def smoke():
-    campaigns = [
-        Campaign(10, 10, 10, 10, 0),
-        Campaign(1000, 100, 20, 17, 7),
-        Campaign(120003, 456, 15, 13, 6)
-    ]
 
-    serialized = serialize(campaigns)
-    result = deserialize(serialized)
+class PyLocalAdsTest(unittest.TestCase):
 
-    if campaigns.sort() == result.sort():
-        return True
+    def test_smoke(self):
+        campaigns = [
+            Campaign(10, 10, 10, 10, 0),
+            Campaign(1000, 100, 20, 17, 7),
+            Campaign(120003, 456, 15, 13, 6)
+        ]
 
-    return False
+        serialized = serialize(campaigns)
+        result = deserialize(serialized)
 
+        self.assertEqual(campaigns.sort(), result.sort())
 
-def main():
-    if smoke():
-        print "Smoke OK"
-    else:
-        print "Smoke FAIL"
 
 if __name__ == "__main__":
-    main()
+    unittest.main()
diff --git a/map/local_ads_manager.cpp b/map/local_ads_manager.cpp
index fd16295845..59c7dfe6da 100644
--- a/map/local_ads_manager.cpp
+++ b/map/local_ads_manager.cpp
@@ -79,7 +79,7 @@ std::string MakeCampaignDownloadingURL(MwmSet::MwmId const & mwmId)
     return {};
 
   std::ostringstream ss;
-  auto const campaignDataVersion = static_cast<uint32_t>(local_ads::Version::latest);
+  auto const campaignDataVersion = static_cast<uint32_t>(local_ads::Version::Latest);
   ss << kServerUrl << "/"
      << campaignDataVersion << "/"
      << mwmId.GetInfo()->GetVersion() << "/"