views:

42

answers:

2

I have an user interface that print user, and I wan't to have a filter by country. I made a classic <select /> element.

In JSP I have

<select id="country" onchange="return filter();">
    <option value="">...</option>
    <c:forEach var="country" items="${countries}">
        <option value="${country.id}"
            ${country.name}
        </option>
    </c:forEach>
</select>

The thing is some users doesn't have a country, so I need to deal with the 2 filters : - one that print all the user, no filter - one that print only users that doesn't have a country

So I'm wondering what is the best way to say to Java : "find me all users", and "find me all users who doesn't have a country".

I have some idea : if the countryId = 0, the server translate it to "users who doesn't have a country, and if countryId = null, the server translate it to "all users".

At the end, the DAO object will make a query like

public List<User> findByCountry(Integer countryId){

   query = "select * from users"
   if(countryId==0){
      query+= " where country_id is null"
   }
   else if(countryId==null){
      query += " where country_id = " + countryId;
   }
   return query results...
}

So is this ok, or is this ugly, or someone have a better pattern to do this ?

A: 

I think your approach is correct, but you might want to make any user WHERE CountryID=0 || CountryID IS NULL use the 'All Users' label, this way you can filter to see anyone without a country set. Then you can fix those users if desired/needed. But I think overall your solution is good.

BrianB
+1  A: 

I would actually come up with two DAO APIs:

public ... findAllUsers(...) {...}

public ... findAllUsersWithoutACountry(...) {...} 

The problem with your approach, in my honest opinion, is your API is not explicit due to the dynamic SQL code. It makes it harder for your coworker to understand your code. Second, these are 2 different tasks, while they are similar, it is best to come up with two explicit methods. This is much easier to unit test and you have less cyclomatic complexity because you have less control flow in the method. Further, the code is easier to understand because other developers don't need to wonder why you are testing the countryId against 0 or null which doesn't convey whole lot of meaningful message to them, other than it is just a quick hack to solve your current problem. 3 weeks down the road, and you will be wondering why you are testing against these weird values yourself. :)

limc