I have one following question and write a function for it. can anyone give me some suggestion how my code can be better and better way to handle to build up return list with even frequency?

One message system contains two device type message that each message is formatted with device type identifier with device id and message count. Write a function to parse the message based on device type and message count and return a list with each device id in even message frequency
rule: input string: message
device identifier
1:iOS device ID identifier: start with 'I' following with 3 character, total length is 4 character
2:Android device ID identifier: start with 'A' following with 2 character, total length is 3 character
3:message count is following by device id until next device ID
ex: input: Asq2: {'Asq': 2} Asq with 2 message count
output: ['Asq', 'Asq']

input: Akb2IAld3: ID: {'Akb': 2, 'IAld2': 3} Akb with 2 message count, IAld with 3 message count
output: ['Akb', 'IAld', 'Akb', 'IAld', 'IAld']

input: Aqp1Iasd2Aqp4IAbd1: {'Aqp': 5, 'Iasd': 2, 'IAbd': 1}
output: ['Aqp', 'Iasd', 'IAbd', 'Aqp', 'Iasd', 'Aqp', 'Aqp', 'Aqp']

from typing import List

def parse_message(string) -> List:
    i, j, ids_map, n, ids = 0, 0, dict(), len(string), ''

    while i < n:
        if string[i] in ('I', 'A') or i == n - 1:
            if ids:
                if i == n - 1:
                    ids_map[ids] = ids_map.get(ids, 0) + int(string[j:])
                    ids_map[ids] = ids_map.get(ids, 0) + int(string[j:i])
            j = i + 4 if string[i] == 'I' else i + 3
            ids = string[i:j]
            i = j - 1
        i += 1
    res = []
    while any(i > 0 for i in ids_map.values()):
        for k, v in ids_map.items():
            if v > 0:
                ids_map[k] -= 1
    return res


1 Answer 1


Your code is really hard to understand. When I read the description I noticed three steps:

  1. Extract ID and message count.
  2. Total duplicate message counts.
  3. Return a round robin of these IDs.

You've got 1&2 mangled together, and 3 nicely by itself.

Since you're using Python, it's easier to read iterator based approaches to these things. Firstly you can replace your step 3 with a recipe from the itertools standard library. All you need to do is call itertools.repeat beforehand.

It's not immediately clear what your code is doing when you've mangled indexed iteration with business logic, with step 2 as well. And so I suggest re-writing it. You know the data will come in the form {type}{message}{amount}, where the type and amount has a length of 1, and message is either 2 or 3. And so to extract the messages and amounts you can just use next/islice on an iterator. Using a try while loop. After this totaling the message counts is simple.

Also if you're going to use typing you should use mypy too, and put type information on both the arguments and return types.

from typing import List, Iterator, Tuple, Sequence, TypeVar import itertools

    'I': 3,
    'A': 2

TValue = TypeVar('TValue')

def roundrobin(*iterables: Tuple[Sequence[TValue], ...]) -> Iterator[TValue]:
    "roundrobin('ABC', 'D', 'EF') --> A D E B F C"
    # Recipe credited to George Sakkis
    num_active = len(iterables)
    nexts = itertools.cycle(iter(it).__next__ for it in iterables)
    while num_active:
            for next in nexts:
                yield next()
        except StopIteration:
            # Remove the iterator we just exhausted from the cycle.
            num_active -= 1
            nexts = itertools.cycle(itertools.islice(nexts, num_active))

def extract_messages(input: Iterator[str]) -> Iterator[Tuple[str, int]]:
    input = iter(input)
    message_type = next(input)
    while True:
        length = MESSAGE_LENGTH[message_type]
        message = ''.join([message_type] + list(itertools.islice(input, length)))
        message_type = next(input)
        number = []
            while message_type not in MESSAGE_LENGTH:
                number += [message_type]
                message_type = next(input)
        except StopIteration:
            yield message, int(''.join(number))

def parse_message(string: str) -> List[str]:
    message_counts = {}
    for message, amount in extract_messages(string):
        message_counts.setdefault(message, 0)
        message_counts[message] += amount

    return list(roundrobin(*(
        itertools.repeat(key, amount)
        for key, amount in message_counts.items()

if __name__ == '__main__':
  • \$\begingroup\$ thanks for feedback. however need to clarify that length of message count does not have to be 1, we may have input like Akb2000IAld300 \$\endgroup\$
    – A.Lee
    Commented Mar 27, 2019 at 18:32
  • \$\begingroup\$ @A.Lee I don't see that in the examples or text you gave in your original post. It's also easy to change my code to account for that. \$\endgroup\$
    – Peilonrayz
    Commented Mar 27, 2019 at 18:34
  • \$\begingroup\$ understand, again, great feedback and lots of great point. thank you \$\endgroup\$
    – A.Lee
    Commented Mar 27, 2019 at 18:35
  • \$\begingroup\$ @A.Lee Please see the updated code to handle that. In the future I'd recommend that you link to challenge descriptions so these errors don't happen again. :) \$\endgroup\$
    – Peilonrayz
    Commented Mar 27, 2019 at 18:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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