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();
}
Title
setter. First you're assigningvalue
to_title
, then you are assigning the result ofGetTitleId
to_value
. And you're passing theTitle
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. – comecme Mar 15 '14 at 8:34