Should internal class methods return values or just modify instance variables?

Question

I am creating a query builder class that will help in constructing a query for mongodb from URL params. I have never done much object oriented programming, or designed classes for consumption by people other than myself, besides using basic language constructs and using django's built in Models.

So I have this QueryBuilder class

class QueryHelper():
    """
    Help abstract out the problem of querying over vastly
    different dataschemas.
    """

    def __init__(self, collection_name, field_name, params_dict):
        self.query_dict = {}
        self.params_dict = params_dict
        db = connection.get_db()
        self.collection = db[collection_name]

    def _build_query(self):
        # check params dict and build a mongo query
        pass

Now in _build_query I will be checking the params_dict and populating query_dict so as to pass it to mongo's find() function. In doing this I was just wondering if there was an absolute correct approach to as whether _build_query should return a dictionary or whether it should just modify self.query_dict. Since it is an internal method I would assume it is OK to just modify self.query_dict. Is there a right way (pythonic) way of approaching this? Is this just silly and not an important design decision? Any help is appreciated.

Answer 1

Returning a value is preferable as it allows you to keep all the attribute modifying in one place (__init__). Also, this makes it easier to extend the code later; suppose you want to override _build_query in a subclass, then the overriding method can just return a value, without needing to know which attribute to set. Here's an example:

class QueryHelper(object):
    def __init__(self, param, text):
        self._param = param
        self._query = self._build_query(text)

    def _build_query(self, text):
        return text + " and ham!"

class RefinedQueryHelper(QueryHelper):
    def _build_query(self, text):
        # no need to know how the query object is going to be used
        q = super(RefinedQueryHelper, self)._build_query()
        return q.replace("ham", "spam")

vs. the "setter version":

class QueryHelper(object):
    def __init__(self, param, text):
        self._param = param
        self._build_query(text)

    def _build_query(self, text):
        self._query = text + " and ham!"

class RefinedQueryHelper(QueryHelper):
    def _build_query(self, text):
        # what if we want to store the query in __query instead?
        # then we need to modify two classes...
        super(RefinedQueryHelper, self)._build_query()
        self._query = self._query.replace("ham", "spam")

If you do choose to set an attribute, you might want to call the method _set_query for clarity.