Reputation: 4573
I have a requirement to verify whether the postal code for a particular country is mandatory or not based on the countryid
supplied. Currently I'm doing this with a switch
statement but this code breaks the Open/Closed SOLID principle. I would like to know how to get rid of the switch
in this scenario.
public class PostCodeVerifyMandatory : IPostCodeVerifyMandatory {
public bool IsPostCodeRequired(int countryId, string region)
{
switch (countryId) {
case 1: //UK
case 12: //Australia
case 29: //Brazil
case 31: //Brunei
case 37: //Canada
case 56: //Denmark
case 105: //Japan
case 110: //South Korea
case 114: //Latvia
case 136: //Moldova
case 137: //Monaco
case 145: //Netherlands
case 165: //Poland
case 166: //Portugal
case 183: //Slovak Republic (Slovakia)
case 189: //Spain
case 196: //Sweden
case 197: //Switzerland
case 199: //Taiwan Region
case 213: //Ukraine
case 215: //USA
case 221: //Vietnam
return true;
case 232: //Ireland
return region == "Dublin";
default:
return false;
}
}
}
Upvotes: 3
Views: 6413
Reputation: 4156
Try this:
public class PostCodeVerifyMandatory : IPostCodeVerifyMandatory
{
public List<Func<int, string, bool>> Rules { get; private set; }
public PostCodeVerifyMandatory()
{
Rules = new List<Func<int, string, bool>>();
}
public bool IsPostCodeRequired(int countryId, string region)
{
if(Rules == null)
return false;
return (Rules.Any(r => r(countryId, region)));
}
}
You will have to load the ruleset, before using it:
var simpleCountries = new List<int>
{
1, // UK
12, // Australia
29, // Brazil
56, // Brunei
//..
//..
215, //USA
221 //Vietnam
};
var postCodeVerifier = new PostCodeVerifyMandatory();
// 1 rule for simple countries
postCodeVerifier.Rules.Add((id, region) => simpleCountries.Contains(id));
// Special rule for Ireland
postCodeVerifier.Rules.Add((id, region) => id == 232 && region.Equals("Dublin"));
var a = postCodeVerifier.IsPostCodeRequired(232, "Dublin");
or to make it fully data driven (using dictionary as example):
var countries = new Dictionary<int, string>
{
{ 1, null }, // UK
{ 12, null }, // Australia
{ 29, null }, // Brazil
{ 56, null }, // Brunei
//..
//..
{ 215, null }, //USA
{ 221, null }, //Vietnam
{ 232, "Dublin" } // Ireland
};
var postCodeVerifier = new PostCodeVerifyMandatory();
// 1 rule for all
postCodeVerifier.Rules.Add((id, region) =>
countries.ContainsKey(id) &&
(countries[id] ?? region) == region);
Upvotes: 1
Reputation: 14064
I'd probably follow that piece of advice from the c2 Wiki page "Switch statements smell" :
Using a database or TableOrientedProgramming is sometimes the appropriate "fix", not polymorphism. For example, store product classications are best handled in a database with many-to-many category tables, not case statements.
You could have something like :
public class Country
{
public List<Region> Regions { get; set; }
public bool IsPostCodeRequiredByDefault { get; set; }
}
public class Region
{
private bool? _isPostCodeRequired;
public Country Country { get; set; }
public bool IsPostCodeRequired
{
get { return _isPostCodeRequired ?? Country.IsPostCodeRequiredByDefault; }
}
}
Which also has the benefit of adressing a secondary "primitive obsession" smell by making region a first-class domain concept instead of just a string.
Upvotes: 1
Reputation: 8245
Your switch
statement effectively maps integers to booleans, with a default value of false
.
So in this case, I would simply create a Dictionary<int,bool>
with the appropriate values. Since the values are pretty much fixed, you could initialize it in the declaration:
Dictionary<int, bool> dict = new Dictionary<int, bool>() {
{ 1 /* UK */, true },
{ 12 /* Australia */, false }
...etc...
};
As @Nick points out, the case for Ireland means you'll still need some extra logic, so you'll want the Dictionary to be private
, and the answer accessible via your IsPostCodeRequired(int,strnig)
method.
EDIT:
It would probably be best to get these values from a database, as @JustinHarvey points out.
If you want to be very strict about the Open/Closed Principle, you could use the Strategy design pattern - you would create a separate ConcreteStrategy object for every country. If a new country were added, you would create a new ConcreteStrategy object for that country. That way you can add the logic for countries with special rules, without touching your original code.
However, the number of countries with special rules is probably very small, so unless you really cannot change the code once it's in production, this is over-engineering.
Upvotes: 3
Reputation: 1695
Maybe something like this:
private Dictionary<int, string> _dict;
protected Dictionary<int, string> CountryDictionary
{
get
{
if (_dict == null)
{
_dict = new Dictionary<int, string>();
_dict.Add(1, "UK");
_dict.Add(12, "Australia");
// and so on
}
return _dict;
}
}
public class PostCodeVerifyMandatory : IPostCodeVerifyMandatory
{
public bool IsPostCodeRequired(int countryId, string region)
{
return CountryDictionary.ContainsKey(countryId);
}
}
Upvotes: 1