HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

The multilingual duck

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
multilingualtheduck

Problem

So, we have this "Rubberduck Settings" window, with a number of "tabs" dedicated to each configurable feature. The "root" tab contains the application's "general settings" and, in the next release, we've found something to put there:

The display language! Credits to @SimonAndréForsberg and @Vogel612, for the Swedish and German translations respectively.. I did the French translation myself.

The first thing I tried after Simon's pull request was merged, was of course trying to switch the display language to Swedish... and that blew up with a CultureNotFoundException.

I had to handle the case when a culture wasn't supported/installed on the user's system, so this is how I went about the DisplayLanguageSetting class, which gets serialized into the application's xml configuration file:

```
namespace Rubberduck.Settings
{
[XmlType(AnonymousType = true)]
public class DisplayLanguageSetting
{
[XmlAttribute]
public string Code { get; set; }

public DisplayLanguageSetting()
{
// serialization constructor
}

public DisplayLanguageSetting(string code)
{
Code = code;

CultureInfo culture;
try
{
culture = CultureInfo.GetCultureInfo(code);
_exists = true;
}
catch (CultureNotFoundException)
{
culture = RubberduckUI.Culture;
_exists = false;
}

_name = RubberduckUI.ResourceManager.GetString("Language_" + Code.Substring(0, 2).ToUpper(), culture);
}

private readonly string _name;
private readonly bool _exists;

[XmlIgnore]
public string Name { get { return _name; } }

[XmlIgnore]
public bool Exists { get { return _exists; } }

public override bool Equals(object obj)
{
var other = (DisplayLanguageSetting) obj;
return Code.Equals(other

Solution


  • I would group all of your properties together. Right now you have Code separated from the other properties by the ctors.



-
When overriding Equals, it's important to be sure that it can't throw an exception. The current implementation can throw an InvalidCastException.

public override bool Equals(object obj)
    {
        var other = (DisplayLanguageSetting) obj;
        return Code.Equals(other.Code);
    }


A proper implementation would look something more like this.

public override bool Equals(object obj)
{
    var other = obj as DisplayLanguageSetting;

    if (other == null)
        return false;

    return Code.Equals(other.Code);
}


-
I don't much care for the control either.

public partial class GeneralSettingsControl : UserControl


That's more or less all I need to see to know I don't care for it. How's that? Because that's all I need to know about the class to know that business logic is being put into the code behind instead of a presenter class. We've got a "smart" form on our hands here. Invert the control. This should be implementing an interface that exposes properties for the different view elements.

public interface IGeneralSettingsView
{
    DisplayLanguageSetting CurrentSetting {get; set;}
    IList {get; set;}
}


Note that once you do this, the class no longer needs a Configuration Service at all. (The presenter will still need it though).

Code Snippets

public override bool Equals(object obj)
    {
        var other = (DisplayLanguageSetting) obj;
        return Code.Equals(other.Code);
    }
public override bool Equals(object obj)
{
    var other = obj as DisplayLanguageSetting;

    if (other == null)
        return false;

    return Code.Equals(other.Code);
}
public partial class GeneralSettingsControl : UserControl
public interface IGeneralSettingsView
{
    DisplayLanguageSetting CurrentSetting {get; set;}
    IList<DisplayLanguageSetting> {get; set;}
}

Context

StackExchange Code Review Q#94107, answer score: 6

Revisions (0)

No revisions yet.