Spring controller code to group information by date

Question

I'm using the Spring Framework in my web application and I'm curious as to whether my controller implementations are good or not.

I've used many Hashmaps to make a object that serves my purpose for grouping from list of objects as per provided date params

The following is my controller method, I am curious as to its quality, did I create this well?

@RequestMapping(value="/view/DSR", method=RequestMethod.GET)
public String showMyDSR(@RequestParam(value="viewBy", required = true, defaultValue = "Project")String viewBy,@RequestParam(value="fromDate", required = true, defaultValue = "None")String fromDate, 
        @RequestParam(value="toDate", required = true, defaultValue = "None")String toDate, HttpServletRequest request, ModelMap model) {

        User user = (User) SecurityContextHolder.getContext().getAuthentication().getPrincipal();

        if(!fromDate.equals("None") && !toDate.equals("None")){

            DateTimeFormatter dtf = DateTimeFormat.forPattern("yyyy-MMMM-dd");
            DateTime fd = dtf.parseDateTime(fromDate);
            DateTime td = dtf.parseDateTime(toDate);

            if(user.getRole() == SystemRole.ADMIN){

                //#Admin maps
                HashMap<Double, HashMap<HashMap<String, Double>, List<TaskHours>>> totalWithDateMapAdmin;
                HashMap<Date, HashMap<Double, HashMap<HashMap<String, Double>, List<TaskHours>>>> taskHoursMapAdmin = new HashMap<Date, HashMap<Double,HashMap<HashMap<String,Double>,List<TaskHours>>>>();
                HashMap<String, Double> groupingHours = new HashMap<String, Double>();
                HashMap<HashMap<String, Double>, List<TaskHours>> groupingWithTaskHoursMap;

                for (DateTime date = fd; date.isBefore(td.plusDays(1)); date = date.plusDays(1)){

                    /*list of all logged hours for date range*/
                    List<TaskHours> taskHours = taskHourService.findAlltaskHoursBetweenDates(DateTimeUtils.getDayStartString(date), DateTimeUtils.getDayEndString(date));
                    totalWithDateMapAdmin = new HashMap<Double, HashMap<HashMap<String,Double>,List<TaskHours>>>();
                    groupingWithTaskHoursMap = new HashMap<HashMap<String,Double>, List<TaskHours>>();

                    if(taskHours.size() > 0){
                        //filter for hours
                        taskHours.stream().filter(h -> h.getHours() > 0.0).map(th -> {
                            //set hours in time
                            th.setHoursInTime(DateTimeUtils.getHoursFromSecondsAsString(DateTimeUtils.getSecondsFromHours(th.getHours())));
                            //set total hours
                            th.setTotalHours(taskHourService.getTotalHours(th.getTask()));
                            return th;
                        }).collect(Collectors.toList());

                        if(viewBy.equals("Project")){

                            List<Project> projects = taskHours.stream().map(th -> {return th.getTask().getProject();}).distinct().collect(Collectors.toList());
                            List<TaskHours> projectsHours = new ArrayList<TaskHours>();

                            for (Project project : projects) {
                                groupingHours = new HashMap<String, Double>();
                                projectsHours = taskHours.stream().filter(th -> th.getTask().getProject().getId() == project.getId()).collect(Collectors.toList());
                                groupingHours.put(project.getName(), CommonUtils.formatDoubleToHours(projectsHours.stream().mapToDouble(ph -> ph.getHours()).sum()));
                                groupingWithTaskHoursMap.put(groupingHours, projectsHours);
                            }

                        }else{

                            List<User> users = taskHours.stream().map(th -> {return th.getTask().getAssignedTo();}).distinct().collect(Collectors.toList());
                            List<TaskHours> usersHours = new ArrayList<TaskHours>();

                            for (User u : users) {
                                groupingHours = new HashMap<String, Double>();
                                usersHours = taskHours.stream().filter(th -> th.getTask().getAssignedTo().getId() == u.getId()).collect(Collectors.toList());
                                groupingHours.put(u.getName(), CommonUtils.formatDoubleToHours(usersHours.stream().mapToDouble(ph -> ph.getHours()).sum()));
                                groupingWithTaskHoursMap.put(groupingHours, usersHours);
                            }
                        }

                        double totalHoursForDateGroup = CommonUtils.formatDoubleToHours(taskHours.stream().mapToDouble(th -> th.getHours()).sum());
                        totalWithDateMapAdmin.put(totalHoursForDateGroup, groupingWithTaskHoursMap);

                        taskHoursMapAdmin.put(date.toDate(), totalWithDateMapAdmin);

                    }else{
                        taskHoursMapAdmin.put(date.toDate(), null);
                    }
                }

                model.addAttribute("taskHoursMapAdminModel", taskHoursMapAdmin);

                model.addAttribute("fromDate", fromDate);
                model.addAttribute("toDate", toDate);
            }else{

                //#User maps
                HashMap<Date, HashMap<Double, List<TaskHours>>> taskHoursMap = new HashMap<Date, HashMap<Double, List<TaskHours>>>();
                HashMap<Double, List<TaskHours>> totalWithDateMap;

                for (DateTime date = fd; date.isBefore(td.plusDays(1)); date = date.plusDays(1)){
                    List<TaskHours> taskHours = taskHourService.findAlltaskHoursBetweenDatesAndUser(DateTimeUtils.getDayStartString(date), DateTimeUtils.getDayEndString(date), user.getId());
                    totalWithDateMap = new HashMap<Double, List<TaskHours>>();

                    if(taskHours.size() > 0){
                        taskHours.stream().filter(h -> h.getHours() > 0.0).map(th -> {
                            //set hours in time
                            th.setHoursInTime(DateTimeUtils.getHoursFromSecondsAsString(DateTimeUtils.getSecondsFromHours(th.getHours())));
                            //set total hours
                            th.setTotalHours(taskHourService.getTotalHours(th.getTask()));
                            return th;
                        }).collect(Collectors.toList());

                        double totalHoursForDateGroup = taskHours.stream().mapToDouble(th -> th.getHours()).sum();
                        totalWithDateMap.put(CommonUtils.formatDoubleToHours(totalHoursForDateGroup), taskHours);

                        taskHoursMap.put(date.toDate(), totalWithDateMap);

                    }else{
                        taskHoursMap.put(date.toDate(), null);
                    }
                }

                model.addAttribute("taskHoursMap", taskHoursMap);

                model.addAttribute("fromDate", fromDate);
                model.addAttribute("toDate", toDate);
            }
        }else{

            model.addAttribute("fromDate", "");
            model.addAttribute("toDate", "");
        }

        model.addAttribute("userId", user.getId());

        model.addAttribute("viewBy", viewBy);

        return "user/fragments/ViewDSR";

}

Answer 1

If you set params to required, then (null) params, that are missing in the request, will not be accepted (an exception will be thrown), but params with empty value will do. If you want to work with defaultValue, then you need to set required to false.

The default value to use as a fallback when the request parameter is not provided or has an empty value. @Spring

1. I believe at least this construction can be a bit refactored:

      if (!fromDate.equals("None") && !toDate.equals("None")){
   
       ...
            model.addAttribute("fromDate", fromDate);
            model.addAttribute("toDate", toDate);
       ... 
       }else{
       ...
            model.addAttribute("fromDate", fromDate);
            model.addAttribute("toDate", toDate);
       ...
       }else{
            model.addAttribute("fromDate", "");
            model.addAttribute("toDate", "");
       }
   

   Params are not required and defaultValue="":

   • null -> ""
   • "" -> ""

   if(!fromDate.isEmpty() && !toDate.isEmpty){
   
       ...
   }
   ...
   model.addAttribute("fromDate", fromDate);
   model.addAttribute("fromDate", fromDate);
   

2. Too much tautology. Define a small function. The length of a function in the ideal case should not be more than 20-25 lines.

At the first sight:

for (Project project : projects) {
   groupingHours = new HashMap<String, Double>();
   projectsHours = taskHours.stream().filter...

is identical to (just different names of list of tasks):

for (User u : users) {
      groupingHours = new HashMap<String, Double>();
      usersHours = taskHours.stream().filter...

Code for not Admin seems similar to a part of Admin. Maybe there can be refactored as well.

Some lambdas expressions can be put into one method etc.

And after that the controller method will be not so overloaded.

What about hashes -> difficult to say. I don't see the whole picture. But there is definitely a bunch of classes with internal container can be written, that will replace this code and make it more readable.