Clean Code: Write Methods with No Side-effects

I was listening to Uncle Bob Martin’s lecture on YouTube last night. His “No side-effects” advice hit me and made me realize that I was doing something in my code that I shouldn’t be doing it that way.

I wrote a HibernateUtil utility class that unwraps Hibernate SessionFactory object from EntityManagerFactory object and sets it to a static member. It also takes care of opening and closing Hibernate Session objects. I’ve written this utility to avoid writing duplicated blocks of try/catch code responsible for opening a session, creating an Query object, running it to get the result, and finally closing the session throughout the DAO layer.

This is how it looks.

@Component
@Slf4j
public class HibernateUtil {

    @Autowired
    EntityManagerFactory entityManagerFactory;

    private static SessionFactory sessionFactory;

    @Bean
    public void setSessionFactory() {
        try {
            sessionFactory = entityManagerFactory.unwrap(SessionFactory.class);
        } catch (Exception e) {
            log.error(e.getMessage());
        }
    }

    public static List<?> runNativeQuery(String queryString) {
        Session session = null;
        Query query;
        List<?> list = null;

        try {
            session = sessionFactory.openSession();
            query = session.createNativeQuery(queryString);
            list = query.getResultList();
        } catch (Exception e) {
            log.error(e.getMessage());
        } finally {
            if (session != null && session.isOpen()) {
                session.close();
            }
        }

        return list;
    }
}

The runNativeQuery(String queryString) can take any SQL statement in String format, run it, and return the result for it. But it gets a little tricky when we need to set parameters to Hibernate’s Query object inside our DAO layer.

I have added a pair of methods to the HibernateUtil class to handle it. The first pair opens a Session, creates, and returns the Query object to the DAO layer. Now inside the DAO layer, we can set whatever parameters we want to the Query object. The second pair takes this Query object, runs it, and closes the Session.

public static Query openSessionAndGetQueryObject(String queryString) {
        Session session;
        Query query = null;

        try {
            session = sessionFactory.openSession();
            query = session.createNativeQuery(queryString);
        } catch (Exception e) {
            log.error(e.getMessage());
        }

        return query;
    }

    public static List<?> getResultListAndCloseSession(Query query) {
        List<?> list = null;
        Session session = null;

        try {
            list = query.getResultList();
            session = query.unwrap(Session.class);
        } catch (Exception e) {
            log.error(e.getMessage());
        } finally {
            if (session != null && session.isOpen()) {
                session.close();
            }
        }

        return list;
    }

Now let’s get back to Uncle Bob’s advice. He says that a method should not have a side-effect.

The classical definition of a side-effect is a change to the state of the system if you call a function and that function causes the system to change state then that function has a side-effect.

He also says that the side-effect methods come in pairs. And that’s exactly what I’ve written which should have been avoided.

The openSessionAndGetQueryObject(String queryString) method opens a Session and leaves it in that state. The getResultListAndCloseSession(Query query) method unwraps the already opened Session object from the Query object and later closes it. These two methods are causing the system to change state and thus they’ve got a side-effect.

On the other hand, the runNativeQuery(String queryString) has no side effects on the system since it is both opening and closing the Session.

I refactored the HibernateUtil class to get rid of the pair and their side effects. It looks like this now.

public static List<?> runNativeQueryAfterSettingQueryParameters(String queryString, Map<String, Object> parametersToSet) {
        Session session = null;
        Query query;
        List<?> list = null;

        try {
            session = sessionFactory.openSession();
            query = session.createNativeQuery(queryString);
            parametersToSet.forEach(query::setParameter);
            list = query.getResultList();
        } catch (Exception e) {
            log.error(e.getMessage());
        } finally {
            if (session != null && session.isOpen()) {
                session.close();
            }
        }

        return list;
    }

It takes a HashMap from the DAO layer where we put parameter names and their values. This utility method here itself sets those parameters to the Query object and returns the result. This has no side effects on our system since it is not leaving the Session in an open state.

Here is a method from the DAO layer that is using it.

public List<?> getActiveEnquiryIdsBySubStatus(DetailsRequest detailsRequest) {
        String queryString = "select id from cm_enquiry where " +
                "sub_status = :subStatus and current_status = :currentStatus";

        Map<String, Object> parametersToSet = new HashMap<>();
        parametersToSet.put("subStatus", detailsRequest.getColumnClicked().getOptions());
        parametersToSet.put("currentStatus", "F");
        return HibernateUtil.runNativeQueryAfterSettingQueryParameters(queryString, parametersToSet);
    }

This is much easier to understand and cleaner. It is also hard for a programmer to maintain and use methods that come in pairs. One can easily create a mess if one doesn’t call the pairs in proper order.

Thanks to Uncle Bob!