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) <= limit:            yield url            return        else:            raise ValueError(url is longer than limit)    if len(url) > 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}&{key}={value}'.format(**locals()), new        if len(new) > limit:            if len(old) > 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) > limit:            raise ValueError(String '{new}' is longer than GET query limit..format(**locals()))        yield newI 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 = urlnew = '{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?
Group key-value pairs in chunks no longer than specified limit
python
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) <= limit:            yield url            return        else:            raise ValueError(url is longer than limit)    if len(url) > 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())            continueI discourage the use of continue. It's almost always better to put things in an else block        new, old = '{new}&{key}={value}'.format(**locals()), newUse of locals() is discouraged. It encourages sloppy coding and forces python into a slower compatibility mode for that function.         if len(new) > limit:            if len(old) > 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) > limit:Why is this check inside the other check?            raise ValueError(String '{new}' is longer than GET query limit..format(**locals()))        yield newOf 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 <= the limit        current = []    current_length = 0    for item in items:        length = len(item) + len(seperator)        if length > limit:            raise ValueError('{} cannot fit'.format(item))        elif length + current_length > 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) > 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), &):        if block:            yield '{}?{}'.format(url, block)        else:            yield url