2
\$\begingroup\$

I'm interested in reducing the repetition in the doPost() method of my servlet. Specifically, I don't like the loop and error handling repetition. Any other refactoring advice will also be helpful.

protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
    Map params = req.getParameterMap();
    String[] versions = req.getParameterValues("versions");
    String[] projects = req.getParameterValues("projects");
    Map<String, Object> context = Maps.newHashMap();

    if(versions == null || versions.length == 0 || projects == null || projects.length == 0) {
        resp.setContentType("text/html;charset=utf-8");
        context = prepContext(context);
        ArrayList<String> error = new ArrayList<String>();
        error.add("Please select at least one version and product");
        context.put("errors", error);
        templateRenderer.render(UI_TEMPLATE, context, resp.getWriter());
        return;
    }

    ApplicationUser user = getCurrentUser();
    ArrayList<Message> errors = new ArrayList<Message>();
    String act = req.getParameter("action");

    if (act.equals("add")) {
        Collection<Version> versionsToAdd = new HashSet();
        for (String version : versions) {
            // get version object by id and put it into a collection or array
            versionsToAdd.add(versionManager.getVersion(Long.parseLong(version)));
        }
        for (String project : projects) {
            if(project != null) {
                for (Version version : versionsToAdd) {
                    if (version != null) {
                        VersionBuilder versionBuilder = versionService.newVersionBuilder();
                        versionBuilder = setVersionBuilder(versionBuilder, version, project);

                        VersionService.VersionBuilderValidationResult result = versionService.validateCreate(user, versionBuilder);
                        if (result.getErrorCollection().hasAnyErrors()) {
                            // If the validation fails, we re-render the edit page with the errors in the context
                            errors.addAll(processErrors(result.getErrorCollection(), version.getName(), project));

                        } else {
                            // If the validation passes, we perform the update then redirect the user back to the
                            // page with the list of issues
                            versionService.create(user, result);
                            errors.add(createMessage("Version was created successfully.", version.getName(), project, SUCCESS_STYLE_CLASS));
                        }
                    } else {
                        errors.add(createMessage(NULL_VERSION, "unknown", project, ERROR_STYLE_CLASS));
                    }
                }
            }
            else {
                errors.add(createMessage(NULL_PROJECT, "unknown", null, ERROR_STYLE_CLASS));
            }
        }
    }
    if (act.equals("archive")) {
        for (String project : projects) {
            if(project != null) {
                for (String version : versions) {
                    if (version != null) {
                        //get the corresponding version in the project
                        Version versionToArchive = versionManager.getVersion(Long.parseLong(project), versionManager.getVersion(Long.parseLong(version)).getName());
                        if (versionToArchive != null) {
                            VersionService.ArchiveVersionValidationResult result = versionService.validateArchiveVersion(user, versionToArchive);
                            if (result.getErrorCollection().hasAnyErrors()) {
                                errors.addAll(processErrors(result.getErrorCollection(), versionToArchive.getName(), project));
                            } else {
                                versionService.archiveVersion(result);
                                errors.add(createMessage("Version was archived successfully.", versionToArchive.getName(), project, SUCCESS_STYLE_CLASS));
                            }
                        }
                        else {
                            errors.add(createMessage(NULL_VERSION, "unknown", project, ERROR_STYLE_CLASS));
                        }
                    }
                    else {
                        errors.add(createMessage(NULL_VERSION, "unknown", project, ERROR_STYLE_CLASS));
                    }
                }
            }
            else {
                errors.add(createMessage(NULL_PROJECT, "unknown", null, ERROR_STYLE_CLASS));
            }
        }
    }
    if (act.equals("unarchive")) {
        for (String project : projects) {
            if(project != null) {
                for (String version : versions) {
                    if (version != null) {
                        //get the corresponding version in the project
                        Version versionToUnarchive = versionManager.getVersion(Long.parseLong(project), versionManager.getVersion(Long.parseLong(version)).getName());
                        if (versionToUnarchive != null) {
                            VersionService.ArchiveVersionValidationResult result = versionService.validateUnarchiveVersion(user, versionToUnarchive);
                            if (result.getErrorCollection().hasAnyErrors()) {
                                errors.addAll(processErrors(result.getErrorCollection(), versionToUnarchive.getName(), project));
                            } else {
                                versionService.unarchiveVersion(result);
                                errors.add(createMessage("Version was unarchived successfully.", versionToUnarchive.getName(), project, SUCCESS_STYLE_CLASS));
                            }
                        }
                        else {
                            errors.add(createMessage(NULL_VERSION, "unknown", project, ERROR_STYLE_CLASS));
                        }
                    }
                    else {
                        errors.add(createMessage(NULL_VERSION, "unknown", project, ERROR_STYLE_CLASS));
                    }
                }
            }
            else {
                errors.add(createMessage(NULL_PROJECT, "unknown", null, ERROR_STYLE_CLASS));
            }
        }
    }
    if (act.equals("delete")) {
        Collection<Version> versionsToDelete = new HashSet();
        for (String version : versions) {
            // get version object by id and put it into a collection so that we know which versions to delete even when they get deleted
            versionsToDelete.add(versionManager.getVersion(Long.parseLong(version)));
        }
        for (String project : projects) {
            if(project != null) {
                for (Version version : versionsToDelete) {
                    if (version != null) {
                        //get the corresponding version from the project
                        Version versionToDelete = versionManager.getVersion(Long.parseLong(project), version.getName());
                        //delete the version with validation
                        if (versionToDelete != null) {
                            JiraServiceContext jsc = new JiraServiceContextImpl(user);
                            DeleteVersionWithReplacementsParameterBuilder deleteVersionWithReplacementsParameterBuilder = versionService.createVersionDeletaAndReplaceParameters(versionToDelete);
                            DeleteVersionWithCustomFieldParameters deleteVersionWithCustomFieldParameters = deleteVersionWithReplacementsParameterBuilder.build();
                            ServiceResult result = versionService.deleteVersionAndSwap(jsc, deleteVersionWithCustomFieldParameters);
                            if (result.getErrorCollection().hasAnyErrors()) {
                                errors.addAll(processErrors(result.getErrorCollection(), version.getName(), project));
                            } else {
                                errors.add(createMessage("Version was deleted successfully.", version.getName(), project, SUCCESS_STYLE_CLASS));
                            }
                        }
                        else {
                            errors.add(createMessage(NULL_VERSION, "unknown", project, ERROR_STYLE_CLASS));
                        }
                    }
                    else {
                        errors.add(createMessage(NULL_VERSION, "unknown", project, ERROR_STYLE_CLASS));
                    }
                }
            }
            else {
                errors.add(createMessage(NULL_PROJECT, "unknown", null, ERROR_STYLE_CLASS));
            }
        }

    }
    if (!errors.isEmpty()) {
        // If there are any messages, we re-render the edit page with the "errors" in the context
        resp.setContentType("text/html;charset=utf-8");
        context = prepContext(context);
        context.put("errors", errors);
        templateRenderer.render(UI_TEMPLATE, context, resp.getWriter());
    } else {
        // If the there are no messages, we redirect the user back
        resp.sendRedirect("versioncontrol");
    }
}

You can view the full class code in my GitHub repository.

Edit:

Looks like null checks in the loop for versions and projects are no longer needed due to the other null checks. I think those were just artifacts from the initial work-in-progress logic that didn't have the other checks. Thanks for pointing it out, @mtj!

I think I should give a bit more background on what the module that I'm building is going to be used for. This is a JIRA plugin. JIRA is an SDLC tracking software. JIRA has entities called projects, which are used to group tasks under. Each project has a separate set of versions out-of-box, however, this is not always desirable. Sometimes its good to have the same versions in multiple projects, since multiple projects can be for the same product, but for a different major feature within that product. This is a very common scenario at my company. JIRA is not well optimized to handle this sort of use case and business requirement. It would require a lot of manual work to do it with out-of-box tools, which is not sustainable. This plugin aims to solve this problem by letting a JIRA Administrator to push versions to projects in bulk. The list of available versions is compiled from all unique versions in all JIRA projects. As an Admin performing this sort of version push, I wouldn't really want to modify which versions I've selected if the versions I'm pushing are already in one of the multiple projects I've selected. I just want to select which projects should have the selected set of versions, and let the module figure out what should be updated and what shouldn't to match my requirement. The use case is similar for the other actions: archive, unarchive, and delete.

\$\endgroup\$
1
\$\begingroup\$

First of all, cramming that all into a single method is not a good idea. Refactor the different actions into other methods (ideally with basically the same signature) and use a switch over the action:

switch(act) {
    case "add":
        // Note: renamed "errors" to "messages" as they are also used for success messages
        messages.addAll(processAdd(versions, projects));
        break;
    case "archive":
        messages.addAll(processArchive(versions, projects));
        break;
    ...
}

// methods like: return is the method specific messages. Might as well pass the errors list in, or whatever
List<Message> processWhatever(String[] versions, String[] projects)

Then, for the methods themselves. Basically you always do some outer loop checking projects != null, then an inner loop checking versions != null. If you can live with a different message order (errors first), I suggest you create a preprocessing method first, which will sanitize the input and emit the according warnings. Something along the lines of (attn: untested editor-only code)

// switching to collections while we are at it
List<String> sanitizedProjects = new ArrayList<>();
List<String> sanitizedVersions = new ArrayList<>();
messages.addAll(sanitizeInput(projects, versions, sanitizedProjects, sanitizedVersions));

...

private List<Message> sanitizeInput(
    String[] projects,
    String[] versions,
    List<String> sanitizedProjects,
    List<String> sanitizedVersions) {
    ... preprare myErrors list ....
    for(String project : projects) {
        if(projects != null) {
            sanitizedProjects.add(project);
            for(String version : versions) {
                if(version != null) {
                    sanitizedVersions.add(version);
                }
                else
                    ... emit error ...
            }
        }
        else
            ... emit error ...
    }
    return myErrors;
}

This way, you can do this once in the beginning, and in all further processing methods operate on sanitized input and be sure that there are no null pointers included.


Edit: As an afterthought: why do you waste so much code with null-checks anyway? This is a technical interface, so whoever calls this, should simply not send you invalid requests. Therefore, in real life, I'd simply add the condition "values must not be null" to the interface contract (if possible), then do a single validity check at the beginning of the method, and reject with status code "bad request" if there are nulls in the data.


Edit 2 (to long for a comment): Disclaimer: this is from experience (> 20 yrs) and not from some ready-to-link standard text. Probably you will find something supporting these claims, probably you'll also find something saying exactly the opposite.

Basically, you have to strategies on how you can cope with multi-action requests (like you have here).

If requested actions "A" and "B":

  1. you can perform every possible action and return a detailed result ("performed 'A', but 'B' was not possible due to...")
  2. you can validate the actions, and only perform the complete set, if everything was possible ("no action taken as 'B' is invalid")

The first variant is what your code aims to do. Perform what is possible, and detail the result. I consider this the second-best solution for the following reasons:

  • Violation of the law of least surprises. Everybody knows databases, everybody loves transactions. Everybody expects a process to either complete 100% or 0% with no other possibilities. (Note: "everybody" is probably not true.... ;-))
  • Possible breaking of user's agenda. If a user wants to perform A and B together, who says that he still wants to do A if B is not possible? Maybe he wants to do something totally different if confronted with that information.
  • Code complexity (the original reason for your question). Doing a first pass to check and a second pass to perform (if everything is valid) is much simpler to write and maintain, than trying to do as much as possible with best effort. (Note: the first pass does validation, so yes, always check your input! But if a request is only partially legal, don't process it.)
\$\endgroup\$
  • \$\begingroup\$ What do you mean by "technical interface"? There are 3 null checks per action: 1. check if the project input via the UI was not null 2. check that the version input via the UI was not null 3. check that the not-null input that was entered is still valid, since there are other ways for these actions to be performed in other parts of the system, so if someone deletes a version or project in those parts of the system without refreshing this interface, the processing won't be able to complete for that version or project. \$\endgroup\$ – Cohaven Apr 30 '18 at 13:17
  • \$\begingroup\$ So 3 kinda has to be there. 1 and 2 are already handled in the first validation done in doPost. I feel like there was another reason to do that check in the loop, but I will have to revisit the use cases to determine what it was. \$\endgroup\$ – Cohaven Apr 30 '18 at 13:31
  • \$\begingroup\$ Looks like checks 1 and 2 are no longer needed due to the other null checks. I think those were just artifacts from the initial work-in-progress logic that didn't have the other checks. Thanks for pointing it out! \$\endgroup\$ – Cohaven Apr 30 '18 at 14:27
  • \$\begingroup\$ @Cohaven By "technical interface" I meant to say: this is probably called by some other piece of software (e.g. a javascript client) and not called directly by a user. This means, somewhere there should be an interface definition which defines which calls are valid and which calls are not. In my experience, the most simple, clean, and maintainable software design is, that the caller ensures that the call is valid, and the callee simply validates or rejects instead of trying to process the valid parts of the call. This also fits better into eventual transactional control. \$\endgroup\$ – mtj May 1 '18 at 5:51
  • \$\begingroup\$ Could you please provide me with a link where I could read more about the interface you are talking about? I'm having trouble finding articles on this using the key terms in your response. Yes, the form contents are processed for nulls before being submitted to the servlet, however, I was taught that validation should happen both on the client side and the server side, to prevent security vulnerabilities, as JavaScript can be disabled on a page. The JS validation serves more to improve the user experience by giving immediate feedback, rather than to establish security or validity. \$\endgroup\$ – Cohaven May 1 '18 at 13:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.