1
\$\begingroup\$

I have the block of code from my project service layer and I'm not happy with it. It seems overly complex and I would hate to have to calculate the cyclomatic complexity. This block is called in response to a web service request and it is supposed to process the request depending on the fields that are set. It needs to be able to handle the creation of a new contact list with supplied valid contacts, update an existing contact list with existing valid contacts and update an existing contact list with new valid contacts.

It doesn't really help that I am mixing non-transactional and transactional operations up here, but its kinda unavoidable, because creating new contacts can't occur within a transactional context (because Hibernate eats up memory like me at a bacon buffet), but updating contacts can occur within a transactional context, because the result set is constrained by the contact Id.

While this code works, it is very ugly. I'm looking for ways to reduce the number of inline'd if statements, as well as they very ugly error handling block at the bottom of the code.

My object structure is something like this:

A contact list has many contact attributes. Contact attributes consist of basically a row id, contact id and then a key/value pair representing a single piece of contact information. ContactDetails amalgamates all the contact attributes for a single contact id into a Map. We try to do as much work as possible with the map.

The web service request can pass in a ContactListId, contact list name, and an active flag. It also passes in a list containing a contact id and a Map that is analogous to the ContactDetails map for a single contact.

try {
        if (request.getContactListId() == null) {
            contactList = ContactListAdapter.convertWebRequestToContactList(request);
            contactService.createContactListNonTransactionally(contactList);
        }
        else {
            contactListId = Long.parseLong(request.getContactListId().toString());
            contactList = contactService.find(contactListId);
        }

        for (WsContactDetails webServiceContactDetails : request.getListOfContacts()
                .getContact()) {
            ContactDetails validContactDetails = ContactDetails
                    .createContactDetails(contactService
                            .validateContactDetails(webServiceContactDetails.getContactMap()));
            String contactId = webServiceContactDetails.getContactId();
            if (contactId.isEmpty()) {
                // Will create as many contacts as it can. Exceptions are caught and logger in
                // create method.
                String generatedContactId = contactService.saveContactMap(contactList,
                        validContactDetails.getDetails());
                validContactDetails.addAttribute(ContactColumn.CONTACT_ID.name(),
                        generatedContactId);
            }
            else {
                List<String> existingContactIdList = contactService
                        .getListOfDistinctContactIdForContactList(contactListId);
                if (existingContactIdList.contains(webServiceContactDetails.getContactId())) {
                    // Will be rolled back on exception because it is @Transactional
                    contactService.updateContactDetailsForContactInList(validContactDetails,
                            webServiceContactDetails.getContactId());
                }
                else {
                    throw new ElementDoesNotExistInListException(
                            ErrorCodePrefix.CNCT_,
                            ExceptionMessages.ELEMENT_NOT_IN_LIST.getCode(),
                            "The given contactId: "
                                    + contactId
                                    + " does not exist on the given contactList with contactListId: "
                                    + contactListId);
                }
            }
            validContactDetailsList.add(validContactDetails);
        }

        response = ContactListAdapter.convertContactListToWebResponse(contactList,
                validContactDetailsList);
    } catch(CompanyException e) {
        logger.error(e.getMessage(), e);
        response.setExceptionCode(e.getCode());
        response.setFailureMessage(e.getMessage());
    } catch(Exception e) {
        /*
         * Delete contact list if exception has been thrown and contact list was just created.
         * Check the initial request contactListId value, because the actual contact list will
         * have a contactListId set if it's been created correctly.
         */
        if (request.getContactListId() == null) {
            try {
                contactService.delete(contactList.getContactListId());
            } catch(ContactListNotDeletedException e1) {
                String exceptionMessage = e1.getMessage()
                        + " This contact list has been created by a web service call, "
                        + "but has not been deleted when an exception has been thrown."
                        + "The contactListId is: " + contactList.getContactListId();
                logger.error(exceptionMessage, e1);
                response.setExceptionCode(e1.getCode());
                response.setFailureMessage(exceptionMessage);
            }
        }
    }
    return response;
\$\endgroup\$
3
\$\begingroup\$

in my opinion, the problem with this code is well described by on of your first sentences:

It needs to be able to handle the creation of a new contact list with supplied valid contacts, update an existing contact list with existing valid contacts and update an existing contact list with new valid contacts.

Obviously it violates the Single responsibility principle because you handle different use cases in one method.

I started to refactor this code to extract some little functions (see below).

public class ContactListController {

private ContactService contactService;

private Object request;
private Object response;

public Object processContactLists(Object request, Object response) {

    this.request = request;
    this.response = response;

    try { handleRequest(); }
    catch(CompanyException e) { logErrorAndSetResponseValues(e); }
    catch(Exception e) { deleteContactListIfJustCreated(); }

    return this.response;
}

private void logErrorAndSetResponseValues(CompanyException e) {
    logger.error(e.getMessage(), e);
    response.setExceptionCode(e.getCode());
    response.setFailureMessage(e.getMessage());
}

private void handleRequest() throws ElementDoesNotExistInListException {
    if (request.getContactListId() == null) createContactList();
    else findContactListFromId();


    for (WsContactDetails wsContactDetail : request.getListOfContacts().getContact()) {
        handleContactDetail(wsContactDetail);
    }

    createContactListResponse();
}

private void createContactListResponse() {
    this.response = ContactListAdapter.convertContactListToWebResponse(contactList,
            validContactDetailsList);
}

private void handleContactDetail(WsContactDetails webServiceContactDetails) throws ElementDoesNotExistInListException {
    ContactDetails validContactDetails = getValidContactDetails(webServiceContactDetails);
    String contactId = webServiceContactDetails.getContactId();

    if (contactId.isEmpty()) {
        createContact(validContactDetails);
    }
    else {
        List<String> existingContactIdList = contactService.getListOfDistinctContactIdForContactList(contactListId);

        if (existingContactIdList.contains(webServiceContactDetails.getContactId())) {
            // Will be rolled back on exception because it is @Transactional
            contactService.updateContactDetailsForContactInList(validContactDetails,
                    webServiceContactDetails.getContactId());
        }
        else {
            throw createElementNotInListException(contactId);
        }
    }
    validContactDetailsList.add(validContactDetails);
}


private ContactDetails getValidContactDetails(WsContactDetails wsContactDetails) {
    return ContactDetails.createContactDetails(
            contactService.validateContactDetails(wsContactDetails.getContactMap())
    );
}


private void createContact(ContactDetails validContactDetails) {
    String generatedContactId = contactService.saveContactMap(contactList, validContactDetails.getDetails());

    validContactDetails.addAttribute(ContactColumn.CONTACT_ID.name(), generatedContactId);
}

private void findContactListFromId() {
    contactListId = Long.parseLong(request.getContactListId().toString());
    contactList = contactService.find(contactListId);
}

private void createContactList() {
    contactList = ContactListAdapter.convertWebRequestToContactList(request);
    contactService.createContactListNonTransactionally(contactList);
}


private void deleteContactListIfJustCreated() {

    if (request.getContactListId() == null) {
        try {
            contactService.delete(contactList.getContactListId());
        } catch(ContactListNotDeletedException e1) {
            contactListCouldNotBeDeleted(e1);
        }
    }
}



/////
///// Error Handling Methods (this is a single concern, which could be further separated
/////

private void contactListCouldNotBeDeleted(ContactListNotDeletedException e1) {
    String exceptionMessage = e1.getMessage()
            + " This contact list has been created by a web service call, "
            + "but has not been deleted when an exception has been thrown."
            + "The contactListId is: " + contactList.getContactListId();
    logger.error(exceptionMessage, e1);
    response.setExceptionCode(e1.getCode());
    response.setFailureMessage(exceptionMessage);
}


private ElementDoesNotExistInListException createElementNotInListException(String contactId) {
    return new ElementDoesNotExistInListException(
            ErrorCodePrefix.CNCT_,
            ExceptionMessages.ELEMENT_NOT_IN_LIST.getCode(),
            contactIdNotFoundInContactListMessage(contactId));
}

private String contactIdNotFoundInContactListMessage(String contactId) {
    return "The given contactId: " + contactId + " does not exist on the given contactList with contactListId: " + contactListId;
}

}

Unfortunately this hits a brick wall, because more than one concern are mixed together here:

  • request response handling
  • error handling
  • request parsing
  • business logic

All these parts could be in a separate class, but this would just be necessary because the different use-cases from above are all in one place (a http endpoint i would guess).

I would instead go another route and try to put the different use-cases to the api level. In terms of an RESTful HTTP Endpoint the following stuff could make sense:

  • POST /contactList (^= create a new contact list (with supplied valid contacts perhaps)
  • PUT /contactList/123 (^= update meta data of this list)
  • POST /contactList/123/contacts (^= create a new contact within the list 123)
  • PUT /contactList/123/contacts/456 (^= update contact 456 from the contact list 123)

Of course this leads to a situation, where your clients have differentiate the use-cases and take the necessary steps (like doing the correct http call) instead of calling doIt(request)and everything will be handled there. But i think this is the better situation because you would not create some kind of god object, which has to handle doIt.

\$\endgroup\$
  • \$\begingroup\$ Funny you should mention a RESTful HTTP endpoint, because this method has to handle that as well as a SOAP Web Service request. Hence my dilemma \$\endgroup\$ – JamesENL Jul 30 '14 at 0:43
  • \$\begingroup\$ so, than the situation even gets worse, i think. From what type are these request / response objects, because i thought that you code snippet showed some kind of controller code, that handles the corresponding http requests / responses directly. But as you say this code is used by two endpoints, this code seems more like a "service layer code". But then, why does is talks to request / response objects, because i see http / SOAP mediation on the endpoint / controller level. Nevertheless, i would stick on the approach to push the complexity to the api level. \$\endgroup\$ – Mario David Jul 30 '14 at 8:43
  • \$\begingroup\$ Hahaha yeah, basically I've created wrapper objects (request and response) which get created from XML/JSON depending on where the request comes from. It actually looks a lot cleaner just by separating things up into sub-methods and commenting about functionality. I'd much rather have all the complexity within this one class, so that on my endpoint/controller I can just call return handler.handleRequest(requestObject) and trust that the response is correct. \$\endgroup\$ – JamesENL Jul 30 '14 at 8:46

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.