3
\$\begingroup\$

I am a beginner coder and SonarLint tells me code is too complex. I tried recursion, iterators, slicing but had not luck making this code any simpler.

The code will parse the input string to detect the following categories:

  • bold
  • italic
  • normal

depending on whether the characters are enclosed in 0-2 asterisks.

Sorry for the short description luckily, Iterniam inferred correctly. And side note nested **Bold *and* italic** are not supported by the code. I will read more on Iterniam's answer thank you for that!

Maybe a philosophical question at last, even though cyclomatic complexity in my code is higher the regex answer, the proposed solution is still much harder to understand (if it were not for the comments), debug and has worse performance. So is it the better code or should I just ignore cyclomatic complexity hints in the future?

txt = "A **A** is *italic* **bold** but some are *a* *b* **B**"

def fun(text=None):
    if text:
        bold_splits = text.split('**')
        for i, s in enumerate(bold_splits):
            if i % 2:
                print('BOLD:', s )
            else:
               italic_splits = s.split('*')
               for m, n in enumerate(italic_splits):
                   if m % 2:
                       print('ITALIC:', n )
                   else:
                       if n != '':
                           print('NORMAL:', n )
\$\endgroup\$
5
  • 4
    \$\begingroup\$ We need to know what the code is intended to achieve. To help reviewers give you better answers, please add sufficient context to your question, including a title that summarises the purpose of the code. We want to know why much more than how. The more you tell us about what your code is for, the easier it will be for reviewers to help you. The title needs an edit to simply state the task, rather than your concerns about the code. \$\endgroup\$ Mar 5 '21 at 15:42
  • 2
    \$\begingroup\$ By "too complex", they're likely referring to "cyclomatic complexity". If that's the case, it means essentially that your code is too deeply nested, which is leading to too many independent paths of execution within the function. Some code (like the inner loop) can be factored out into its own function for starters. \$\endgroup\$ Mar 5 '21 at 18:07
  • 1
    \$\begingroup\$ Is nesting supposed to be supported? "For example: **Bold text with *Italics* embedded inside**."? \$\endgroup\$
    – AJNeufeld
    Mar 5 '21 at 18:23
  • \$\begingroup\$ The code complexity can be reduced by breaking the solution into functions. Check out the Single Responsibility Principle. \$\endgroup\$
    – pacmaninbw
    Mar 8 '21 at 12:44
  • \$\begingroup\$ the proposed solution is still much harder to understand, debug… this entirely depends on internalisation of regular expression implementations - once mastered, Iterniam's proposition is obvious, if excellently crafted. \$\endgroup\$
    – greybeard
    Mar 9 '21 at 7:21
4
\$\begingroup\$

It looks like you're trying to extract information from a string. This is most often done through regexes as they are often faster than your own code while being less complex.

Because as of writing, you have no clear description of your goal, I'm inferring these assumptions from your code:

  • You want to split every sequence of characters in your string in one of the categories bold, italic and normal, determined by the characters being enclosed in 2, 1 or 0 asterisks.
  • You want to preserve the order of the original string.

If we disallow asterisks in the normal text, a regex for this is rather simple. I recommend exploring what it does on regex101.

\*\*(?P<bold>.+?)\*\*|\*(?P<italic>.+?)\*|(?P<normal>[^*]+)

Notably, the | character matches on the first match it finds, from left to right, meaning that a match will always try to match bold, then italic, then normal.

In Python, we can iterate over all the matches using re.finditer. For every match, lastgroup will return the name of the captured group, so we can use that in our print statement and also use it to get the actual match that we're interested in.

def fun(text=None):
    if text:
        for match in re.finditer(r'\*\*(?P<bold>.+?)\*\*|'  # match bold lazily
                                 r'\*(?P<italic>.+?)\*|'  # match italics lazily
                                 r'(?P<normal>[^*]+)',  # match normal greedily
                                 text):
            print(f'{match.lastgroup.upper()}: {match.group(match.lastgroup)}')

It should be noted that if the function is called plenty of times and performance is of concern, you should compile the regex once before using it in fun.

The regex used in this example is very primitive because you haven't specified your constraints or even what the function is meant to do. By specifying your question better and explaining how edge cases like ** a ** *** and **ab*cd** should behave, a more complete answer can be given.

Everything below this is in response to your 'philosophical question':

The regex answer [...] has worse performance.

I did some testing using timeit on the original string (multiplied in size anywhere between 0-3000 times) on differing number of runs and found that the regex approach is consistently 1.5 times faster than the original solution as long as the print statements are included.
Because printing it is unlikely to be your actual use-case, I modified both functions to return a tuple of all bold, italic and normal matches. I was surprised to find that your code is generally twice as fast.

The proposed solution is still much harder to understand (if it were not for the comments).

The comments are exactly what make the regex a relatively readable solution compared to yours. By commenting what the individual parts do, someone that needs to use the code can quickly see what the code achieves. The cost is that it may require more time to figure out how the code works.

Note that this extra cost also diminishes as you get more experience. For some, figuring out how your code achieves its goal is more difficult than understanding the regex. Regexes are very powerful, quite popular, and are used in many languages. This means that when one encounters a regex construct they know what to look for and how to read it. Your solution, meanwhile, is based on a few implicit assumptions like "every other string from the split is the thing we want to match". Someone that reads your code has to discover these assumptions because they're not explicitly written down.

The proposed solution is still much harder to [...] debug.

If nothing else, it's easy to pinpoint where the issue if there is one: in the regex. Given the input, it shouldn't be hard to figure out why the regex is failing if you inspect it using a tool like regex101. It may be difficult to correct the regex, but at least you won't have a scavenger hunt for where your bug is.

Should I just ignore cyclomatic complexity hints in the future?

Avoiding a high cyclomatic complexity mostly comes down to avoiding nesting a little more. This ensures that you have a more linear (and therefore more comprehensible) flow to your code.

The algorithm you used requires quite a bit of nesting which is not readily avoidable. You can, however, make some improvements by using the if as a guard. Using list comprehension, you can also eliminate the final for-if-else loop.

def fun_unnested_order(text=None):
    if not text:
        return None

    res = []
    bold_splits = text.split('**')  # Odd entries are bold, even entries are italicized or normal.
    for i, s in enumerate(bold_splits):
        if i % 2 == 1:  # Store bold
            res.append((s, 'bold'))
        else:
            italic_splits = s.split('*')  # Odd entries are italicized, rest normal.
            # Replace the for-if-else loop with list comprehension
            res += [(e, 'italic') if i % 2 == 1 else (e, 'normal')
                    for i, e in enumerate(italic_splits)]
    return res

This example is 30% slower than your original code using the same timeit setup.

So is it the better code?

It depends on your viewpoint. If performance is of concern, you should go for greybeard's answer as it is around 30% faster than your original code.
If readability is of concern, I would take the regex version any day of the week. While many may consider regexes black magic, it is widely applicable to many problems in most if not all popular languages. Most crucially perhaps, the regex solution doesn't require a complex algorithm, which means it's easier to develop and less error-prone.
Again, in terms of readability, it's about the familiarity of the programmer. If you are unfamiliar with regexes, list comprehensions and/or list slicing, your original version is the option to go for.

In the end, there is no one implementation that wins across all categories. You should familiarize yourself with the different ways to approach the problem and then tackle it in a way you deem the best solution.

\$\endgroup\$
7
  • \$\begingroup\$ FWIW to support **ab*cd** you can change bold's [^*]+ to .+?. We can do the same for italic, but currently nothing would change. IMO non-greedy searches are normally more helpful here. \$\endgroup\$
    – Peilonrayz
    Mar 5 '21 at 17:07
  • \$\begingroup\$ @Peilonrayz I've edited the post to include your suggestion as it is indeed cleaner, thanks. \$\endgroup\$
    – Iterniam
    Mar 5 '21 at 17:55
  • \$\begingroup\$ From the documentation "REs separated by '|' are tried from left to right. When one pattern completely matches, that branch is accepted. This means that once A matches, B will not be tested further, even if it would produce a longer overall match. In other words, the '|' operator is never greedy." \$\endgroup\$
    – RootTwo
    Mar 5 '21 at 21:57
  • \$\begingroup\$ @RootTwo I corrected it in the post. I originally tested matching aba|aabaa against aabaa, which matches the latter, which led me to conclude that the longest match was used. Instead, it appears to be because aabaa occurs earlier in the string. \$\endgroup\$
    – Iterniam
    Mar 6 '21 at 19:30
  • 1
    \$\begingroup\$ @Iterniam Thank you for taking the time, you really helped me a lot. I guess readability comes down to what you are used to and I agree with you about debugging my code has more implicit assumptions. My only nitpick is the last solution would be the help I was looking for, if it would preserve the order. I tried playing with enumerate but could not find anything "more" elegant than the original. \$\endgroup\$ Mar 9 '21 at 11:39
1
\$\begingroup\$

A tool telling how complex code is.
Cyclomatic complexity, at that.

Take a somewhat deep breath - or sigh.
Omission of docstrings, renaming and comments in the following is intentional:

def fun(text=None):
    if not text:
        return
    splits = text.split('*')
    bold = text[0] == '*'
    italics = True
    for s in splits:
        italics = not italics
        if s:
            category = 'BOLD:' if bold    \
                else 'ITALIC:' if italics \
                else 'NORMAL:'
            print(category+s+'#')
        else:
            bold = not bold

What is your assessment of relative complexity, what is the tool's?
(Well, there is something to take away:
 returning from a method early does not only reduce statement nesting&indentation level:
 I find it simpler to think about the resultant code.
 Same for leaving the current loop iteration early via continue or break.)
(p.s. Do you believe the above works? (Do I?))

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I am not sure if this is the point you wanted to make, but I think this code is awesome! I definitely play with this solution. More to your point, SonarLint has an easier time telling a lot of indentations mean high code complexity, whereas this seems far more complex. Good point! \$\endgroup\$ Mar 9 '21 at 11:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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