Group key-value pairs in chunks no longer than specified limit Sometime ago I saw a code which packed key,value pairs into several GET messages. That code had a bug (that is why it was shown to me: as the example of beautiful code which doesn't do what it should).

Today I decided to write this code out of curiosity and for training.

This is what I have written:

def group(url,D,limit):
    if len(D) == 0:
        if len(url) &lt;= limit:
            yield url
            return
        else:
            raise ValueError("url is longer than limit")

    if len(url) &gt; limit - 2:
        raise ValueError("url is too long to be able to hold any key-value pairs")

    for i,(key,value) in enumerate(D.iteritems()):
        if i == 0:
            old = url
            new = '{url}?{key}={value}'.format(**locals())
            continue
        new, old = '{new}&amp;{key}={value}'.format(**locals()), new

        if len(new) &gt; limit:
            if len(old) &gt; limit:
                raise ValueError("String '{old}' is longer than GET query limit.".format(**locals()))
            yield old
            old = url
            new = '{url}?{key}={value}'.format(**locals())
    if len(new) != len(url):
        if len(new) &gt; limit:
            raise ValueError("String '{new}' is longer than GET query limit.".format(**locals()))
        yield new


I have written some tests and the code seems to work correctly.

Still, I don't like it.

If we leave alone corner case checking at the beginning, we will only have one working cycle.
Inside that cycle I had to keep two strings: old and new. If new is longer than the limit, I yield the old one, and start a new string with blank url plus key,value pair added which caused the previous new string to grow beyond the limit.

To make it work I had to write the same statements in different places of that loop.

For example, 

old = url
new = '{url}?{key}={value}'.format(**locals())


is written it two places.

raise ValueError("String '{new}' is longer than GET query limit.".format(**locals()))


is also in two places.

Can it be refactored to have statements to be written only once without duplication?

Are there any other things that I should pay attention to?
def group(url,D,limit):


D is a bad parameter name because it doesn't follow the python convention: lowercase_with_underscores, and because it a pointless abbreviation.

    if len(D) == 0:
        if len(url) &lt;= limit:
            yield url
            return
        else:
            raise ValueError("url is longer than limit")

    if len(url) &gt; limit - 2:
        raise ValueError("url is too long to be able to hold any key-value pairs")


Is this really a useful check? Two characters is probably not enough to actually hold any of the pairs. You probably don't catch anything here you wouldn't already catch

    for i,(key,value) in enumerate(D.iteritems()):
        if i == 0:


enumerating for the sole purpose of special casing the first entry is rather gross. 

            old = url
            new = '{url}?{key}={value}'.format(**locals())
            continue


I discourage the use of continue. It's almost always better to put things in an else block

        new, old = '{new}&amp;{key}={value}'.format(**locals()), new


Use of locals() is discouraged. It encourages sloppy coding and forces python into a slower compatibility mode for that function. 

        if len(new) &gt; limit:
            if len(old) &gt; limit:
                raise ValueError("String '{old}' is longer than GET query limit.".format(**locals()))
            yield old
            old = url
            new = '{url}?{key}={value}'.format(**locals())
    if len(new) != len(url):


In which case would this be false?

        if len(new) &gt; limit:


Why is this check inside the other check?

            raise ValueError("String '{new}' is longer than GET query limit.".format(**locals()))
        yield new


Of course, you've already noted the duplicated code here.

Here's my take on reworking it:

def blocked(items, limit, seperator):
    """
    items is a list of strings, to be joined with seperator.
    Produces a generator of strings, each with a length &lt;= the limit
    """
    current = []
    current_length = 0
    for item in items:
        length = len(item) + len(seperator)
        if length &gt; limit:
            raise ValueError('{} cannot fit'.format(item))
        elif length + current_length &gt; limit:
            yield seperator.join(current)
            current = [item]
            current_length = length
        else:
            current.append(item)
            current_length += length
    yield seperator.join(current)

def group(url,data,limit):
    if len(url) &gt; limit:
        raise ValueError('{} is longer than the limit of {}'.format(url, limit))

    pairs = ["{}={}".format(k,v) for k, v in data.iteritems()]
    for block in blocked(pairs, limit - len(url), "&amp;"):
        if block:
            yield '{}?{}'.format(url, block)
        else:
            yield url