From 4b045fdb730634e0ef28bad1c0733b6f363a32cc Mon Sep 17 00:00:00 2001
From: Alex Zolotarev <deathbaba@gmail.com>
Date: Mon, 3 Sep 2012 23:43:28 +0300
Subject: [PATCH] Changes according to code review

---
 map/bookmark.cpp                 | 38 ++++++++++++++++++--------------
 map/bookmark.hpp                 |  2 ++
 map/map_tests/bookmarks_test.cpp | 11 +++++++++
 3 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/map/bookmark.cpp b/map/bookmark.cpp
index eff1a87573..c080d0cf9d 100644
--- a/map/bookmark.cpp
+++ b/map/bookmark.cpp
@@ -146,10 +146,11 @@ namespace
     void CharData(string value)
     {
       strings::Trim(value);
-      if (m_tags.size() > 1 && !value.empty())
+      size_t const count = m_tags.size();
+      if (count > 1 && !value.empty())
       {
-        string const & currTag = m_tags.back();
-        string const & prevTag = *(m_tags.end() - 2);
+        string const & currTag = m_tags[count - 1];
+        string const & prevTag = m_tags[count - 2];
 
         if (currTag == "name")
         {
@@ -290,22 +291,25 @@ void BookmarkCategory::SaveToKML(ostream & s)
   s << kmlFooter;
 }
 
+string BookmarkCategory::GenerateUniqueFileName(const string & path, string name)
+{
+  // Remove not allowed symbols
+  char const illegalChars[] = ":/";
+  for (size_t i = 0; i < ARRAY_SIZE(illegalChars); ++i)
+    name.erase(std::remove(name.begin(), name.end(), illegalChars[i]), name.end());
+  if (name.empty())
+    name = "Bookmarks";
+  size_t counter = 1;
+  while (Platform::IsFileExistsByFullPath(path + name))
+    name.append(strings::to_string(counter++));
+  return path + name + ".kml";
+}
+
 bool BookmarkCategory::SaveToKMLFileAtPath(string const & path)
 {
   if (m_file.empty())
-  {
-    // Generate unique file name from category name
-    string newName = m_name + ".kml";
-    // Remove not allowed symbols
-    char const illegalChars[] = ":/";
-    for (size_t i = 0; i < ARRAY_SIZE(illegalChars); ++i)
-      newName.erase(std::remove(newName.begin(), newName.end(), illegalChars[i]), newName.end());
-    if (newName.empty())
-      newName = "Bookmarks";
-    while (Platform::IsFileExistsByFullPath(path + newName))
-      newName.append(strings::to_string(m_name.size()));
-    m_file = path + newName;
-  }
+    m_file = GenerateUniqueFileName(path, m_file);
+
   try
   {
     // @TODO On Windows UTF-8 file names are not supported.
@@ -314,7 +318,7 @@ bool BookmarkCategory::SaveToKMLFileAtPath(string const & path)
   }
   catch (std::exception const & e)
   {
-    LOG(LWARNING, ("Can't save bookmarks catebory", m_name, "to file", m_file));
+    LOG(LWARNING, ("Can't save bookmarks category", m_name, "to file", m_file));
     return false;
   }
   return true;
diff --git a/map/bookmark.hpp b/map/bookmark.hpp
index b529bd0797..3559aa9b5f 100644
--- a/map/bookmark.hpp
+++ b/map/bookmark.hpp
@@ -65,6 +65,8 @@ public:
   /// creates unique file name on first save and uses it every time
   /// @param[in] path directory name where to save
   bool SaveToKMLFileAtPath(string const & path);
+
+  static string GenerateUniqueFileName(const string & path, string name);
 };
 
 /// Non-const category is needed to "edit" bookmark (actually, re-add it)
diff --git a/map/map_tests/bookmarks_test.cpp b/map/map_tests/bookmarks_test.cpp
index ed62cba27f..9fa336eb73 100644
--- a/map/map_tests/bookmarks_test.cpp
+++ b/map/map_tests/bookmarks_test.cpp
@@ -206,3 +206,14 @@ UNIT_TEST(Bookmarks_AddressInfo)
   // assume that developers have English or Russian system language :)
   TEST(info.m_types[0] == "cafe" || info.m_types[0] == "кафе", (info.m_types[0]));
 }
+
+UNIT_TEST(Bookmarks_UniqueFileName)
+{
+  char const * FILENAME = "SomeUniqueFileName";
+  {
+    FileWriter file(FILENAME);
+    file.Write(FILENAME, strlen(FILENAME));
+  }
+  TEST_NOT_EQUAL(BookmarkCategory::GenerateUniqueFileName("", FILENAME), FILENAME, ());
+  FileWriter::DeleteFileX(FILENAME);
+}