How to delete last element in java.util.Set efficiently?

Question

List<Route> listOfRoutes = routeStopsService.getAllRoutes();
Set<String> listOfStopNames = new TreeSet<String>();
for(Route route:listOfRoutes){
 Set<Stop> stops = routeStopsService.getStops(route);
 for(Stop stop:stops)
 listOfStopNames .add(stop.getStopName());
 listOfStopNames .remove( ((TreeSet<String>) listOfSources).last());
}

Here I am getting list of stop names for every route, and I am removing the last stop name from every route.

• luketorjussen: can you guarantee that the last one is the one you want to remove? sets aren't necessarily ordered...

Answer 1

First of all, you should always use block statements with the for loops. The original formatting of the question was this:

for(Stop stop:stops)
 listOfStopNames .add(stop.getStopName());
 listOfStopNames .remove( ((TreeSet<String>) listOfSources).last());

It's hard to read. Readers could think that the remove call is part of the for loop since it's the same indetation level as the line before.

for (final Stop stop : stops) {
 listOfStopNames.add(stop.getStopName());
}
listOfStopNames.remove(listOfSources.last());

---

Hungartion notation is redundant, try to avoid it. A few new names:

• listOfRoutes -> routes
• listOfStopNames -> stopNames
• listOfSources -> sources

---

As @luketorjussen already mentioned, downcasting is nasty. If the implementation of listOfSources changes it may return another type and the compiler will not warn you but you'll get exceptions at runtime. At least change the casting to SortedSet instead of the TreeSet. TreeSet implements SortedSet and SortedSet has the required last method.

---

I'd extract out some method for better readability. For further review you should share some code from the routeStopsService, Router and Stop classes.

private Set<String> getStopNamesWithoutLast(final Route route) {
 final Set<String> result = getStopNames(route);
 final String lastStopName = ((SortedSet<String>) sources).last();
 result.remove(lastStopName);
 return result;
}
private Set<String> getStopNames(final Route route) {
 final Set<String> result = new TreeSet<String>();
 final Set<Stop> stops = routeStopsService.getStops(route);
 for (final Stop stop : stops) {
 final String stopName = stop.getStopName();
 result.add(stopName);
 }
 return result;
}

final List<Route> routes = routeStopsService.getAllRoutes();
final Set<String> allStopName = new TreeSet<String>();
for (final Route route : routes) {
 final Set<String> stopNames = getStopNamesWithoutLast(route);
 allStopName.addAll(stopNames);
}

Answer 2

It is kind of hard to tell by looking at your code, (you have not shown us the definition of listOfSources, and you appear to be doing some stuff which appears to have no purpose in the sample code you provided,) but it appears to me that your approach of removing the last item from the list is not inefficient. Do you have any reasons to believe that it could be made any more efficient?