refactor(converter): optimize resource handling and entry conversion

WendellXY · WendellXY · commit d5d0086805ed · 2025-09-03T03:49:18.000+08:00
- Replaced `first()` with `into_iter().next()` for more efficient resource access in `convert_resources_to_format`.
- Updated plural and string entry handling to use `into_iter()` for improved performance and clarity.
- Enhanced the conversion logic in `From&lt;Resource&gt;` implementations to avoid unnecessary cloning, streamlining the entry creation process.
- Adjusted tests to reflect changes in resource handling and ensure correctness in entry conversions.
diff --git a/langcodec-cli/src/convert.rs b/langcodec-cli/src/convert.rs
@@ -509,11 +509,12 @@ pub fn read_resources_from_any_input(
                 });
 
             if let Some(lang) = lang_from_filename {
-                // Try with explicit format and language
+                let err_prefix = format!("Failed to read input with language '{}': ", lang);
+
                 let format_type = if input.ends_with(".strings") {
-                    langcodec::formats::FormatType::Strings(Some(lang.clone()))
+                    langcodec::formats::FormatType::Strings(Some(lang))
                 } else if input.ends_with(".xml") {
-                    langcodec::formats::FormatType::AndroidStrings(Some(lang.clone()))
+                    langcodec::formats::FormatType::AndroidStrings(Some(lang))
                 } else if input.ends_with(".xcstrings") {
                     langcodec::formats::FormatType::Xcstrings
                 } else if input.ends_with(".csv") {
@@ -525,10 +526,9 @@ pub fn read_resources_from_any_input(
                 };
 
                 let mut codec = Codec::new();
-                codec.read_file_by_type(input, format_type).map_err(|e2| {
-                    format!("Failed to read input with language '{}': {}", lang, e2)
-                })?;
-                return Ok(codec.resources);
+                codec
+                    .read_file_by_type(input, format_type)
+                    .map_err(|e2| format!("{err_prefix}{e2}"))?;
             } else {
                 eprintln!("Standard format detection failed: {}", e);
             }
diff --git a/langcodec/src/converter.rs b/langcodec/src/converter.rs
@@ -54,8 +54,8 @@ pub fn convert_resources_to_format(
 ) -> Result<(), Error> {
     match output_format {
         FormatType::AndroidStrings(_) => {
-            if let Some(resource) = resources.first() {
-                AndroidStringsFormat::from(resource.clone())
+            if let Some(resource) = resources.into_iter().next() {
+                AndroidStringsFormat::from(resource)
                     .write_to(Path::new(output_path))
                     .map_err(|e| {
                         Error::conversion_error(
@@ -70,8 +70,8 @@ pub fn convert_resources_to_format(
             }
         }
         FormatType::Strings(_) => {
-            if let Some(resource) = resources.first() {
-                StringsFormat::try_from(resource.clone())
+            if let Some(resource) = resources.into_iter().next() {
+                StringsFormat::try_from(resource)
                     .and_then(|f| f.write_to(Path::new(output_path)))
                     .map_err(|e| {
                         Error::conversion_error(
@@ -891,19 +891,19 @@ mod tests {
         // Read back as Android
         let android = crate::formats::AndroidStringsFormat::read_from(&output).unwrap();
         assert_eq!(android.plurals.len(), 1);
-        let p = &android.plurals[0];
+        let p = android.plurals.into_iter().next().unwrap();
         assert_eq!(p.name, "apples");
         // Should include at least 'one' and 'other'
         let mut qs: Vec<_> = p
             .items
-            .iter()
+            .into_iter()
             .map(|i| match i.quantity {
-                PluralCategory::One => ("one", i.value.clone()),
-                PluralCategory::Other => ("other", i.value.clone()),
-                PluralCategory::Zero => ("zero", i.value.clone()),
-                PluralCategory::Two => ("two", i.value.clone()),
-                PluralCategory::Few => ("few", i.value.clone()),
-                PluralCategory::Many => ("many", i.value.clone()),
+                PluralCategory::One => ("one", i.value),
+                PluralCategory::Other => ("other", i.value),
+                PluralCategory::Zero => ("zero", i.value),
+                PluralCategory::Two => ("two", i.value),
+                PluralCategory::Few => ("few", i.value),
+                PluralCategory::Many => ("many", i.value),
             })
             .collect();
         qs.sort_by(|a, b| a.0.cmp(b.0));
diff --git a/langcodec/src/formats/android_strings.rs b/langcodec/src/formats/android_strings.rs
@@ -131,22 +131,22 @@ impl From<Resource> for Format {
     fn from(value: Resource) -> Self {
         let mut strings = Vec::new();
         let mut plurals = Vec::new();
-        for entry in &value.entries {
-            match &entry.value {
-                Translation::Singular(_) => strings.push(StringResource::from_entry(entry)),
+        for entry in value.entries {
+            match entry.value {
+                Translation::Singular(_) => strings.push(StringResource::from_entry(&entry)),
                 Translation::Plural(p) => {
                     let mut items: Vec<PluralItem> = p
                         .forms
-                        .iter()
+                        .into_iter()
                         .map(|(cat, v)| PluralItem {
-                            quantity: cat.clone(),
-                            value: v.clone(),
+                            quantity: cat,
+                            value: v,
                         })
                         .collect();
                     // Ensure stable order later
                     items.sort_by(|a, b| a.quantity.cmp(&b.quantity));
                     plurals.push(PluralsResource {
-                        name: entry.id.clone(),
+                        name: entry.id,
                         items,
                         translatable: match entry.status {
                             EntryStatus::Translated => Some(true),
@@ -159,7 +159,7 @@ impl From<Resource> for Format {
         }
 
         Self {
-            language: value.metadata.language.clone(),
+            language: value.metadata.language,
             strings,
             plurals,
         }
@@ -168,13 +168,18 @@ impl From<Resource> for Format {
 
 impl From<Format> for Resource {
     fn from(value: Format) -> Self {
-        let mut entries: Vec<Entry> = value.strings.iter().map(StringResource::to_entry).collect();
+        let mut entries: Vec<Entry> = value
+            .strings
+            .into_iter()
+            .map(StringResource::into_entry)
+            .collect();
 
         // Convert plurals to entries
-        for pr in &value.plurals {
+        for pr in value.plurals {
             let mut forms = std::collections::BTreeMap::new();
-            for item in &pr.items {
-                forms.insert(item.quantity.clone(), item.value.clone());
+            for item in pr.items {
+                let PluralItem { quantity, value } = item;
+                forms.insert(quantity, value);
             }
             let all_empty = forms.values().all(|v| v.is_empty());
             let status = match pr.translatable {
@@ -190,10 +195,7 @@ impl From<Format> for Resource {
             };
             entries.push(Entry {
                 id: pr.name.clone(),
-                value: Translation::Plural(Plural {
-                    id: pr.name.clone(),
-                    forms,
-                }),
+                value: Translation::Plural(Plural { id: pr.name, forms }),
                 comment: None,
                 status,
                 custom: HashMap::new(),
@@ -202,7 +204,7 @@ impl From<Format> for Resource {
 
         Resource {
             metadata: Metadata {
-                language: value.language.clone(),
+                language: value.language,
                 domain: String::new(), // strings.xml does not have a domain
                 custom: HashMap::new(),
             },
@@ -219,15 +221,23 @@ pub struct StringResource {
 }
 
 impl StringResource {
-    fn to_entry(&self) -> Entry {
+    fn into_entry(self) -> Entry {
+        let StringResource {
+            name,
+            value,
+            translatable,
+        } = self;
+
+        let is_value_empty = value.is_empty();
+
         Entry {
-            id: self.name.clone(),
-            value: Translation::Singular(self.value.clone()),
+            id: name,
+            value: Translation::Singular(value),
             comment: None,
-            status: match self.translatable {
+            status: match translatable {
                 Some(true) => EntryStatus::Translated,
                 Some(false) => EntryStatus::DoNotTranslate,
-                None if self.value.is_empty() => EntryStatus::New,
+                None if is_value_empty => EntryStatus::New,
                 None => EntryStatus::Translated,
             },
             custom: HashMap::new(),
@@ -386,6 +396,7 @@ fn parse_plurals_resource<R: BufRead>(
 
 #[cfg(test)]
 mod tests {
+
     use super::*;
     use crate::traits::Parser;
     use crate::types::EntryStatus;
@@ -487,8 +498,9 @@ mod tests {
         </resources>
         "#;
         let format = Format::from_str(xml).unwrap();
-        assert_eq!(format.strings.len(), 1);
-        let entry = format.strings[0].to_entry();
+        let length = format.strings.len();
+        assert_eq!(length, 1);
+        let entry = format.strings.into_iter().next().unwrap().into_entry();
         assert_eq!(entry.status, EntryStatus::New);
     }
 
diff --git a/langcodec/src/formats/strings.rs b/langcodec/src/formats/strings.rs
@@ -110,7 +110,7 @@ impl From<Format> for Resource {
                 domain: String::from(""),
                 custom: HashMap::new(),
             },
-            entries: value.pairs.iter().map(Pair::to_entry).collect(),
+            entries: value.pairs.into_iter().map(Pair::into_entry).collect(),
         }
     }
 }
@@ -119,14 +119,12 @@ impl TryFrom<Resource> for Format {
     type Error = Error;
 
     fn try_from(value: Resource) -> Result<Self, Self::Error> {
-        let language = value.metadata.language.clone();
-
-        let pairs = value
-            .entries
-            .iter()
-            .map(|entry| Pair::try_from(entry.clone()))
+        let Resource { metadata, entries } = value;
+        let language = metadata.language;
+        let pairs = entries
+            .into_iter()
+            .map(Pair::try_from)
             .collect::<Result<Vec<_>, _>>()?;
-
         Ok(Format { language, pairs })
     }
 }
@@ -150,12 +148,20 @@ pub struct Pair {
 }
 
 impl Pair {
-    fn to_entry(&self) -> Entry {
+    fn into_entry(self) -> Entry {
+        let Pair {
+            key,
+            value,
+            comment,
+        } = self;
+
+        let is_pair_value_empty = value.is_empty();
+
         Entry {
-            id: self.key.clone(),
-            value: Translation::Singular(self.value.clone()),
-            comment: self.comment.clone(),
-            status: if self.value.is_empty() {
+            id: key,
+            value: Translation::Singular(value),
+            comment,
+            status: if is_pair_value_empty {
                 EntryStatus::New
             } else {
                 EntryStatus::Translated
@@ -687,7 +693,7 @@ mod tests {
         assert_eq!(pair.key, "empty");
         assert_eq!(pair.value, "");
         // Should be marked as New status in Entry
-        let entry = pair.to_entry();
+        let entry = pair.clone().into_entry();
         assert_eq!(entry.status, EntryStatus::New);
     }
 
diff --git a/langcodec/src/formats/xcstrings.rs b/langcodec/src/formats/xcstrings.rs
@@ -41,38 +41,47 @@ impl TryFrom<Vec<Resource>> for Format {
         let mut source_language = String::new();
         let mut version = String::new();
 
-        for resource in resources {
-            if let Some(meta_source_language) = resource.metadata.custom.get("source_language") {
-                // source_language should be modified if and only if it is empty, otherwise,
-                // it should throw an error if it differs.
-                if source_language.is_empty() {
-                    source_language = meta_source_language.clone();
-                } else if source_language != *meta_source_language {
+        for mut resource in resources {
+            // source_language
+            if source_language.is_empty() {
+                if let Some(v) = resource.metadata.custom.remove("source_language") {
+                    source_language = v; // moved, no clone
+                } else {
+                    return Err(Error::InvalidResource(
+                        "No source language found in metadata".into(),
+                    ));
+                }
+            } else if let Some(v) = resource.metadata.custom.get("source_language") {
+                if source_language != *v {
                     return Err(Error::DataMismatch(format!(
                         "Source language mismatch: expected {}, found {}",
-                        source_language, meta_source_language
+                        source_language, v
                     )));
                 }
             } else {
                 return Err(Error::InvalidResource(
-                    "No source language found in metadata".to_string(),
+                    "No source language found in metadata".into(),
                 ));
             }
 
-            if let Some(meta_version) = resource.metadata.custom.get("version") {
-                // version should be modified if and only if it is empty, otherwise,
-                // it should throw an error if it differs.
-                if version.is_empty() {
-                    version = meta_version.clone();
-                } else if version != *meta_version {
+            if version.is_empty() {
+                if let Some(v) = resource.metadata.custom.remove("version") {
+                    version = v; // move, no clone
+                } else {
+                    return Err(Error::InvalidResource(
+                        "No version found in metadata".into(),
+                    ));
+                }
+            } else if let Some(v) = resource.metadata.custom.get("version") {
+                if version != *v {
                     return Err(Error::DataMismatch(format!(
                         "Version mismatch: expected {}, found {}",
-                        version, meta_version
+                        version, v
                     )));
                 }
             } else {
                 return Err(Error::InvalidResource(
-                    "No version found in metadata".to_string(),
+                    "No version found in metadata".into(),
                 ));
             }
 
