There are various approach to this problem.
- Use while loop
- Use for loop with additional stop condition
- Use break key word.
Let first create a template before introduce our logic:
public long findLocationId(String locationName) throws SystemException
{
if(locationName == null) { //Here we cover first issue.
throw new IllegalArgumentException("THe locationName must not be null");
}
long locId = Long.MIN_VALUE; //We declare a default value that will be returned if none match found.
Collection<Location> locationList = getLocationList(); //The location can be read from another method so we are not binded to field.
if(locationList == null || locationList.isEmpty()) {
return locId; // Or throw an exception about invalid state.
}
//Place for the logic
return locId;
}
Typically when we do not know when we want to stop the iteration, it is a sign that we should start with while loop.
So lets try it.
Solution 1 - The while way.
Iterator<Location> iterator = locationList.iterator();
while(iterator.hasNext() && Long.MIN_VALUE != locId) {
Location location = iterator.next();
if(locationName.equalsIgnoreCase(location.getLocationName())) {
locId = location.getLocationId(); // This will change the locId, so second condition will be no longer true and loop will end.
}
}
The pros:
- It works
The cons:
- Leave the iterator
We should not leave iterators, as this is error prone. This lead us to next solution.
Solution 2 - We use the pattern for iterator instead of while.
for(Iterator<Location> iterator2 = locationList.iterator();iterator.hasNext() && Long.MIN_VALUE != locId;) {
Location location = iterator.next();
if(locationName.equalsIgnoreCase(location.getLocationName())) {
locId = location.getLocationId(); // This will change the locId, so second condition will be no longer true and loop will end.
}
}
Pros
- It works
Cons
- It complicated, we must thing about the stop, when reading this code.
As above solution is not easy to read is should be also removed.
Solution 3 - Why break is useful.
for(Location location : locationList) {
if(locationName.equalsIgnoreCase(location.getLocationName())) {
locId = location.getLocationId();
break;
}
}
Pros
- It works
- It readable
Cons
- None
Conclusion is that the code should be readable. Using break, we point that we found the match and we do not want to progress anymore.
Fine. But what about the case when location was found ?
OP example we return 1L. This is not the best choice as is very likely that this value could be used as ID.
In previous examples i have used the min value of long. This is acceptable for some cases but, still we need to validate the method result, and also document it.
The final solution present additional loop exit, that is return key word.
public long findLocationId(String locationName) throws SystemException
{
if(locationName == null) { //Here we cover fist issue.
throw new IllegalArgumentException("THe locationName must not be null");
}
Collection<Location> locationList = getLocationList(); //The location can be read from another method so we are not binded to field.
if(locationList == null) {
throw new IllegalStateException("THe location list was not initialized");
}
for(Location location : locationList) {
if(locationName.equalsIgnoreCase(location.getLocationName())) {
return location.getLocationId(); //We exit from the method.
}
}
throw new SystemException("Could not found location for name:" + locationName);
}
Additional note
In the example OP have location == findLoc.getLocationName(), the problem with this code is that we should not use == to compare objects types (details). As we deal with String class recommended method is to use method String#equals(Object) or 'String#equalsIgnoreCase(String)'. For this example i have used the second option.
