15

I just wrote a function that spans approximately 100 lines. Hearing that, you are probably tempted to tell me about single responsibilities and urge me to refactor. This is my gut instinct as well, but here is the issue: The function does one thing. It performs a complex string manipulation, and the function body consists mostly of one verbose regex, broken up into many lines that are documented. If I broke up the regex into multiple functions, I feel like I would actually lose readability, since I am effectively switching languages, and won't be able to take advantage of some features regexes offer. Here now is my question:

When it comes to string manipulation with regular expressions, are large function bodies still an anti-pattern? It seems like named capture groups serve a very similar purpose to functions. By the way, I have tests for every flow through the regex.

closed as primarily opinion-based by gnat, Bart van Ingen Schenau, user40980, GlenH7, Robert Harvey Mar 6 '14 at 20:27

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 3
    I don't think that there's anything wrong with your function, considering that a large part of it is documentation. There may be a maintainability issue with using a large regular expression in the first place however. – Joel Cornett Mar 6 '14 at 6:04
  • 2
    Are you sure a giant regex is the best solution to your problem? Have you considered simpler alternatives, like a parser library or replacing a custom file format with a standard one (XML, JSON etc)? – lortabac Mar 6 '14 at 9:03
  • 2
    Are there other functions, using an altered / enhanced / simplified version of this regex? That would be an important indicator that refactoring should take place. If not, I'd leave it as it is too. Needing a complex string manipulation like that is a yellow flag in its own right (well I don't know the context, hence just yellow), and refactoring the function down seems to me more like a ritual to redeem from the guilt one feels about it ;) – Konrad Morawski Mar 6 '14 at 9:57
  • 8
    How can a 100 line regexp do only 1 thing? – Pieter B Mar 6 '14 at 11:04
  • @lortabac: The input is user generated text (prose.) – DudeOnRock Mar 6 '14 at 17:54
36

What you're encountering is the cognitive dissonance that comes from listening to people who favor slavish adherence to guidelines under the guise of "best practices" over reasoned decision making.

You've clearly done your homework:

  • The function's purpose is understood.
  • The workings of its implementation is understood (i.e., readable).
  • There are full-coverage tests of the implementation.
  • Those tests pass, meaning you believe the implementation to be correct.

If any of those points weren't true, I'd be first in line to say that your function needs work. So there's one vote for leaving the code as-is.

The second vote comes from looking at your options and what you get (and lose) from each:

  • Refactor. This gains you compliance with someone's idea of how long a function should be and sacrifices readability.
  • Do nothing. This maintains existing readability and sacrifices compliance with with someone's idea of how long a function should be.

This decision comes down to which you value more: readability or length. I fall into the camp that believes length is nice but readability is important and will take the latter over the former any day week.

Bottom line: if it isn't broken, don't fix it.

  • 10
    +1 for "If it isn't broken, don't fix it." – Giorgio Mar 6 '14 at 5:39
  • Indeed. Sandy Metz rules (gist.github.com/henrik/4509394) are nice and all, but at youtube.com/watch?v=VO-NvnZfMA4#t=1379 she talks about how they came to be, and why people are taking them way too seriously. – Amadan Mar 6 '14 at 6:53
  • @Amdan: With the extra context from the video, what Metz did makes sense. Her recommendation to that one customer was intentionally extreme at one end to counter behavior that was extreme at the other end as a way to drag it into the more-reasonable middle. The rest of that discussion boils down to the thrust of my answer: reasoning, not faith, is the way to determine the best course of action. – Blrfl Mar 6 '14 at 14:20
19

Honestly, your function may "do one thing", but as you stated by yourself

I could start breaking up the regex into multiple functions,

which means your reg ex code does a lot of things. And I guess it could be broken down into smaller, individually testable units. However, if this is a good idea is not easy to answer, (especially without seeing the actual code). And the correct answer may be neither "yes" or "no", but "not yet, but next time you have to change something in that reg exp".

but feel like I would actually lose readability that way, since I am effectively switching languages

And this is the core point - you have a piece of code written in reg ex language. This language does not provide any good means of abstraction in itself (and I don't consider "named capture groups" as a replacement for functions). So refactoring "in the reg ex language" is not really possible, and interweaving smaller reg exps with the host language may not actually improve readability (at least, you feel so, but you have doubts, otherwise your would not have posted the question). So here is my advice

  • show your code to another advanced developer (maybe on https://codereview.stackexchange.com/) to make sure others think about readability the way you do. Be open to the idea that others may not find a 100 line reg exp as readable as you. Sometimes the notion of "its not easily breakable into smaller pieces" can be overcome just by a second pair of eyes.

  • observe the actual evolvability - does your shiny reg exp still look so good when new requirements arrive and you have to implement and test them? As long as your reg exp works, I would not touch it, but whenever something has to be changed, I would reconsider if it was really a good idea to put everyhing into this one big block - and (seriously!) rethink if splitting into smaller pieces would not be a better option.

  • observe the maintainability - can you effectively debug the reg exp in the current form very well? Especially after you have to change something, and now your tests tell you that something is wrong, do you have a reg exp debugger helping you to find the root cause? If debugging gets hard, that would also be a occasion to reconsider your design.

  • I'd say named capture groups (capture groups in general, really) are most similar to final/write-once variables, or perhaps macros. They allow you to reference specific portions of the match, either from the match object returned from the regex processor or later in the regular expression itself. – JAB Mar 6 '14 at 19:36
4

Sometimes a longer function that does one thing is the most appropriate way to handle a unit of work. You can easily get into very long functions when you start dealing with querying a database (using your favorite query language). To make a function (or method) more readable while limiting it to its stated purpose is what I would consider the most desirable outcome of a function.

Length is an arbitrary "standard" when it comes to code size. Where a 100 line function in C# may be considered longish, it would be tiny in some versions of assembly. I have seen some SQL queries that were well into the 200 lines of code range that returned one very complicated set of data for a report.

Fully working code, that is as simple as you can reasonably make it is the goal.

Don't change it just because it is long.

3

You could always break up the regex into sub-regexes, and gradually compose the final expression. This could aid comprehension for a very large pattern, particularly if the same sub-pattern is repeated many times. For example in Perl;

my $start_re = qr/(?:\w+\.\w+)/;
my $middle_re = qr/(?:DOG)|(?:CAT)/;
my $end_re = qr/ => \d+/;

my $final_re = $start_re . $middle_re . $end_re;
# or: 
# my $final_re = qr/${start_re}${middle_re}${end_re}/
  • I use the verbose flag, which is even more convenient than what you are suggesting. – DudeOnRock Mar 6 '14 at 17:58
1

I would say break it if it is breakable. from maintainability point of view and maybe resuability it makes sense to break it, but of course you have to consider natural of your function and how you get input and what it is going to return.

I remember I was working on parsing streaming chunked data into objects, so what I did basically was I divided it into two main parts, one was building complete unit of String out of encoded text and in second part parsing those units into data dictionary and organize them (could be random property for different object) and than updating or creating objects.

Also I could break each main part into several smaller and more specific functions so at end I had 5 different functions to do the whole thing and I could reuse some of the functions in different place.

1

One thing that you may or may not have considered is to write a small parser in the language you are using instead of using a regex in that language. This may be easier to read, test, and maintain.

  • I have thought about this myself. The issue is that the input is prose and I am taking cues from context and formatting. If it is possible to write a parser for something like this, I would love to learn more about it! I couldn't find anything myself. – DudeOnRock Mar 6 '14 at 17:57
  • 1
    If a regex can parse it, you can parse it. Your response makes it seem to me that you may not be well versed in parsing. If that is the case, you may want to stick with the regex. Either that or learn a new skill. – Thomas Eding Mar 6 '14 at 19:04
  • I'd love to learn a new skill. Any good resources you can suggest? I am interested in the theory behind it as well. – DudeOnRock Mar 6 '14 at 19:07
1

Giant regexes are a bad choice in most cases. In my experience, they are often used because the developer is not familiar with parsing (see Thomas Eding's answer).

Anyway, let's assume you want to stick to a regex-based solution.

As I don't know the actual code, I will examine the two possible scenarios:

  • The regex is simple (a lot of literal matching and few alternatives)

    In this case the advanced features offered by a single regex are not indispensable. This means you will likely benefit from splitting it.

  • The regex is complex (a lot of alternatives)

    In this case you cannot realistically have full test coverage, because you probably have millions of possibile flows. So, in order to test it, you need to split it.

I might lack imagination, but I can't think of any real-world situation where a 100-line regex is a good solution.

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