5

Here I'm working in Python, but it's more of a language agnostic question, unless specific language features makes it clear that an option is better than the other.

I get my raw data from a REST API, and need to do many operations with it.

tc_builds_by_agent = [
        {
            'agent_id': agent_id["id"],
            'agent_name': agent_id["name"],
            'builds':
                [
                    dict(
                        element.attrib,
                        **{
                            'duration': str(
                                datetime.strptime(list_element[1].text[:-5], "%Y%m%dT%H%M%S") -
                                datetime.strptime(list_element[0].text[:-5], "%Y%m%dT%H%M%S")
                            )
                        }
                    )
                    for element in list(
                        tc_rest_request(f'/app/rest/builds?locator=agent(id:{agent_id["id"]})&fields=count,build(id,number,status,state,buildTypeId,startDate,finishDate)&count=10000')
                    )
                    if (list_element := list(element))
                ]
        } for agent_id in agents_id]

So at the same time, I do two list comprehensions, construct a dictionnary, and slice strings -> convert to date time -> calculate delta -> convert back to string, and it's not over yet.

For context the agents_id array may have tens of elements, but each result from the REST call by tc_rest_request have thousands of elements, so at the end tens of thousands elements are being processed.

As a general rule I always try to iterate as little as possible on the same data, because it just seems more efficient.

But now I think I'm hitting a road block, because I need to also work on element.attrib which are dictionnaries with less than 10 elements, and do some more operations on each of them like string splitting and grouping. And as I said there are tens of thousands of them. I'm struggling to find a way to do them in this double list comprehension. And this section of code is already quite busy, it could become even more of a monstrosity.

Optimization isn't really an issue here, meaning that if it takes 15 or 20 seconds instead of 5 it doesn't matter. Of course the faster the better, but if I don't find a solution to do what I want in the next few hours I'll just do an other iteration over tc_builds_by_agent, and it'll do just fine.

So in such a situation, when each new iteration could come at a significant performance cost, what do you prefer, or what is objectively better?

Simpler, clearer code, that may take twice or thrice as long, or a more complex dense code for the sake of performance (and maybe a sense of satisfaction that you succeeded to do it as you wanted)?

5
  • 2
    or a more complex dense code for the sake of performance is "high" performance among requirements?
    – Laiv
    Dec 15, 2022 at 11:36
  • In my case at the moment not really, it's just the first POC version of the system I'm writing. But it could be later, especially since the volume of data I'm using is regularly growing. Slowly, I'd say tens or hundreds per day, but it's growing. Dec 15, 2022 at 11:40
  • 4
    Unless you need it. YAGNI prevails. So you make the simplest solution possible, perhaps, performing the operation in each iteration. Then you compare its performance with the one expected or desired. You can't optimize something unless you have something to optimize.
    – Laiv
    Dec 15, 2022 at 11:45
  • 1
    I'd make a strong case for putting any complicated list comprehensions into a function, so they become just "do_something(x) for x in y". @Amon 's answer is a good one.
    – pjc50
    Dec 15, 2022 at 13:25
  • I'll give you the advice that was given to me when I first started coding; do not prematurely optimize your code. Get it working first in a way that's readable. THEN once it's working, see if you even NEED to optimize. Folk around our office often nickname it list incomprehension because of how quickly the code can become ugly to read. Dec 22, 2022 at 16:18

4 Answers 4

12

Optimize for clarity. Iteration overhead tends to be low.

Some people think that this kind of code

for x in data:
  a(x)
  b(x)

is going to be much more efficient than

for x in data:
  a(x)

for x in data:
  b(x)

But that is not the case. Usually, there is very low overhead from running the for-loop, with the actual time being spent in the functions a() and b(). Then, there is no significant performance impact from choosing one option or the other. Performance does not provide an excuse to avoid clear code.

Exception: The two code snippets differ in their ordering of the a(x) and b(x) invocations. Sometimes that ordering is relevant. Also, I'm assuming you're iterating over data that already exists in memory. If you're iterating over a generator, then you can only do that once, which may require you to implement some kind of caching.

Your current code is very complex, with nested list comprehensions, mixing different tasks (fetching data, parsing it, and organizing it into a new structure), and using features like the walrus-operator for assignment := where I'm not immediately sure what the data flow for that is going to be.

Before modifying this code, I want to note that you are making network requests. Network requests tend to be orders of magnitude slower than the actual work done by your code. For example, the network request might take 50ms, but all the other parts of your code (iteration, formatting dates, creating dicts and lists) might complete in 5μs. Those numbers are made up, but my point is that there might very well be a factor of 10000× here. Focusing on iteration patterns is close to irrelevant because they might only change the total runtime by 0.01%.

What I would do in your case is to extract the processing for one agent into a separate function. With some additional changes, your code might look like:

tc_builds_by_agent = [full_build_info_for_agent(agent) for agent in agents_id]

def full_build_info_for_agent(agent):
  agent_id = agent['id']
  agent_name = agent['name']

  # TODO build URL using library to avoid injection/escaping issues
  locator = f'agent(id:{agent_id})'
  fields = 'count,build(id,number,status,state,buildTypeId,startDate,finishDate)'
  result = tc_rest_request(f'/app/rest/builds?locator={locator}&fields={fields}&count=10000')

  return {
    'agent_id': agent_id,
    'agent_name': agent_name,
    'builds': list(make_build_info(result))
  }

def make_build_info(result):
  for element in result:
    build = make_one_build_info(element)
    if build:
      yield build

def make_one_build_info(element):
  timestamps = list(element)
  if not timestamps:
    return None

  start = parse_timestamp(timestamps[0].text[:-5])
  end = parse_timestamp(timestamps[1].text[:-5])

  build = dict(element.attrib)  # TODO more processing here
  build['duration'] = str(end - start)
  return build

def parse_timestamp(s):
  datetime.strptime(s, "%Y%m%dT%H%M%S")

Note that if your tc_rest_request() function were async, then this entire code could now be made concurrent with very small changes, potentially allowing for much faster overall processing of all agents.

Even without async, we can now use a thread pool to easily make multiple requests at the same time:

import concurrent.futures

# TODO select a suitable limit on concurrency,
# to avoid overwhelming the server
with concurrent.futures.ThreadPoolExecutor(max_workers=5) as executor:
  tc_builds_by_agent = list(executor.map(full_build_info_for_agent, agents_id))

Choosing a sensible structure for our code makes it easier to understand, much easier to extend, and also much easier to optimize it meaningfully. Micro-optimizations like avoiding another loop would be rarely worth it.

1

As @amon already said, optimize for clarity. Optimize for performance only if you know that you have a performance problem, and then optimize targetting exactly and only the symptom that you were able to measure, typically with a profiler tool. And I bet that the iterating code will be negligible.

Regarding clarity, my guideline is to view elements like classes, fields, methods etc. as vocabulary that you introduce, in order to create a language where you can express your algorithm in a straightforward, easily-readable way.

To use an example from plain English:

If nobody had ever coined the word "car", you'd have to say: "I'm going home in my machine with four wheels, an engine, some seats, and some doors...". Saying "I'm going home in my car" is so much more concise and elegant.

So, introduce vocabulary for abstractions that make sense in your application's context, and that provide more clarity for things that are otherwise hard to see.

  • If the two things you do inside the one or two iterations have a common abstraction, i.e. do something that can naturally expressed as one task with one name, go for a single iteration, with this one task (using the proper name) as the body.

  • If they are different things x and y, not serving a common purpose, keep them separate like do_x_with_agent(...) and do_y_with_agent(...). I'd prefer to have separate iterations then as well, allowing you e.g. to introduce names like do_x_for_all_agents(...) and do_y_for_all_agents(...) for the iterations.

0

So, you are stuck with two apparently equal choices :). Kind of things that may jam an engineer.

The way out is to obviously choose an option. No two things in world are exactly equal so there do exist a way out. Just choose the better option.

But how do you know which one is better? Other answers already ruled out performance concerns so there is only one parameter left - cognitive load - to concern.

When you put on socks and shoes which way do you go? Do you get done with one foot altogether then start the second foot? I doubt that. Most people put socks on both feet and only after that start putting on shoes.

The above however is deceiving. It shows preference of getting done with one object (foot) then moving on to next object. Its deceiving because we pick up pairs of socks in our hands so we go for getting done with this cognitive load, before loading the next cognitive load (pair of shoes).

In programming the heaviest cognitive load is the functions to performs. Once you have set up your loop you basically dont care how many objects you process. Dealing with 5 objects is no more a cognitive load than dealing with 5,000 objects as programming goes.

What you are most interested in, therefore focus on, therefore use most mental energy on is what you do with each object. Do you validate the objects before using their values? Should you do operation z as well as operations w, x and y?. These are questions you focus on. Not which objects you work on. It, after all, dont matter for schema rules how many rows are there in a table, to give an example.

Putting objects in a loop, iterating through each of them, running all the functions that have to be run on one of them and then only in next iteration on the next object is the simplest your code can be.

Its also the most "primitive" thing. What else can you do? Putting all your functions in some kind of higher-order lambda array and running loop on that? Dont it sound unnecessary complicated?

0

Mentally, I would first figure out which identical operations I need to perform on each object (I do that once), then the set of objects to operate on. The set of objects might be more complex than just a loop, so you don’t want to duplicate that.

The simplest code is:

<loop figuring out objects to operate on>
    <operations on that object>

Code complexity = complexity of the loop plus complexity of the operation. Better than complexity of the loop times number of operations, plus number of operations.

Now if you need three operations to figure out that you shouldn’t perform more operations on the same object, then multiple loops gets you into trouble. Use the loop with all operations inside.

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.