Rodrigo Rosenfeld Rosas

What did I learn about Code Writing?

Sun, 08 Jan 2012 23:57:00 +0000

I've being coding for about 2 decades now. And still I don't find it to be an exact science, as some would like to suppose. Otherwise, they wouldn't ask you for time estimates on feature requests or try to use tools like MS Project to manage a software project, as if Gantt charts could be useful for this kind of project.

Of course, I can completely understand the reasons for the ones willing to bring such project management tools to the software world. Good luck to them! But I won't talk about this subject in this article as it is too big. I would just like to state that software can be better understood when compared to sciences like Music or general Arts.

Both require lots of experience, personal feelings and are hard to estimate conclusion times since it is almost always something completely new. Although there are some recipes for certain kinds of music or movies, but then they are no longer art.

Some time ago I was asked to estimate how long it would take for me to implement a search system over some HTML documents taken from EDGAR filings. I'm pretty sure that this wouldn't be something new for some of you who have already had experience with search engines before, but that wasn't my case definitely. So, I knew I should research about tools like Lucene for search indexing, but I have never worked with them before. So how could I estimate this?

As I started following the tutorials, I thought the main problem was solved in the first 2 days, but I couldn't predict that I would spend so much time reading about the configuration files for Solr, and how search params could be adjusted. There is a lot of stuff to know about and configure for your needs.

Particularly, one of the curiosities I've noticed is that even if my configuration was set to enable AND-like search for all typed terms, if it happens for a user to prepend some word with a plus ("+") or minus ("-"), then non-prepended words would become optional. I had enabled the DisMax mode, by the way.

The challenge

So, I'd like to talk specifically about this specific challenge as it is a good example for demonstrating some techniques I've learned last year after reading Clean Code. Although being very Java-oriented, this book has a few simple rules that can be applied to every language and be really effective. Just like in Music and Movie Making, Software Writing is also a science in which there are lots of resources to learn from and that can be used in a systematic way. Learning those tools and techniques will help developers to deliver more in less time.

Developers should invest time on well-written code because they'll spend most of their time reading code. So, it makes sense to invest time and money on tools that will make it easier to browse some code as well as investing some time polishing their code so that they become more readable too.

Before talking about those simple rules, I'd like to show you how I might write this code in my early times. Don't waste your time trying to understand this code. Then, I'll show you the code that I've actually written in a couple of hours, exactly as I have estimated before, since it didn't have any external dependencies. So, basically, this is the trend:

Transform terms like 'some +required -not-allowed "any phrase" id:(10 or 20 or 30)' into '+some +required -not-allowed +"any phrase" +id:(10 or 20 or 30)'.

Pretty simple, right? But even software like this can be bug-prone. So, here is a poor implementation (in Groovy, as I'm a Grails programmer in my current job). Don't try to really understand it (more on this later), just take a look at the code (dis)organization. I didn't even try to compile it.

How not to code

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
class SolrService {
  ...
  private String processQuery(String query) {
    query = query.replaceAll('#', '')
    def expressions = [], matches
    while (matches = query =~ /\([^\(]*?\)/) {
      matches.each { match ->
        expressions << match
        query = query.replace(match, "#{${expressions.size()}}".toString())
      }
    }
    (query =~ /\".*?\"/).each { match ->
      expressions << match
      query = query.replace(match, "#{${expressions.size()}}".toString())
    }
    query = query.split(' ').findAll{it}.collect { word ->
      word[0] in ['-', '+'] ? word : "+${word}"
    }.join(' ')
    def s = expressions.size()
    expressions.reverse().eachWithIndex { expression, i ->
      query = query.replace("#{${s - i}}", expression)
    }
  }

  def search(query) {
    query = processQuery(query)
    ...
    return solrServer.request(new SolrQuery(query))
  }
}

Ok, I'll agree that for this specific case, the code may be not that bad, but although processQuery is not that big, you'll need some time for figuring it out what is happened if you're required to modify this method.

Also, looking at it, could you be sure it will work for all cases? Or could you tell me what is the reason for some specific line? What is this code protected from? How comfortable would you be if you were to modify this code? How would you write automated tests for processQuery?

Also, as the logic gets more complex, coding this way could led to some messy code like the one I've just taken from a file in the project that integrates Hibernate to Grails:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
// grails-core/grails-hibernate/src/main/groovy/grails/orm/HibernateCriteriaBuilder.java
// ...
@SuppressWarnings("rawtypes")
@Override
public Object invokeMethod(String name, Object obj) {
    Object[] args = obj.getClass().isArray() ? (Object[])obj : new Object[]{obj};

    if (paginationEnabledList && SET_RESULT_TRANSFORMER_CALL.equals(name) && args.length == 1 &&
            args[0] instanceof ResultTransformer) {
        resultTransformer = (ResultTransformer) args[0];
        return null;
    }

    if (isCriteriaConstructionMethod(name, args)) {
        if (criteria != null) {
            throwRuntimeException(new IllegalArgumentException("call to [" + name + "] not supported here"));
        }

        if (name.equals(GET_CALL)) {
            uniqueResult = true;
        }
        else if (name.equals(SCROLL_CALL)) {
            scroll = true;
        }
        else if (name.equals(COUNT_CALL)) {
            count = true;
        }
        else if (name.equals(LIST_DISTINCT_CALL)) {
            resultTransformer = CriteriaSpecification.DISTINCT_ROOT_ENTITY;
        }

        createCriteriaInstance();

        // Check for pagination params
        if (name.equals(LIST_CALL) && args.length == 2) {
            paginationEnabledList = true;
            orderEntries = new ArrayList<Order>();
            invokeClosureNode(args[1]);
        }
        else {
            invokeClosureNode(args[0]);
        }

        if (resultTransformer != null) {
            criteria.setResultTransformer(resultTransformer);
        }
        Object result;
        if (!uniqueResult) {
            if (scroll) {
                result = criteria.scroll();
            }
            else if (count) {
                criteria.setProjection(Projections.rowCount());
                result = criteria.uniqueResult();
            }
            else if (paginationEnabledList) {
                // Calculate how many results there are in total. This has been
                // moved to before the 'list()' invocation to avoid any "ORDER
                // BY" clause added by 'populateArgumentsForCriteria()', otherwise
                // an exception is thrown for non-string sort fields (GRAILS-2690).
                criteria.setFirstResult(0);
                criteria.setMaxResults(Integer.MAX_VALUE);

                // Restore the previous projection, add settings for the pagination parameters,
                // and then execute the query.
                if (projectionList != null && projectionList.getLength() > 0) {
                    criteria.setProjection(projectionList);
                } else {
                    criteria.setProjection(null);
                }
                for (Order orderEntry : orderEntries) {
                    criteria.addOrder(orderEntry);
                }
                if (resultTransformer == null) {
                    criteria.setResultTransformer(CriteriaSpecification.ROOT_ENTITY);
                }
                else if (paginationEnabledList) {
                    // relevant to GRAILS-5692
                    criteria.setResultTransformer(resultTransformer);
                }
                // GRAILS-7324 look if we already have association to sort by
                Map argMap = (Map)args[0];
                final String sort = (String) argMap.get(GrailsHibernateUtil.ARGUMENT_SORT);
                if (sort != null) {
                    boolean ignoreCase = true;
                    Object caseArg = argMap.get(GrailsHibernateUtil.ARGUMENT_IGNORE_CASE);
                    if (caseArg instanceof Boolean) {
                        ignoreCase = (Boolean) caseArg;
                    }
                    final String orderParam = (String) argMap.get(GrailsHibernateUtil.ARGUMENT_ORDER);
                    final String order = GrailsHibernateUtil.ORDER_DESC.equalsIgnoreCase(orderParam) ?
                            GrailsHibernateUtil.ORDER_DESC : GrailsHibernateUtil.ORDER_ASC;
                    int lastPropertyPos = sort.lastIndexOf('.');
                    String associationForOrdering = lastPropertyPos >= 0 ? sort.substring(0, lastPropertyPos) : null;
                    if (associationForOrdering != null && aliasMap.containsKey(associationForOrdering)) {
                        addOrder(criteria, aliasMap.get(associationForOrdering) + "." + sort.substring(lastPropertyPos + 1),
                                order, ignoreCase);
                        // remove sort from arguments map to exclude from default processing.
                        @SuppressWarnings("unchecked") Map argMap2 = new HashMap(argMap);
                        argMap2.remove(GrailsHibernateUtil.ARGUMENT_SORT);
                        argMap = argMap2;
                    }
                }
                GrailsHibernateUtil.populateArgumentsForCriteria(grailsApplication, targetClass, criteria, argMap);
                GrailsHibernateTemplate ght = new GrailsHibernateTemplate(sessionFactory, grailsApplication);
                PagedResultList pagedRes = new PagedResultList(ght, criteria);
                result = pagedRes;
            }
            else {
                result = criteria.list();
            }
        }
        else {
            result = GrailsHibernateUtil.unwrapIfProxy(criteria.uniqueResult());
        }
        if (!participate) {
            hibernateSession.close();
        }
        return result;
    }

    if (criteria == null) createCriteriaInstance();

    MetaMethod metaMethod = getMetaClass().getMetaMethod(name, args);
    if (metaMethod != null) {
        return metaMethod.invoke(this, args);
    }

    metaMethod = criteriaMetaClass.getMetaMethod(name, args);
    if (metaMethod != null) {
        return metaMethod.invoke(criteria, args);
    }
    metaMethod = criteriaMetaClass.getMetaMethod(GrailsClassUtils.getSetterName(name), args);
    if (metaMethod != null) {
        return metaMethod.invoke(criteria, args);
    }

    if (isAssociationQueryMethod(args) || isAssociationQueryWithJoinSpecificationMethod(args)) {
        final boolean hasMoreThanOneArg = args.length > 1;
        Object callable = hasMoreThanOneArg ? args[1] : args[0];
        int joinType = hasMoreThanOneArg ? (Integer)args[0] : CriteriaSpecification.INNER_JOIN;

        if (name.equals(AND) || name.equals(OR) || name.equals(NOT)) {
            if (criteria == null) {
                throwRuntimeException(new IllegalArgumentException("call to [" + name + "] not supported here"));
            }

            logicalExpressionStack.add(new LogicalExpression(name));
            invokeClosureNode(callable);

            LogicalExpression logicalExpression = logicalExpressionStack.remove(logicalExpressionStack.size()-1);
            addToCriteria(logicalExpression.toCriterion());

            return name;
        }

        if (name.equals(PROJECTIONS) && args.length == 1 && (args[0] instanceof Closure)) {
            if (criteria == null) {
                throwRuntimeException(new IllegalArgumentException("call to [" + name + "] not supported here"));
            }

            projectionList = Projections.projectionList();
            invokeClosureNode(callable);

            if (projectionList != null && projectionList.getLength() > 0) {
                criteria.setProjection(projectionList);
            }

            return name;
        }

        final PropertyDescriptor pd = BeanUtils.getPropertyDescriptor(targetClass, name);
        if (pd != null && pd.getReadMethod() != null) {
            ClassMetadata meta = sessionFactory.getClassMetadata(targetClass);
            Type type = meta.getPropertyType(name);
            if (type.isAssociationType()) {
                String otherSideEntityName =
                    ((AssociationType) type).getAssociatedEntityName((SessionFactoryImplementor) sessionFactory);
                Class oldTargetClass = targetClass;
                targetClass = sessionFactory.getClassMetadata(otherSideEntityName).getMappedClass(EntityMode.POJO);
                if (targetClass.equals(oldTargetClass) && !hasMoreThanOneArg) {
                    joinType = CriteriaSpecification.LEFT_JOIN; // default to left join if joining on the same table
                }
                associationStack.add(name);
                final String associationPath = getAssociationPath();
                createAliasIfNeccessary(name, associationPath,joinType);
                // the criteria within an association node are grouped with an implicit AND
                logicalExpressionStack.add(new LogicalExpression(AND));
                invokeClosureNode(callable);
                aliasStack.remove(aliasStack.size() - 1);
                if (!aliasInstanceStack.isEmpty()) {
                    aliasInstanceStack.remove(aliasInstanceStack.size() - 1);
                }
                LogicalExpression logicalExpression = logicalExpressionStack.remove(logicalExpressionStack.size()-1);
                if (!logicalExpression.args.isEmpty()) {
                    addToCriteria(logicalExpression.toCriterion());
                }
                associationStack.remove(associationStack.size()-1);
                targetClass = oldTargetClass;

                return name;
            }
        }
    }
    else if (args.length == 1 && args[0] != null) {
        if (criteria == null) {
            throwRuntimeException(new IllegalArgumentException("call to [" + name + "] not supported here"));
        }

        Object value = args[0];
        Criterion c = null;
        if (name.equals(ID_EQUALS)) {
            return eq("id", value);
        }

        if (name.equals(IS_NULL) ||
                name.equals(IS_NOT_NULL) ||
                name.equals(IS_EMPTY) ||
                name.equals(IS_NOT_EMPTY)) {
            if (!(value instanceof String)) {
                throwRuntimeException(new IllegalArgumentException("call to [" + name + "] with value [" +
                        value + "] requires a String value."));
            }
            String propertyName = calculatePropertyName((String)value);
            if (name.equals(IS_NULL)) {
                c = Restrictions.isNull(propertyName);
            }
            else if (name.equals(IS_NOT_NULL)) {
                c = Restrictions.isNotNull(propertyName);
            }
            else if (name.equals(IS_EMPTY)) {
                c = Restrictions.isEmpty(propertyName);
            }
            else if (name.equals(IS_NOT_EMPTY)) {
                c = Restrictions.isNotEmpty(propertyName);
            }
        }

        if (c != null) {
            return addToCriteria(c);
        }
    }

    throw new MissingMethodException(name, getClass(), args);
}
// ...

I do really hope never to have to understand such code... I'd be curious to find how would such an automated test be written for this invokeMethod, as I couldn't find the tests in this project.

What is wrong with this code?

Back to the original implementation, what would be wrong with such code?

  1. It takes a lot of time for understand the code (the read cost);
  2. It is hard to test;
  3. Parsing the query is too much responsibility the SolrService class;
  4. If you just need some way of indexing and searching results in stored documents, you shouldn't be relying in a specific solution, like Solr. If you decide later to change your Search solution from Solr to Elastic Search or using Lucene directly, you'll need to change a lot of code. It would be better to have a wrapper for something simple like this;
  5. It can become hard to change/maintain/debug;

Even if you try to split processQuery into smaller methods, you would be required to pass some common values over and over again, like query and the expressions array, that would not only be an in-parameter but would be an out-parameter too as it would have to be changed inside some methods... When that happens, it is a hint that the overall code needs a separate class for doing the job. This is one of the simple rules I've learned in Clean Code.

The simple rules of Clean Code

The top-bottom code writing approach

While reading the original example, the first thing you'll see is the processQuery method declared in the SolrService class. What does it do? Why do we need it? Who is using it? Only when we look forward, we'll be able to detect that it is being used from the search method.

I was always used to write code that way, writing the least dependent methods first and the higher level ones as the latest ones. I guess I thought they should be declared first before they could be mentioned. Maybe that was true for some procedural languages I've started with before my first experience with OOP while reading a book about C++.

But in all OO languages I know about, it is ok to declare your methods in any order. Writing them top down makes it easier for another reader to understand your code because he/she will read your high-level instructions first.

Avoid more than 5 lines in a single method

Keeping your methods really small will make it easier to understand them and to write unit tests against them too. They'll also be less error-prone.

Avoid methods with more than 2 or 3 parameters

Having lots of parameters in methods makes it really complicate to associate what is the meaning of each parameter. Looking at this code, could you understand what is the meaning of the last parameters?

1
request.setAction(ACTION.POST, true, true, 10, false)

You'd certainly have to checkout the API for AbstractUpdateRequest.

Avoid out-parameters

This is a typical example where you'd probably be better served by a separate class.

When you find out some situation where you'd like to return multiple values (I'm not talking about returning a single list, here) and you need some parameter for returning them (and out-parameter), you should reconsider if you're taking the right path.

Also, you should really try to avoid modifying any parameter as debugging such code can be really frustrating.

Provide good names for your variables, classes and methods

This one is gold. Good names are essential for a good code reading experience. It can save you several hours trying to understand some snippet of code.

Take a look at the signature of the invokeMethod method in the Grails-Hibernate integration example code:

1
Object invokeMethod(String name, Object obj)

Wouldn't it be easier to understand what it does if the signature was changed to this one?

1
2
3
4
Object invokeMethodWith(String methodName, Object methodArguments)

// code would look like (just supposing, I'm not sure):
criteria.invokeMethodWith("eq", [attributeName, expectedValue])

What does "obj" mean in the actual implementation? It could be anything with such generic description. Investing some time choosing good names for your methods and variables can save a lot of time from others trying to understand what the code does.

The result of applying such simple rules

Just by making use of those simple rules, you'll be able to:

  1. Easily read your code;
  2. Easily write your unit tests;
  3. Easily modify and evolve your logic;
  4. Have a well-documented code;
  5. Get rid of some otherwise hard-to-find bugs.

Some extra rules

Some rules I've being using for my entire life and I'm not sure if they are all documented in the Clean Code book or not. But I'd like to talk a bit about them too.

Don't use deep-depth blocks for handling validation rules

I've seen pseudo-code like this, so many times:

1
2
3
4
5
declare square_root(number) {
  if (number >= 0) {
    do_real_calculations_with(number)
  }
}

Often, there are even more validation rules inside each block and this style gets really hard to read. And, worse than that, it is only protecting the software from crashing or generating an unexpected exception, but it does not properly handle bad inputs (negative numbers).

Also, usually do_real_calculations_with(number) is written as pages of code in a way you won't be able to see the enclosing brackets of the block in a single page. Take a look again at the Hibernate-Grails integration code to see if you can easily find out where the block beginning at "if (isCriteriaConstructionMethod(name, args)) {" ends.

Even when you don't have to do anything if the necessary conditions are not met, I'd rather code this way:

1
2
3
4
declare square_root(number) {
  if (number < 0) return // or raise "Taking the square root of negative numbers is not supported by this implementation"
  do_real_calculations_with(number)
}

This is a real example found in PersistentManagerBase.java from the Tomcat project.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
protected void processMaxIdleSwaps() {

    if (!getState().isAvailable() || maxIdleSwap < 0)
        return;

    Session sessions[] = findSessions();
    long timeNow = System.currentTimeMillis();

    // Swap out all sessions idle longer than maxIdleSwap
    if (maxIdleSwap >= 0) {
        for (int i = 0; i < sessions.length; i++) {
            StandardSession session = (StandardSession) sessions[i];
            synchronized (session) {
                if (!session.isValid())
                    continue;
                int timeIdle = // Truncate, do not round up
                    (int) ((timeNow - session.getThisAccessedTime()) / 1000L);
                if (timeIdle > maxIdleSwap && timeIdle > minIdleSwap) {
                    if (session.accessCount != null &&
                            session.accessCount.get() > 0) {
                        // Session is currently being accessed - skip it
                        continue;
                    }
                    if (log.isDebugEnabled())
                        log.debug(sm.getString
                            ("persistentManager.swapMaxIdle",
                             session.getIdInternal(),
                             Integer.valueOf(timeIdle)));
                    try {
                        swapOut(session);
                    } catch (IOException e) {
                        // This is logged in writeSession()
                    }
                }
            }
        }
    }
}

It is hard to see what bracket is closing which bracket in the end... This could be rewritten as:

1
2
3
4
5
6
7
...
    if (maxIdleSwap < 0) return;
    for (int i = 0; i < sessions.length; i++) {
...
            if (timeIdle <= maxIdleSwap || timeIdle < minIdleSwap) continue;
            if (session.accessCount != null && session.accessCount.get() > 0) continue;
...

Simple code should be handled first

The pattern is:

1
2
3
4
if some_condition
  lots of lines of complex code handling here
else
  simple handling for the case where some_condition is false

Here is a concrete example taken from ActiveRecord::Explain.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
def logging_query_plan # :nodoc:
  threshold = auto_explain_threshold_in_seconds
  current   = Thread.current
  if threshold && current[:available_queries_for_explain].nil?
    begin
      queries = current[:available_queries_for_explain] = []
      start = Time.now
      result = yield
      logger.warn(exec_explain(queries)) if Time.now - start > threshold
      result
    ensure
      current[:available_queries_for_explain] = nil
    end
  else
    yield
  end
end

I would rather write such code as:

1
2
3
4
5
6
7
8
9
10
11
12
def logging_query_plan # :nodoc:
  threshold = auto_explain_threshold_in_seconds
  current   = Thread.current
  return yield unless threshold && current[:available_queries_for_explain].nil?
  queries = current[:available_queries_for_explain] = []
  start = Time.now
  result = yield
  logger.warn(exec_explain(queries)) if Time.now - start > threshold
  result
ensure
  current[:available_queries_for_explain] = nil
end

Of course, this isn't exactly the same as the original code in the case yield generates some exception for the "else" code, but I'm sure this could be worked around.

Don't handle separate exceptions when you don't need to

I've often found this pattern while reading Java code and I believe that is the result of using some Java IDE. The IDE will tell the developer that some exceptions were not handled and will automatically fill the code as:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
void myMethod() throws MyOwnException {
  try {
    someMethod()
  }
  catch(FileNotFoundException ex) {
    throw MyOwnException("File was not found")
  }
  catch(WrongPermissionException ex) {
    throw MyOwnException("You don't have the right permission to write to the file")
  }
  catch(CorruptFileException ex) {
    throw MyOwnException("The file is corrupted")
  }
  ...
}

If you're only interested in gracefully handle exceptions to give your user a better feedback, why doesn't you just write this instead:

1
2
3
4
5
6
7
8
void myMethod() throws MyOwnException {
  try {
    someMethod()
  } catch(Exception ex) {
    log.error("Couldn't perform XYZ action", ex)
    throw new MyOwnException("Sorry, couldn't perform XYZ action. Please contact our support team and we'll investigate this issue.")
  }
}

The original challenge actual implementation

And, finally, following those techniques, here is how I actually coded that original challenge and implemented the tests in JUnit:

1
2
3
4
5
6
7
8
class SearchService {
  ...
  def search(query) {
    query = new QueryProcessor(query).processedQuery
    ...
    new SearchResult(solrServer.request(new SolrQuery(query)))
  }
}

I'll omit the implementation of SearchResult class, as it is irrelevant to this specific challenge. I just want to point out that I've abstracted the search feature in some wrapper classes for not exposing Solr internals.

And here is the real implementation code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
package myappname.search

/* Solr behaves in an uncommon way:
 Even when configured for making an "AND" search, when a signal (+ or -)
 is prepended to any word, the ones that are not prepended are considered optionals.
 We don't want that, so we're prefixing all terms with a "+" unless they're already
 prefixed.
*/
class QueryProcessor {
  private query, expressions = [], words = []

  QueryProcessor(query) { this.query = query }

  def getProcessedQuery() {
    removeHashesFromQuery()
    extractParenthesis()
    extractQuotedText()
    splitWords()
    addPlusSignToUnsignedWords()
    joinProcessedWords()
    replaceExpressions()
    query
  }

  private removeHashesFromQuery() { query = query.replaceAll('#', '') }

  private extractParenthesis() {
    def matches = query =~ /\([^\(]*?\)/
    if (!matches) return
    replaceMatches(matches)
    // keep trying in case of nested parenthesis
    extractParenthesis()
  }

  private replaceMatches(matches) {
    matches.each {
      expressions << it
      query = query.replace(it, "#{${expressions.size()}}".toString())
    }
  }

  private extractQuotedText() {
    replaceMatches(query =~ /\".*?\"/)
  }

  private splitWords() {
    words = query.split(' ').findAll{it}
  }

  private addPlusSignToUnsignedWords() {
    words = words.collect { word ->
      word[0] in ['-', '+'] ? word : "+${word}"
    }
  }

  private joinProcessedWords() { query = words.join(' ') }

  private replaceExpressions() {
    def s = expressions.size()
    expressions.reverse().eachWithIndex { expression, i ->
      query = query.replace("#{${s - i}}", expression)
    }
  }
}

And the unit tests:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
package myappname.search

import org.junit.*

class QueryProcessorTests {
  @Test
  void removeHashesFromQuery() {
    def p = new QueryProcessor('some#hashes # in # query')
    p.removeHashesFromQuery()
    assert p.query == 'somehashes  in  query'
  }

  @Test
  void extractParenthesis() {
    def p = new QueryProcessor('(abc (cde fgh)) no parenthesis transaction_id:(ijk) (lmn)')
    p.extractParenthesis()
    assert p.query == '#{4} no parenthesis transaction_id:#{2} #{3}'
    assert p.expressions == ['(cde fgh)', '(ijk)', '(lmn)', '(abc #{1})']
  }

  @Test
  void extractQuotedText() {
    def p = new QueryProcessor('some "quoted" text and "some more"')
    p.extractQuotedText()
    assert p.query == 'some #{1} text and #{2}'
    assert p.expressions == ['"quoted"', '"some more"']
  }

  @Test
  void splitWords() {
    def p = new QueryProcessor('some #{1}   text and  id:#{2}  ')
    p.splitWords()
    assert p.words == ['some', '#{1}', 'text', 'and', 'id:#{2}']
  }

  @Test
  void addPlusSignToUnsignedWords() {
    def p = new QueryProcessor('some #{1}   -text and  id:#{2}    +text  ')
    p.splitWords()
    p.addPlusSignToUnsignedWords()
    assert p.words == ['+some', '+#{1}', '-text', '+and', '+id:#{2}', '+text']
  }

  @Test
  void joinProcessedWords() {
    def p = new QueryProcessor('')
    p.words = ['+some', '-minus', '+#{1}']
    p.joinProcessedWords()
    assert p.query == "+some -minus +#{1}"
  }

  @Test
  void replaceExpressions() {
    def p = new QueryProcessor('+#{1} -minus +transaction_id:#{2}')
    p.expressions = ['first', '(23 or 98)']
    p.replaceExpressions()
    assert p.query == '+first -minus +transaction_id:(23 or 98)'
  }

  @Test
  void processedQuery() {
    def p = new QueryProcessor('coca-cola -pepsi  transaction_id:(34 or 76)')
    assert p.processedQuery == '+coca-cola -pepsi +transaction_id:(34 or 76)'
  }
}

Conclusion

That is it. I'd like you to share your opinions on other techniques I may have not talked about here. Are there any improvements that you think would make this code even easier to understand? I'd really appreciate any other considerations you might have since I'm always very interested in writing Clean Code.

comments powered byDisqus