Cleanup codes with clean code principle

- Extract codes into functions to reduce function size.
- Apply early-return policy to reduce indentation depth.
- Integrate duplicate operation logic into one function.
- Remove unnecessary comments.
- Add blank lines for readability.

Change-Id: I5e9cca6e1854dbe781daf5099cc29a5103369698
Signed-off-by: Junghyun Yeon <jungh.yeon@samsung.com>

diff --git a/src/common/step/backup/step_backup_icons.cc b/src/common/step/backup/step_backup_icons.cc
index 6e664d2..5900669 100644 (file)
--- a/src/common/step/backup/step_backup_icons.cc
+++ b/src/common/step/backup/step_backup_icons.cc
@@ -22,37 +22,15 @@ namespace common_installer {
 namespace backup {
 
 Step::Status StepBackupIcons::process() {
-  // gather icon info
-  const char* extra_icon_path = getIconPath(context_->uid.get(),
+  extra_icon_path_ = getIconPath(context_->uid.get(),
       context_->is_readonly_package.get());
-  if (!extra_icon_path)
+  if (!extra_icon_path_)
     return Status::OK;
 
-  for (auto iter = bf::directory_iterator(extra_icon_path);
-      iter != bf::directory_iterator(); ++iter) {
-    if (!bf::is_regular_file(iter->path()))
-      continue;
-    for (application_x* app : GListRange<application_x*>(
-        context_->old_manifest_data.get()->application)) {
-      if (!app->icon)
-        continue;
-      bf::path filename = iter->path().filename();
-      filename.replace_extension();
-      std::string id = filename.string();
-      if (id == app->appid) {
-        bf::path icon_backup = GetBackupPathForIconFile(iter->path());
-        icons_.emplace_back(iter->path(), icon_backup);
-      }
-    }
-  }
+  GetIconInfo();
 
-  // backup
-  for (auto& pair : icons_) {
-    if (!MoveFile(pair.first, pair.second, true)) {
-      LOG(ERROR) << "Cannot create backup for icon: " << pair.first;
-      return Status::ICON_ERROR;
-    }
-  }
+  if (!BackupIcons())
+    return Status::ICON_ERROR;
 
   LOG(DEBUG) << "Icons backup created";
   return Status::OK;
@@ -61,6 +39,7 @@ Step::Status StepBackupIcons::process() {
 Step::Status StepBackupIcons::clean() {
   RemoveBackupIcons();
   LOG(DEBUG) << "Icons backup removed";
+
   return Status::OK;
 }
 
@@ -72,12 +51,49 @@ Step::Status StepBackupIcons::undo() {
     }
   }
   LOG(DEBUG) << "Icons reverted from backup";
+
   return Status::OK;
 }
 
-void StepBackupIcons::RemoveBackupIcons() {
+bool StepBackupIcons::BackupIcons() {
   for (auto& pair : icons_) {
+    if (!MoveFile(pair.first, pair.second, true)) {
+      LOG(ERROR) << "Cannot create backup for icon: " << pair.first;
+      return false;
+    }
+  }
+
+  return true;
+}
+
+void StepBackupIcons::RemoveBackupIcons() {
+  for (auto& pair : icons_)
     Remove(pair.second);
+}
+
+void StepBackupIcons::GetIconInfo() {
+  for (auto iter = bf::directory_iterator(extra_icon_path_);
+      iter != bf::directory_iterator(); ++iter) {
+    if (!bf::is_regular_file(iter->path()))
+      continue;
+
+    AddAppIconToList(iter->path());
+  }
+}
+
+void StepBackupIcons::AddAppIconToList(boost::filesystem::path path) {
+  for (application_x* app : GListRange<application_x*>(
+      context_->old_manifest_data.get()->application)) {
+    if (!app->icon)
+      continue;
+
+    bf::path filename = path.filename();
+    filename.replace_extension();
+    std::string id = filename.string();
+    if (id == app->appid) {
+      bf::path icon_backup = GetBackupPathForIconFile(path);
+      icons_.emplace_back(path, icon_backup);
+    }
   }
 }
 

diff --git a/src/common/step/backup/step_backup_icons.h b/src/common/step/backup/step_backup_icons.h
index 80e36f5..0e5f0c4 100644 (file)
--- a/src/common/step/backup/step_backup_icons.h
+++ b/src/common/step/backup/step_backup_icons.h
@@ -48,10 +48,14 @@ class StepBackupIcons : public Step {
   Status precheck() override { return Status::OK; }
 
  private:
+  bool BackupIcons();
   void RemoveBackupIcons();
+  void GetIconInfo();
+  void AddAppIconToList(boost::filesystem::path path);
 
   std::vector<std::pair<boost::filesystem::path, boost::filesystem::path>>
       icons_;
+  const char* extra_icon_path_;
 
   STEP_NAME(BackupIcons)
 };

diff --git a/src/common/step/backup/step_backup_manifest.cc b/src/common/step/backup/step_backup_manifest.cc
index ed09995..c9149f7 100644 (file)
--- a/src/common/step/backup/step_backup_manifest.cc
+++ b/src/common/step/backup/step_backup_manifest.cc
@@ -29,6 +29,7 @@ Step::Status StepBackupManifest::precheck() {
     LOG(ERROR) << "Xml manifest file does not exist";
     return Status::MANIFEST_NOT_FOUND;
   }
+
   return Status::OK;
 }
 
@@ -50,24 +51,27 @@ Step::Status StepBackupManifest::process() {
 Step::Status StepBackupManifest::clean() {
   LOG(DEBUG) << "Remove manifest backup";
   ci::Remove(context_->backup_xml_path.get());
+
   return Status::OK;
 }
 
 Step::Status StepBackupManifest::undo() {
-  if (bf::exists(context_->backup_xml_path.get())) {
-    bs::error_code error;
-    bf::remove(context_->xml_path.get(), error);
-    if (error) {
-      LOG(ERROR) << "Failed to remove newly generated xml file in revert";
-      return Status::MANIFEST_ERROR;
-    }
-    if (!MoveFile(context_->backup_xml_path.get(),
-        context_->xml_path.get())) {
-      LOG(ERROR) << "Failed to revert a content of xml manifest file";
-      return Status::MANIFEST_ERROR;
-    }
-    LOG(DEBUG) << "Manifest reverted from backup";
+  if (!bf::exists(context_->backup_xml_path.get()))
+    return Status::OK;
+
+  bs::error_code error;
+  bf::remove(context_->xml_path.get(), error);
+  if (error) {
+    LOG(ERROR) << "Failed to remove newly generated xml file in revert";
+    return Status::MANIFEST_ERROR;
   }
+  if (!MoveFile(context_->backup_xml_path.get(),
+      context_->xml_path.get())) {
+    LOG(ERROR) << "Failed to revert a content of xml manifest file";
+    return Status::MANIFEST_ERROR;
+  }
+  LOG(DEBUG) << "Manifest reverted from backup";
+
   return Status::OK;
 }
 

diff --git a/src/common/step/backup/step_copy_backup.cc b/src/common/step/backup/step_copy_backup.cc
index 73f8f8c..2c5f8ba 100644 (file)
--- a/src/common/step/backup/step_copy_backup.cc
+++ b/src/common/step/backup/step_copy_backup.cc
@@ -30,6 +30,7 @@ const char kSharedResPath[] = "shared/res";
 
 bool CheckFreeSpace(const bf::path& backup_path, const bf::path& shared_path) {
   int64_t shared_size = ci::GetDirectorySize(shared_path);
+
   if (!ci::CheckFreeSpaceAtPath(shared_size, backup_path))
     return false;
 
@@ -38,6 +39,7 @@ bool CheckFreeSpace(const bf::path& backup_path, const bf::path& shared_path) {
 
 bool CreateSharedRes(const bf::path& src, const bf::path& dst) {
   bs::error_code error;
+
   bf::create_directories(dst / kSharedResPath, error);
   if (error) {
     LOG(ERROR) << "Cannot create package directory";
@@ -52,6 +54,24 @@ bool CreateSharedRes(const bf::path& src, const bf::path& dst) {
   return true;
 }
 
+bool Move(const boost::filesystem::path& from,
+    const boost::filesystem::path& to,
+    common_installer::FSFlag flag = common_installer::FSFlag::FS_NONE) {
+  if (bf::is_directory(from)) {
+    if (!common_installer::MoveDir(from, to / from.filename(), flag)) {
+      LOG(ERROR) << "Failed to move directory: " << from;
+      return false;
+    }
+  } else {
+    if (!common_installer::MoveFile(from, to / from.filename())) {
+      LOG(ERROR) << "Fail to move file: " << from;
+      return false;
+    }
+  }
+
+  return true;
+}
+
 }  // namespace
 
 namespace common_installer {
@@ -62,6 +82,7 @@ Step::Status StepCopyBackup::precheck() {
     LOG(ERROR) << "pkgid attribute is empty";
     return Step::Status::PACKAGE_NOT_FOUND;
   }
+
   if (context_->root_application_path.get().empty()) {
     LOG(ERROR) << "root_application_path attribute is empty";
     return Step::Status::INVALID_VALUE;
@@ -85,9 +106,9 @@ Step::Status StepCopyBackup::precheck() {
 }
 
 Step::Status StepCopyBackup::process() {
-  // if backup file exists
   if (!CleanBackupDirectory())
     return Status::APP_DIR_ERROR;
+
   if (!Backup())
     return Status::APP_DIR_ERROR;
 
@@ -95,6 +116,7 @@ Step::Status StepCopyBackup::process() {
     return Status::APP_DIR_ERROR;
 
   RemoveContent();
+
   return Status::OK;
 }
 
@@ -112,19 +134,20 @@ Step::Status StepCopyBackup::undo() {
   if (context_->external_storage)
     context_->external_storage->Abort();
 
-  // if backup was created then restore files
-  if (bf::exists(backup_path_)) {
-    if (!RollbackApplicationDirectory()) {
-      LOG(ERROR) << "Failed to revert package directory";
-      return Status::APP_DIR_ERROR;
-    }
-    LOG(DEBUG) << "Application files reverted from backup";
+  if (!bf::exists(backup_path_))
+    return Status::OK;
+
+  if (!RollbackApplicationDirectory()) {
+    LOG(ERROR) << "Failed to revert package directory";
+    return Status::APP_DIR_ERROR;
   }
+
+  LOG(DEBUG) << "Application files reverted from backup";
+
   return Status::OK;
 }
 
 bool StepCopyBackup::Backup() {
-  // create backup directory
   bs::error_code error;
 
   if (!bf::exists(backup_path_)) {
@@ -157,28 +180,17 @@ bool StepCopyBackup::Backup() {
       }
     }
 
-    if (bf::is_directory(iter->path())) {
-      if (!MoveDir(iter->path(), backup_path_ / iter->path().filename())) {
-        LOG(ERROR) << "Fail to backup package directory of: " << iter->path();
-        return false;
-      }
-    } else {
-      if (!MoveFile(iter->path(), backup_path_ / iter->path().filename())) {
-        LOG(ERROR) << "Fail to backup package file: " << iter->path();
-        return false;
-      }
-    }
+    if (!Move(iter->path(), backup_path_))
+      return false;
   }
 
-  if (bf::exists(backup_path_ / kSharedResPath) &&
-      bf::exists(context_->unpacked_dir_path.get() / kSharedResPath))
+  if (ShouldBackupSharedRes()) {
     if (!CreateSharedRes(backup_path_, context_->GetPkgPath()))
       return false;
+  }
+
+  AddRecoveryInfo();
 
-  recovery::RecoveryFile* recovery_file =
-      context_->recovery_info.get().recovery_file.get();
-  recovery_file->set_backup_done(true);
-  recovery_file->WriteAndCommitFileContent();
   LOG(INFO) << "Old package context saved to: " << backup_path_;
   return true;
 }
@@ -191,24 +203,20 @@ bool StepCopyBackup::MoveMountPointContent(const boost::filesystem::path& from,
 
   for (bf::directory_iterator iter(from);
        iter != bf::directory_iterator(); ++iter) {
-    if (bf::is_directory(iter->path())) {
-      if (!MoveDir(iter->path(), to / iter->path().filename())) {
-        LOG(ERROR) << "Fail to backup package directory of: " << iter->path();
-        return false;
-      }
-    } else if (bf::is_symlink(symlink_status(iter->path()))) {
+    if (bf::is_symlink(symlink_status(iter->path()))) {
       bf::copy_symlink(iter->path(), to / iter->path().filename(), error);
       if (error) {
         LOG(ERROR) << "Failed to backup package symlink: " << iter->path();
         return false;
       }
     } else {
-      if (!MoveFile(iter->path(), to / iter->path().filename())) {
+      if (!Move(iter->path(), to)) {
         LOG(ERROR) << "Fail to backup package file: " << iter->path();
         return false;
       }
     }
   }
+
   return true;
 }
 
@@ -216,6 +224,7 @@ void StepCopyBackup::RemoveContent() {
   if (context_->request_type.get() == RequestType::Update &&
     !context_->external_storage && bf::exists(install_path_ / ".mmc")) {
     LOG(WARNING) << "Remove unnecessary files for external storage";
+
     bs::error_code error;
     bf::remove((install_path_ / ".mmc"), error);
     if (error)
@@ -232,22 +241,11 @@ bool StepCopyBackup::NewContent() {
     LOG(ERROR) << "Cannot create package directory";
     return false;
   }
+
   for (bf::directory_iterator iter(context_->unpacked_dir_path.get());
        iter != bf::directory_iterator(); ++iter) {
-    if (bf::is_directory(iter->path())) {
-      if (!MoveDir(iter->path(), install_path_ / iter->path().filename(),
-        FS_MERGE_SKIP)) {
-        LOG(ERROR) << "Fail to copy tmp dir: " << iter->path()
-                   << " to dst dir: " << install_path_;
-        return false;
-      }
-    } else {
-      if (!MoveFile(iter->path(), install_path_ / iter->path().filename())) {
-        LOG(ERROR) << "Fail to copy tmp dir: " << iter->path()
-                   << " to dst dir: " << install_path_;
-        return false;
-      }
-    }
+    if (!Move(iter->path(), install_path_, FS_MERGE_SKIP))
+      return false;
   }
 
   // If other application tries to access shared/res of package being installed,
@@ -297,5 +295,20 @@ bool StepCopyBackup::RollbackApplicationDirectory() {
   return true;
 }
 
+void StepCopyBackup::AddRecoveryInfo() {
+  recovery::RecoveryFile* recovery_file =
+      context_->recovery_info.get().recovery_file.get();
+  recovery_file->set_backup_done(true);
+  recovery_file->WriteAndCommitFileContent();
+}
+
+bool StepCopyBackup::ShouldBackupSharedRes() {
+  if (bf::exists(backup_path_ / kSharedResPath) &&
+      bf::exists(context_->unpacked_dir_path.get() / kSharedResPath))
+    return true;
+
+  return false;
+}
+
 }  // namespace backup
 }  // namespace common_installer

diff --git a/src/common/step/backup/step_copy_backup.h b/src/common/step/backup/step_copy_backup.h
index 6a637ee..ee5ab7a 100644 (file)
--- a/src/common/step/backup/step_copy_backup.h
+++ b/src/common/step/backup/step_copy_backup.h
@@ -60,6 +60,8 @@ class StepCopyBackup : public Step {
   bool RollbackApplicationDirectory();
   bool MoveMountPointContent(const boost::filesystem::path& from,
                    const boost::filesystem::path& to);
+  void AddRecoveryInfo();
+  bool ShouldBackupSharedRes();
 
   boost::filesystem::path install_path_;
   boost::filesystem::path backup_path_;

diff --git a/src/common/utils/file_util.cc b/src/common/utils/file_util.cc
index 60295cb..b965249 100644 (file)
--- a/src/common/utils/file_util.cc
+++ b/src/common/utils/file_util.cc
@@ -394,6 +394,7 @@ bool BackupDir(const boost::filesystem::path& src,
     const boost::filesystem::path& dst, const std::string& entry) {
   if (!bf::exists(src / entry))
     return true;
+
   if (!MoveDir(src / entry, dst / entry,
       FS_MERGE_OVERWRITE |
       FS_COMMIT_COPY_FILE |
@@ -409,7 +410,6 @@ int64_t GetUnpackedPackageSize(const bf::path& path) {
   int64_t size = 0;
   int64_t block_size = GetBlockSizeForPath(path);
 
-  // if failed to stat path
   if (block_size == -1)
     return -1;
 
@@ -448,7 +448,6 @@ int64_t GetUnpackedPackageSize(const bf::path& path) {
 int64_t GetDirectorySize(const boost::filesystem::path& path) {
   int64_t block_size = GetBlockSizeForPath(path);
 
-  // if failed to stat path
   if (block_size == -1)
     return -1;
 
@@ -471,6 +470,7 @@ bool CheckFreeSpaceAtPath(int64_t required_size,
     const boost::filesystem::path& target_location) {
   bs::error_code error;
   boost::filesystem::path root = target_location;
+
   while (!bf::exists(root) && root != root.root_path())
     root = root.parent_path();
 
@@ -478,6 +478,7 @@ bool CheckFreeSpaceAtPath(int64_t required_size,
     LOG(ERROR) << "No mount point for path: " << target_location;
     return false;
   }
+
   bf::space_info space_info = bf::space(root, error);
   if (error) {
     LOG(ERROR) << "Failed to get space_info: " << error.message();
@@ -508,9 +509,11 @@ boost::filesystem::path GenerateTemporaryPath(
   bf::path pattern = path;
   pattern += "-%%%%%%";
   bf::path tmp_path;
+
   do {
     tmp_path = boost::filesystem::unique_path(pattern);
   } while (boost::filesystem::exists(tmp_path));
+
   return tmp_path;
 }
 
@@ -560,7 +563,6 @@ bool ExtractToTmpDir(const char* zip_path, const bf::path& tmp_dir,
         std::string(raw_file_name_in_zip).find(filter_prefix) == 0) {
       bf::path filename_in_zip_path(raw_file_name_in_zip);
 
-      // prevent "directory climbing" attack
       if (HasDirectoryClimbing(filename_in_zip_path)) {
         LOG(ERROR) << "Relative path in widget in malformed";
         return false;
@@ -680,6 +682,7 @@ boost::filesystem::path MakeRelativePath(const boost::filesystem::path& input,
       LOG(ERROR) << base.string() << " is not base path for " << input.string();
       return input;
   }
+
   return input.string().substr(base.string().length() + 1);
 }
 
@@ -692,6 +695,7 @@ bool IsSubDir(const boost::filesystem::path& path,
     else
       p = p.parent_path();
   }
+
   return false;
 }
 
@@ -699,6 +703,7 @@ bf::path RelativePath(const bf::path& from,
                                      const bf::path& to) {
   bf::path::const_iterator itr_path = from.begin();
   bf::path::const_iterator itr_relative_to = to.begin();
+
   while (itr_path != from.end() && itr_relative_to != to.end() &&
          *itr_path == *itr_relative_to) {
     ++itr_path;