1
\$\begingroup\$

I would like a class to store a default string, and also store translated values for that string. The results will be stored in json. I'm looking for a peer review on how this could be improved.

My biggest problem with this is to get the string value from the class you have to use the concrete type, using var will return the object.

Translatable String Class

namespace OpenPartsBook
{
    [Serializable()]
    public class TranslatableString
    {
        [JsonProperty]
        private string Text;
        public List<Translation> Translations { get; set; }

        public TranslatableString()
        {
            Translations = new List<Translation>();        
        }
        public TranslatableString(string value)
        {
            Translations = new List<Translation>();
            Text = value;
        }

        /// <summary>
        /// Get a translation for the string by walking up the translation hierarchy
        /// if the requested language is not found it returns the default
        /// uses ISO 3166/ISO 3166-2
        /// </summary>
        /// <param name="LanguageCode">string - language code</param>
        /// <returns>string</returns>
        public string ToTranslatedString(string LanguageCode)
        {
            string temp = String.Empty;

            int i = LanguageCode.Length;

            if (this.Translations.Any(t => t.LanguageCode.ToLower().Equals(LanguageCode.ToLower())))
            {
                temp = this.Translations.First(t => t.LanguageCode.ToLower().Equals(LanguageCode.ToLower())).Text;  
            }
            else
            {               

                while ((i = LanguageCode.LastIndexOf("-", i)) != -1)
                {
                    var code = LanguageCode.Substring(0, i).ToLower();

                    if (this.Translations.Any(t => t.LanguageCode.ToLower().Equals(code.ToLower())))
                    {
                        temp = this.Translations.First(t => t.LanguageCode.ToLower().Equals(code.ToLower())).Text;
                        break;
                    }

                    --i;                  
                }
            }

            if (String.IsNullOrEmpty(temp)) temp = Text;

            return temp;
        }

        public static implicit operator TranslatableString(string s)
        {
            return s == null ? null : new TranslatableString(s);
        }

        public static implicit operator string(TranslatableString s) { return s.ToString(); }

        public override string ToString() { return Text; }

    }
}

Translation Class

namespace OpenPartsBook
{
    [Serializable()]
    [DataContract]
    public class Translation
    {
        [DataMember(Name = "languageCode", IsRequired = true)]
        public string LanguageCode { get; set; }
        [DataMember(Name = "text", IsRequired = true)]
        public string Text { get; set; }

        public Translation()
        {

        }
    }
}

Resulting Json:

"Title": {
    "Text": "Radiator",
    "Translations": [
      {
        "languageCode": "es",
        "text": "radiador"
      },
      {
        "languageCode": "fr",
        "text": "radiateur"
      }
    ]
  }
\$\endgroup\$
  • \$\begingroup\$ In what context is your code meant to be used? In a dictionary application? Or as part of a localization system? \$\endgroup\$ – Pieter Witvoet Sep 18 '17 at 16:06
  • \$\begingroup\$ It would be used as part of a localization system for viewing parts manuals. \$\endgroup\$ – Kevin Sep 18 '17 at 17:09
3
\$\begingroup\$

Localization

In my experience, translations are often provided per language, so I would normally store them per language rather than per piece of text.

I also prefer to have a single 'localization' object, rather than many 'translatable strings'. It gives me a clear entry-point for loading translations into memory and it provides a simple interface to other code: a single object with a GetText(string languageCode, string key) method.

Most applications only use a single language most of the time, so you may not even need to load all translations into memory. A central localization object makes it easier to perform such optimizations.

Some localization systems use the default (often English) text as a lookup key. But lately I've been using 'hierarchical' keys instead, such as MainWindow.Menu.FileButton or Settings.DataSources.DataSourceName. It gives translators slightly more context, which makes it easier for them to provide more accurate translations. And sometimes, where the default language uses the same word, another language uses different words depending on context. The hierarchical aspect of these keys allows me to organize the translations somewhat, so it's easier to prevent duplicate entries and all that.

Other notes

  • I don't think implicit conversion operators are a good idea here. They only really make sense when you're converting between similar types, and only when there's no risk of data loss. That's not the case here:
    • Converting a string to a TranslatableString gives you a translatable string that doesn't contain any translations. I don't see why that is useful, since the whole point of this class is to give access to translations.
    • Converting a TranslatableString to a string gives you the default text, but you lose all the translations. Instead, I would turn that Text field into a property with a public getter.
  • TranslatableString only needs one constructor if you give the value parameter a default value: (string value = null). Also, C# 6.0 introduced auto-property initializers, so you can further reduce code repetition in constructors: public List<Translation> Translations { get; set; } = new List<Translation>();
  • Making that Translations property publicly settable is probably not a good idea. I assume you're doing it for the free serialization, but it also allows any other code to modify it.
  • It's good to see some documentation! Perhaps you could add a small example with some hierarchical language codes for those that haven't memorized their ISO standards?

The ToTranslatedString method

  • It's doing double work. if (Any()) item = First() has to find a matching item twice. item = FirstOrDefault(); if (item != null) only has to find the item once.
  • A dictionary that maps language codes to translations would be a more appropriate data structure here.
  • Rather than repeatedly calling ToLower(), obtain a lower-case code beforehand, and make sure that your translations only use lower-case codes internally.
  • The main lookup code is duplicated. Instead of an if/else construction and a loop, you only need a loop. You just need to generalize the language code after you've tried to find a match.
  • You'll probably want to ignore a translation immediately if it's empty, rather than checking that at the end. Currently your code won't try to find a more general translation in such cases.
  • GetTranslation(string languageCode) is probably a better name - To implies some kind of conversion.

An improved version could look somewhat like this:

private Dictionary<string, Translation> _translations = new Dictionary<string, Translation>();

public string GetTranslation(string languageCode)
{
    languageCode = languageCode.ToLower();
    while (true)
    {
        if (_translations.TryGetValue(languageCode, out Translation translation) && !string.IsNullOrEmpty(translation.Text))
            return translation.Text;

        // Try again with a more general language code, if possible:
        var dashIndex = languageCode.LastIndexOf('-');
        if (dashIndex == -1)
            break;

        languageCode = languageCode.Substring(0, dashIndex);
    }
    return Text;
}
\$\endgroup\$
  • \$\begingroup\$ Pieter - thanks for the detailed response. I will implement many of your suggestions in my code. \$\endgroup\$ – Kevin Sep 19 '17 at 14:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.