Reputation: 18649
I have what seems to me a basic issue that confuses me.
Right now I have the following bad design (it seems to me it is bad design). I have a util class which looks something like this:
public class Countries
{
public boolean isCountryPresent ( String c )
{
//public static final http://en.wikipedia.org/wiki/ISO_3166-1
Set<String> myStrings = new HashSet<String>();
myStrings.add("us"); // Afghanistan
myStrings.add("af"); // Afghanistan
myStrings.add("dz"); // Algeria
myStrings.add("ao"); // Angola
myStrings.add("az"); // Azerbiajan
...
if ( myStrings.contains(c))
return true;
else
return false;
}
And then I check for the existance of the item like this:
Countries co = new Countries ( );
boolean isPresent = co.isCountryPresent( countryISOCode );
But I am thinking that instantiating the object each time wastes resources, no? Is there a more efficient way for doing this given that the country data does not need to be compiled more than once and does not change?
Upvotes: 1
Views: 191
Reputation: 133679
You can use an enum
or declare a single static instance of Countries which is initialized on startup, eg:
class Countries {
public final static Countries instance = new Countries();
private Set<String> myStrings;
private Countries() {
myStrings = new HashSet<String>();
myStrings.add("us");
..
}
}
Upvotes: 1
Reputation: 26078
I usually do something like this:
public class Countries
{
private static Set<String> myStrings = null
public static boolean isCountryPresent ( String c )
{
if (myStrings == null) {
myStrings = initializeSet();
}
if ( myStrings.contains(c))
return true;
else
return false;
}
private static Set<String> initializeSet()
{
//public static final http://en.wikipedia.org/wiki/ISO_3166-1
Set<String> countrySet = new HashSet<String>();
myStrings.add("us"); // Afghanistan
myStrings.add("af"); // Afghanistan
myStrings.add("dz"); // Algeria
myStrings.add("ao"); // Angola
myStrings.add("az"); // Azerbiajan
...
return countrySet;
}
}
This way it initializes the set the first time the method is called, but for all subsequent calls, the old initialization is cached.
You could also declare it in the constructor, but I tend to lean toward the lazy loading approach, so the application isn't waiting for all this stuff to load right when it launches, but is loaded sort of on demand when you actually need it.
Also as mentioned in the comments this should probably all be static, unless you are always reusing the same Countries
object throughout the code, otherwise it will be redone for every new Countries
object you instantiate.
Upvotes: 2
Reputation: 12795
The simplest transition is to move the set to a class variable, and instantiate it in the constructor:
public class Countries
{
private Set<String myStrings = new HashSet<String();
public Countries() {
//public static final http://en.wikipedia.org/wiki/ISO_3166-1
Set<String> myStrings = new HashSet<String>();
myStrings.add("us"); // Afghanistan
myStrings.add("af"); // Afghanistan
myStrings.add("dz"); // Algeria
myStrings.add("ao"); // Angola
myStrings.add("az"); // Azerbiajan
...
}
public boolean isCountryPresent ( String c )
{
return myStrings.contains(c);
}
}
That way it's just called once, when you first construct the Countries object.
Also note that you don't need to have an if
statement: contains
returns a boolean, so you can just return that straight.
Edit: just noticed your second code segment. Instead of creating a new Countries
object each time, just make it once, and persist it.
Alternatively, you could make this static (private static Set...
, static {
instead of public Countries() {
and public static boolean...
)
Then you just call it like Countries.isCountryPresent(SomeCountry);
without instantiating a new object.
Upvotes: 1