5
\$\begingroup\$

Is there any way to simplify the following C# helper class to numerate a string for Json.Net?

internal static string CreateContact(string title)
{
    var createContact = new {Title = title};
    string output = JsonConvert.SerializeObject(createContact);
    return output;
}

[JsonProperty(PropertyName = "9")]
public string Title
{
    get
    {
        return _title;
    }
    set
    {
        _title = value;
        _title = GetTitleId(Title).ToString();
    }
}

public static int? GetTitleId(string title)
{
    //remove "."
    var titleRemoveDots = title.Replace(".", "");
    var titleLowerCase = titleRemoveDots.Trim().ToLower();
    switch(titleLowerCase)
    {
        case "dir":
            return 1;
        case "dr": case "doctor":
            return 4;
        case "mag": case "magister":
            return 5;
        case "ing":
            return 6;
        case "dipling": case "dipl":
            return 7;
        case "prof": case "professor":
            return 8;
        case "dkfm":
            return 9;
        case "prok":
            return 12;
        default:
            return null;
    }
}
\$\endgroup\$
  • \$\begingroup\$ How is the class named? Is there more to it? \$\endgroup\$ – Mathieu Guindon Mar 14 '14 at 17:26
  • 1
    \$\begingroup\$ I don't get what you're trying to do in the Title setter. First you're assigning value to _title, then you are assigning the result of GetTitleId to _value. And you're passing the Title property in the method call. Also, I would normally expect the value of a property to have the value I've just set. Finally, the Title property doesn't contain a Title, but contains a TitleId. \$\endgroup\$ – comecme Mar 15 '14 at 8:34
4
\$\begingroup\$

Just a quick point, regarding readability; the switch block would be better off written like this:

//remove "."
var titleRemoveDots = title.Replace(".", "");
var titleLowerCase = titleRemoveDots.Trim().ToLower();
switch(titleLowerCase)
{
    case "dir":
        return 1;
    case "dr": 
    case "doctor":
        return 4;
    case "mag": 
    case "magister":
        return 5;
    case "ing":
        return 6;
    case "dipling": 
    case "dipl":
        return 7;
    case "prof": 
    case "professor":
        return 8;
    case "dkfm":
        return 9;
    case "prok":
        return 12;
    default:
        return null;
}

The absence of break; between cases (/ the presence of return in all cases) can make it harder to refactor the switch block later. I think it would be better to separate the concept of figuring out the return value and that of returning a value:

int? result;

//remove "."
var titleRemoveDots = title.Replace(".", "");
var titleLowerCase = titleRemoveDots.Trim().ToLower();
switch(titleLowerCase)
{
    case "dir":
        result = 1;
        break;
    case "dr": 
    case "doctor":
        result = 4;
        break;
    case "mag": 
    case "magister":
        result = 5;
        break;
    case "ing":
        result = 6;
        break;
    case "dipling": 
    case "dipl":
        result = 7;
        break;
    case "prof": 
    case "professor":
        result = 8;
        break;
    case "dkfm":
        result = 9;
        break;
    case "prok":
        result = 12;
        break;
    default:
        break;
}

return result;

Now, call me a switch-hater, I'd probably end up with something like this:

public enum PersonTitle
{
    Director = 1,
    Doctor = 4,
    Magister = 5,
    Ingineer = 6,
    Dipling = 7,
    Professor = 8,
    Dkfm = 9, // todo: give readable name
    Prok = 12 // todo: give readable name
}

private static _titleIds = new Dictionary<string, PersonTitle> {
                             { "dir", PersonTitle.Director },
                             { "dr", PersonTitle.Doctor },
                             { "doctor", PersonTitle.Doctor },
                             { "mag", PersonTitle.Magister },
                             { "magister", PersonTitle.Magister },
                             { "ing", PersonTitle.Ingineer },
                             { "dipl", PersonTitle.Dipling },
                             { "dipling", PersonTitle.Dipling },
                             { "prof", PersonTitle.Professor },
                             { "professor", PersonTitle.Professor },
                             { "dkfm", PersonTitle.Dkfm },
                             { "prok", PersonTitle.Prok }
                         };

And then GetTitleId could look like this:

public static int? GetTitleId(string title)
{
    var cleanTitle = title.Replace(".", string.Empty).Trim().ToLower();
    PersonTitle result;

    if (_titleIds.TryGetValue(cleanTitle, out result))
    {
        return (int)result;
    }
    else
    {
        return null;
    }
}

One issue is that Title is actually a string representation of the int value, which can be allowed to be null - I might be mistaken, but it looks like the setter for Title would throw a NullReferenceException if/when that is the case, because it's calling ToString() on null:

[JsonProperty(PropertyName = "9")]
public string Title
{
    get
    {
        return _title;
    }
    set
    {
        _title = value;
        _title = GetTitleId(Title).ToString();
    }
}

I'd expect a null-check there:

 set
 {
     _title = value;
     _title = (GetTitleId(Title) ?? string.Empty).ToString();
 }
\$\endgroup\$
  • \$\begingroup\$ If anything this code is more complex than the code in my original question. \$\endgroup\$ – user2151345 Apr 9 '14 at 14:02
  • 1
    \$\begingroup\$ @user2151345 indeed, a switch block with hard-coded magic numbers and strings is simpler to code. I find the dictionary simpler to maintain. \$\endgroup\$ – Mathieu Guindon Apr 9 '14 at 14:11

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.