Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

This is a function I just wrote that tries to condense a set of strings into grouped lines. It actually works, but looks ugly. Is there a better way to achieve the same thing?

Take 4 filtering empty strings first and dropping the if line on the final yield

def mergeToLines(strings, length=40, sep="    "):
    strs = (st for st in sorted(strings, key=len, reverse=True) if st)
    line = strs.next()
    for s in strs:
        if (len(line) + len(s) + len(sep)) >= length:
            yield line
            line = s
        else:
            line += sep + s
    yield line

The idea is that it takes a bunch of strings, and combines them into lines about as long as lineLength. Each line has one or more of the original strings in it, but how many isn't clear until I actually get to slotting them in. Also, note that if a single entry in the list is longer than lineLength, it's still added as its own line (dropping elements is not an option here).

share|improve this question
1  
In take 3, if line: is redundant unless you have empty strings in strings. If you do have empty strings, you probably should filter out all of them earlier on. – Janne Karila Apr 26 at 6:36
@JanneKarila - good point. – Inaimathi Apr 26 at 13:46

2 Answers

up vote 3 down vote accepted

RECURSIVE SOLUTION: NOTE: it assumes that the input strings is already sorted. You could sort this at every call of the function, but this seems like unnecessary overhead.

def recursiveMerge(strings=[], current_line='', line_length=40, result=[]):
    """ Gather groups of strings into a line length of approx. 40 """

    # base case
    if not strings:
        result.append(current_line)
        return result

    else:
        next_line = strings.pop(0)

        # if current_line + next_line < line_length, add them
        # otherwise append current_line to result
        if len(current_line + next_line) < line_length:
            current_line = '    '.join([current_line, next_line]).lstrip(' ')    
        else:
            result.append(current_line)
            current_line = next_line

        # recursively call function
        return recursiveMerge(strings, current_line = current_line, result=result)

ITERATIVE SOLUTION:

def myMerge(strings, line_length=40, res=[]):
    """ Gather groups of strings into a line length of approx. 40 """

    # sort the list of string by len
    strings = sorted(strings, key=len, reverse=True)

    # loop through the group of strings, until all have been used
    current_line = strings.pop(0)
    while strings:
        next_line = strings.pop(0)

        # if current_line + next_line shorter than line_length, add them
        # otherwise, append current_line to the result

        if len(current_line + next_line) < line_length:
            current_line = '    '.join([current_line, next_line])
        else:
            res.append(current_line)
            current_line = next_line

    # add any remaining lines to result     
    res.append(current_line)

    return res
share|improve this answer
1  
l_len should be line_length, no? – William Morris Apr 26 at 0:59
it should - thank you. Have edited – Nick Burns Apr 26 at 2:21
up vote 2 down vote

Your code isn't so ugly, guys :) But I can write it better

def cool_merger(strings, line_len=40, sep="    "):
    ss = list(sorted(strings, key=len, reverse=True))
    return [line for line in [(lambda n, s: s if n+1 < len(ss) and (len(s) + len(ss[n+1]) >= line_len) else (ss.__setitem__(n+1, sep.join((str(ss[n]), str(ss[n+1]))))) if len(ss) > n+1 else ss[n] )(n, s) if len(s) < line_len else s for n, s in enumerate(ss)] if line != None]

(joke)

By the way, works fast.

share|improve this answer
2  
+1 for it being crazy short! Very nice. Wouldn't want to be your team-mate who had to maintain it though :) – Nick Burns Apr 26 at 0:19
@NickBurns Thanks :) Surely it's written not for production but just for fun. Maintainability should be on the first place. – dexor Apr 26 at 6:53

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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