2
\$\begingroup\$

I'm a relatively new (~6 mos) developer, and I'm hoping for some feedback on my code style/structure. While I'm particularly interested in efficiency and maintainability, I'm open to feedback regarding anything that comes to mind from more experienced developers.

/**
 * Attempts to create new SavedSearch (eventual POST call) using current
 * search criteria. If searchName already exists in database, create does
 * NOT occur, and user is prompted.
 * 
 * @param event
 */
private void doCreateSavedSearch(ActionEvent event) {
    String THIS_METHOD_NAME = "doCreateSavedSearch";
    SavedSearch searchReturnedFromService = null;

    if (logger.isDebugEnabled()) {
        logger.debug(
                getClass(),
                THIS_METHOD_NAME,
                "Attempting to CREATE new SavedSearch | searchName="
                        + searchToSave.getSearchName());
    }
    try {
        searchReturnedFromService = savedSearchService.createSavedSearch(
                executionControlParameters, searchToSave);

        if (logger.isDebugEnabled()) {
            logger.debug(getClass(), THIS_METHOD_NAME, "searchReturnedFromService=["
                    + searchReturnedFromService + "]");
        }

        if (searchReturnedFromService == null) { // indicates error from service
            ProcessingException e = new ProcessingException(
                    "RestClientBaseSavedSearchService", "SavedSearchService", null,
                    "searchReturnedFromService=" + searchReturnedFromService);
            throw e;
        }

        else if (null == searchReturnedFromService.getSearchName()) { // indicates name
                                                                        // already exists
            if (logger.isDebugEnabled()) {
                logger.debug(
                        getClass(),
                        THIS_METHOD_NAME,
                        "SEARCH NOT CREATED | searchName="
                                + searchToSave.getSearchName()
                                + " already exists in database (searchReturnedFromService.searchId="
                                + searchReturnedFromService.getSearchId() + ")");
            }
            searchNameExistsInDatabase = true;
            doUniqueNameViolationActions(searchReturnedFromService, event);
        }

        else {
            showMessage(
                    "search_results.operation.msg.confirmation.save_search.messgsuccess",
                    "INFO");

            if (logger.isDebugEnabled()) {
                logger.debug(getClass(), THIS_METHOD_NAME,
                        "SEARCH SUCCESSFULLY CREATED for searchName="
                                + searchReturnedFromService.getSearchName());
            }
            cleanUp();
        }
    }
    catch (ResourceAlreadyExistsExcepltion e) {
        showMessage("error.error.message.default", "ERROR");
        logger.error(getClass(), THIS_METHOD_NAME
                + " | ResourceAlreadyExistsExcepltion - " + e.getMessage(), e);
        cleanUp();
    }
    catch (ProcessingException e) {
        showMessage("error.error.message.default", "ERROR");
        logger.error(getClass(), THIS_METHOD_NAME
                + " | ProcessingException - " + e.getMessage(), e);
        cleanUp();
    }
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

You're a bit inconsistent with how you handle large argument lists. The large argument lists also make your code unreadable (or at least hard to read). I have trouble discerning the parts where your code does actual work and the parts where checks and logging happens.

private void doCreateSavedSearch(ActionEvent event) {
    String THIS_METHOD_NAME = "doCreateSavedSearch";
    SavedSearch searchReturnedFromService = null;

    /*logging*/ if (logger.isDebugEnabled()) {
    /*logging*/     logger.debug(
    /*logging*/             getClass(),
    /*logging*/             THIS_METHOD_NAME,
    /*logging*/             "Attempting to CREATE new SavedSearch | searchName="
    /*logging*/                     + searchToSave.getSearchName());
    /*logging*/ }
    try {
        searchReturnedFromService = savedSearchService.createSavedSearch(
                executionControlParameters, searchToSave);

        /*logging*/ if (logger.isDebugEnabled()) {
        /*logging*/     logger.debug(getClass(), THIS_METHOD_NAME, "searchReturnedFromService=["
        /*logging*/             + searchReturnedFromService + "]");
        /*logging*/ }

        if (searchReturnedFromService == null) { // indicates error from service
            ProcessingException e = new ProcessingException(
                    "RestClientBaseSavedSearchService", "SavedSearchService", null,
                    "searchReturnedFromService=" + searchReturnedFromService);
            throw e;
        }

        else if (null == searchReturnedFromService.getSearchName()) { // indicates name
                                                                        // already exists
            /*logging*/ if (logger.isDebugEnabled()) {
            /*logging*/     logger.debug(
            /*logging*/             getClass(),
            /*logging*/             THIS_METHOD_NAME,
            /*logging*/             "SEARCH NOT CREATED | searchName="
            /*logging*/                     + searchToSave.getSearchName()
            /*logging*/                     + " already exists in database (searchReturnedFromService.searchId="
            /*logging*/                     + searchReturnedFromService.getSearchId() + ")");
            /*logging*/ }
            searchNameExistsInDatabase = true;
            doUniqueNameViolationActions(searchReturnedFromService, event);
        }

        else {
            showMessage(
                    "search_results.operation.msg.confirmation.save_search.messgsuccess",
                    "INFO");

            /*logging*/ if (logger.isDebugEnabled()) {
            /*logging*/     logger.debug(getClass(), THIS_METHOD_NAME,
            /*logging*/             "SEARCH SUCCESSFULLY CREATED for searchName="
            /*logging*/                     + searchReturnedFromService.getSearchName());
            /*logging*/ }
            cleanUp();
        }
    }
    catch (ResourceAlreadyExistsExcepltion e) {
        showMessage("error.error.message.default", "ERROR");
        logger.error(getClass(), THIS_METHOD_NAME
                + " | ResourceAlreadyExistsExcepltion - " + e.getMessage(), e);
        cleanUp();
    }
    catch (ProcessingException e) {
        showMessage("error.error.message.default", "ERROR");
        logger.error(getClass(), THIS_METHOD_NAME
                + " | ProcessingException - " + e.getMessage(), e);
        cleanUp();
    }
}

Let's remove the logging bits, just so I can see what I'm dealing with.

private void doCreateSavedSearch(ActionEvent event) {
    String THIS_METHOD_NAME = "doCreateSavedSearch";
    SavedSearch searchReturnedFromService = null;

    try {
        searchReturnedFromService = savedSearchService.createSavedSearch(
                executionControlParameters, searchToSave);

        if (searchReturnedFromService == null) { // indicates error from service
            ProcessingException e = new ProcessingException(
                    "RestClientBaseSavedSearchService", "SavedSearchService", null,
                    "searchReturnedFromService=" + searchReturnedFromService);
            throw e;
        }

        else if (null == searchReturnedFromService.getSearchName()) { // indicates name
                                                                        // already exists
            searchNameExistsInDatabase = true;
            doUniqueNameViolationActions(searchReturnedFromService, event);
        }

        else {
            showMessage(
                    "search_results.operation.msg.confirmation.save_search.messgsuccess",
                    "INFO");

            cleanUp();
        }
    }
    catch (ResourceAlreadyExistsExcepltion e) {
        showMessage("error.error.message.default", "ERROR");
        logger.error(getClass(), THIS_METHOD_NAME
                + " | ResourceAlreadyExistsExcepltion - " + e.getMessage(), e);
        cleanUp();
    }
    catch (ProcessingException e) {
        showMessage("error.error.message.default", "ERROR");
        logger.error(getClass(), THIS_METHOD_NAME
                + " | ProcessingException - " + e.getMessage(), e);
        cleanUp();
    }
}

We end up with... weirdly formatted code.

You use the following structure:

if (condition) {
    code
}

else if (condition) {
    code
}

Don't do that.

There's a couple schools of thought when it comes to braces {}...

//1
if (condition) {
   code
} else if (condition) {
   code
}

//2
if (condition) {
   code
} 
else if (condition) {
   code
}

//3
if (condition) 
{
   code
} 
else if (condition) 
{
   code
}

I suggest you pick one of these three. Right now, there's no way to see the if/elseif chain as connected, which makes the function hard to understand.

Moving on...

    logger.debug(
            getClass(),
            THIS_METHOD_NAME,
            "Attempting to CREATE new SavedSearch | searchName="
                    + searchToSave.getSearchName());

What happened here?

A single log call over 5 lines of code. It's drowning out your more important code!

Looking at all the log calls:

    logger.debug(
            getClass(),
            THIS_METHOD_NAME,
            "Attempting to CREATE new SavedSearch | searchName="
                    + searchToSave.getSearchName());

        logger.debug(getClass(), THIS_METHOD_NAME, "searchReturnedFromService=["
                + searchReturnedFromService + "]");

            logger.debug(
                    getClass(),
                    THIS_METHOD_NAME,
                    "SEARCH NOT CREATED | searchName="
                            + searchToSave.getSearchName()
                            + " already exists in database (searchReturnedFromService.searchId="
                            + searchReturnedFromService.getSearchId() + ")");

            logger.debug(getClass(), THIS_METHOD_NAME,
                    "SEARCH SUCCESSFULLY CREATED for searchName="
                            + searchReturnedFromService.getSearchName());

    logger.error(getClass(), THIS_METHOD_NAME
            + " | ResourceAlreadyExistsExcepltion - " + e.getMessage(), e);

    logger.error(getClass(), THIS_METHOD_NAME
            + " | ProcessingException - " + e.getMessage(), e);

You need to pass in getClass(), THIS_METHOD_NAME, and a message. The first two parameters are always the same.

I'd try making them less pronounced:

    logger.debug(getClass(), THIS_METHOD_NAME,
                  "Attempting to CREATE new SavedSearch | searchName=" + searchToSave.getSearchName()
                 );

Perhaps even going as far as to do this:

    String debugMessage = "Attempting to CREATE new SavedSearch | searchName=" + searchToSave.getSearchName();
    logger.debug(getClass(), THIS_METHOD_NAME, debugMessage);

        ProcessingException e = new ProcessingException(
                "RestClientBaseSavedSearchService", "SavedSearchService", null,
                "searchReturnedFromService=" + searchReturnedFromService);
        throw e;

You don't use e. Consider using throw new ProcessingException(... instead.


        showMessage(
                "search_results.operation.msg.confirmation.save_search.messgsuccess",
                "INFO");

Magic string "INFO", consider using an enum instead (see java.util.logging.Level).

Magic string "search_results.operation.msg.confirmation.save_search.messgsuccess", consider defining somewhere else (top of class). You WILL screw this up one day, and it will be really hard to spot it.


catch (ResourceAlreadyExistsExcepltion e) {
    showMessage("error.error.message.default", "ERROR");
    logger.error(getClass(), THIS_METHOD_NAME
            + " | ResourceAlreadyExistsExcepltion - " + e.getMessage(), e);
    cleanUp();
}
catch (ProcessingException e) {
    showMessage("error.error.message.default", "ERROR");
    logger.error(getClass(), THIS_METHOD_NAME
            + " | ProcessingException - " + e.getMessage(), e);
    cleanUp();
}

If you're gonna go with a specific yet generic exception handler, use multicatch:

catch (ResourceAlreadyExistsExcepltion | ProcessingException e) {
    showMessage("error.error.message.default", "ERROR");
    logger.error(getClass(), THIS_METHOD_NAME
            + " | " + e.getClass().getName() + " - " + e.getMessage(), e);
    cleanUp();
}

    catch (ResourceAlreadyExistsExcepltion e) {
        showMessage("error.error.message.default", "ERROR");
        logger.error(getClass(), THIS_METHOD_NAME
                + " | ResourceAlreadyExistsExcepltion - " + e.getMessage(), e);
        cleanUp();
    }

Typo; ResourceAlreadyExistsExcepltion.


Final result (not included: Enums for showMessage, Level for logging severities)

/**
 * Attempts to create new SavedSearch (eventual POST call) using current
 * search criteria. If searchName already exists in database, create does
 * NOT occur, and user is prompted.
 * 
 * @param event
 */
private void doCreateSavedSearch(ActionEvent event) {
    String THIS_METHOD_NAME = "doCreateSavedSearch";
    SavedSearch searchReturnedFromService = null;

    if (logger.isDebugEnabled()) {
        String debugMessage = "Attempting to CREATE new SavedSearch | searchName=" 
                               + searchToSave.getSearchName();
        logger.debug(getClass(), THIS_METHOD_NAME, debugMessage);
    }
    try {
        searchReturnedFromService = savedSearchService.createSavedSearch(
                executionControlParameters, searchToSave);

        if (logger.isDebugEnabled()) {
            String debugMessage = "searchReturnedFromService=["
                                   + searchReturnedFromService 
                                   + "]";
            logger.debug(getClass(), THIS_METHOD_NAME, debugMessage);
        }

        if (searchReturnedFromService == null) { // indicates error from service
            throw new ProcessingException(
                    "RestClientBaseSavedSearchService", "SavedSearchService", null,
                    "searchReturnedFromService=" + searchReturnedFromService);
        } 
        else if (null == searchReturnedFromService.getSearchName()) { // indicates name already exists
            if (logger.isDebugEnabled()) {
                String debugMessage = "SEARCH NOT CREATED | searchName="
                                        + searchToSave.getSearchName()
                                        + " already exists in database (searchReturnedFromService.searchId="
                                        + searchReturnedFromService.getSearchId() + ")";
                logger.debug(getClass(), THIS_METHOD_NAME, debugMessage);
            }
            searchNameExistsInDatabase = true;
            doUniqueNameViolationActions(searchReturnedFromService, event);
        } 
        else {
            showMessage(
                 "search_results.operation.msg.confirmation.save_search.messgsuccess",
                 "INFO");

            if (logger.isDebugEnabled()) {
                String debugMessage = "SEARCH SUCCESSFULLY CREATED for searchName="
                                       + searchReturnedFromService.getSearchName();
                logger.debug(getClass(), THIS_METHOD_NAME, debugMessage);
            }
            cleanUp();
        }
    }
    catch (ResourceAlreadyExistsException | ProcessingException e) {
        showMessage("error.error.message.default", "ERROR");
        logger.error(getClass(), THIS_METHOD_NAME
                + " | " + e.getClass().getName() + " - " + e.getMessage(), e);
        cleanUp();
    }
}

Final result without logging:

/**
 * Attempts to create new SavedSearch (eventual POST call) using current
 * search criteria. If searchName already exists in database, create does
 * NOT occur, and user is prompted.
 * 
 * @param event
 */
private void doCreateSavedSearch(ActionEvent event) {
    String THIS_METHOD_NAME = "doCreateSavedSearch";
    SavedSearch searchReturnedFromService = null;

    try {
        searchReturnedFromService = savedSearchService.createSavedSearch(
                executionControlParameters, searchToSave);

        if (searchReturnedFromService == null) { // indicates error from service
            throw new ProcessingException(
                    "RestClientBaseSavedSearchService", "SavedSearchService", null,
                    "searchReturnedFromService=" + searchReturnedFromService);
        } 
        else if (null == searchReturnedFromService.getSearchName()) { // indicates name already exists
            searchNameExistsInDatabase = true;
            doUniqueNameViolationActions(searchReturnedFromService, event);
        } 
        else {
            showMessage(
                 "search_results.operation.msg.confirmation.save_search.messgsuccess",
                 "INFO");

            cleanUp();
        }
    }
    catch (ResourceAlreadyExistsException | ProcessingException e) {
        showMessage("error.error.message.default", "ERROR");
        cleanUp();
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the thorough answer! It'll take a while to process/practice with all of the feedback, but a quick question in the meantime: I've been using Eclipse's autoformat feature - is there a way to personalize it to make the result more readable? \$\endgroup\$ Jan 26, 2015 at 14:26
  • \$\begingroup\$ @JeffLevine stackoverflow.com/questions/1392744/… \$\endgroup\$
    – Pimgd
    Jan 26, 2015 at 14:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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