Hacker News Reader

Comment by dataflow | Hacker News Reader

> But the arguments given in support of the feature were generally fairly weak; they often used arbitrary, "made up" examples that demonstrated a place where multi-level break could be used, but were not particularly compelling.

> To make this proposal convincing, we need a realistic example of an algorithm that uses it, and that example needs to be significantly more readable and maintainable than the refactorings into functions, or the use of try…except (also a localised goto).

How about this:

  found = None

  for r, row in enumerate(table.rows):
    for c, cell in enumerate(row):
      if search_query.matches(cell.value):
        found = (r, c)
        break 2

  logger.log("Found cell: {}".format(found))
  return found

View on HN

EDIT: made a mistake, see my next comment bellow. I won't update this comment so that other readers can see the code for the 2 use cases.

Python has itertools.product and unpacking for this case:

    from itertools import product

    space = product(enumerate(table.rows), enumerate(row))
    for (r, row), (c, cell) in space:
        if search_query.matches(cell.value):
            logger.info("Found cell: (%s,%s)", r, c)
            return (r, c)

Note that we don't need the "found" variable neither to get the same beheavior. We can also use break + else, but in this case, the return is cleaner since that's the default value for python functions. Additionnally, the logger should do the interpolation, not format().

I think the Python team is having to do the difficult job of filtering new feature, and it makes sense they don't introduce it in this case. We already have all the facilities in the language to deal with most nested loop cases, and those are rare cases anyway.

Not in this case. 'row' is an item in the 'table.rows' iterable.

Right, I misread the code, my bad.

For this case, one should use a nested comprehension:

    space = ((r, c, cell) for r, row in enumerate(table.rows) for c, cell in enumerate(row))
    for (r, c, cell) in space:
        if search_query.matches(cell.value):
            logger.info("Found cell: (%s,%s)", r, c)
            return (r, c)

Or see masklinn comment if you don't need the logger.

In all languages, you separate code for preparing your input and code for processing it. In python, it's also a good idea to do that at the loop level:

- prepare your stream of data so that it's normalized using itertools, generators, a function, a numpy array, etc

- do a single loop pass for your processing

This way you will you obtain a single iterable, and will be able to get the most out of the language tooling:

- you can pass it to map/filter, itertools or another comprehension

- you can convert to a list, a tuble, a Counter, etc. And slice it

- you can use "else" in the loop, which is an underused construct, but very useful when you need break

- you can use advanced unpacking

- you can call iter() and/or next(). This is what masklinn does in his example.

The point being, nested loops are rare enough, and we have tool to deal with them in an idiomatic way in Python, and not adding new syntax for a rare case that usually has a good solution makes sense to me.

At that point you might as well make the entire thing into a gencomp inside a `next()` call, the explicit for loop is redundant.

Yes, if you don't need the logger, it is indeed cleaner.

In that case a flatmap does the job fine[0]. In fact you don't even need a flatmap:

    found = next((
        (r, c)
        for r, row in enumerate(table.rows):
        for c, cell in enumerate(row):
        if search_query.matches(cell.value)
    ), None)

Or just return at the location where you found a match. That way you can also provide a better error message in the failure case.

[0] well map + itertools.concat, IIRC the stdlib doesn't have a flatmap per-se, though more_itertools probably does.

That doesn't look to be "significantly more readable" than:

  def find_cell(table):
    for r, row in enumerate(table.rows):
      for c, cell in enumerate(row):
        if search_query.matches(cell.value):
          return (r, c)

  found = find_cell(table)
  logger.log("Found cell: {}".format(found))

It sure looks more readable/maintainable to me than if it was refactored into a function. Could you try refactoring it and showing what it would look like in your mind that would be similar in that respect?

They…they did show it refactored…

No, they edited their comment after I requested it without indicating that they did so, which makes me look dumb.

Sorry, didn't see your message. You're right I edited it after, but I didn't refresh the page to see your comment.

Apologies

Wouldn't you just write:

  found = next((
    (r, c)
    for r, row in enumerate(rows)
    for c, cell in enumerate(row)
    if search_query.matches(cell.value)
  ), None)

  def _find_cell(table, search_query):
      for r, row in enumerate(table.rows):
        for c, cell in enumerate(row):
          if search_query.matches(cell.value):
            return (r, c)
      return None

  found = _find_cell(table, search_query)
  logger.log("Found cell: {}".format(found))
  return found

Okay, so we managed to add cruft to the code without making it significantly worse. Now is this a change you would actually prefer as an improvement if someone asked you to review the original version?

It may be worth observing Gouvernathor does not exactly write "factored-out" code like this either: https://github.com/Gouvernathor/renpy-ParliamentDiagram/blob...

But not making it significantly worse is the bar requested:

> that example needs to be significantly more readable and maintainable

Yes, things can be slightly tidier with labelled breaks in some contexts. But that’s not a particularly high bar. There are tons of language features for which that’s true. Adding everyone’s pet nicety to the language makes it a huge language, which isn’t what they’re aiming for. If that’s what you want, go use Perl or C#, both of which are very fine languages.

> But not making it significantly worse is the bar requested. [...] Adding everyone’s pet nicety to the language makes it a huge language, which isn’t what they’re aiming for.

I might risk going on a rant here, but if the improvement from break-label is insignificant enough that it wouldn't meet that bar, I'm not sure how a ton of other features already in the language ever met it. Did for-else and while-else clauses (which somehow pretty much every other language on earth gets by without) genuinely meet this bar? How about the (pitfall-ridden!) match-case and walrus operator that they added quite recently? Did assignments-in-conditionals suddenly meet this bar in 2019 but not meet it in the decades prior where they refused to consider it compelling enough? What about annotations—couldn't you already do f = annotate(f)?

You're arguing mostly about cases where something was added two decades ago. The bar for new features should go up as time goes on. But importantly "it was a mistake to add for-else" is an argument to not add a similarly not useful and confusing feature.

Assignment expressions passed basically because there are around 2 common places where they clean up code. But unlike labeled break, those cases are relatively common. I dislike the walrus operator and don't think it was worth adding to the language, but I've still run into cases where it's been valuable. That's never been true for labelled break.

you're arguing against your own position now.

You are unimpressed with various marginal features that have been added but still think this other marginal feature is the special one that should be added.

In other words, the "but everyone else jumped off the bridge" argument is not especially compelling.

No, I'm saying their bar for adding things to the language (i.e. their "significance" threshold, which needs to be defined consistently somehow) seems to be lower in reality than your interpretation—or my default one, for that matter. I'm saying if I use the same threshold they utilize in reality, then yes, the improvement is very much "significant" enough to add.

> Okay, so we managed to add cruft to the code without making it significantly worse. Now is this a change you would actually prefer as an improvement if someone asked you to review the original version?

Added cruft? I don't think so.

The second version without the break is imho a clear improvement! It's pure functional code (ignoring the logging).

Compared to the imperative code which uses mutation of state defined outside of the function, which makes the inner workings of this algo even harder to follow, it's an obvious improvement.

Line count stays the same. Therefore I would in any case prefer the second, refactored version.

There may be cases where a multi-level break or even a goto would yield a better solution, sure. But those cases are rare and likely not present in Python at all as you do such things usually only to avoid a few machine code instructions; something that is irrelevant in Python as it's anyway slow, no matter what you do, so such optimizations would not make any sense at all.

There's a number of options here that I'd suggest: first would be that you're doing something wrong with the api of the table, it should be indexable as `table[r][c]` or `table[r, c]`, and you really should opt for that if at all possible, then this becomes super straightforward.

But if you really need this to work on an 2-d array that is only an Iterable, and somehow isn't Sized or Sequence, this still works:

    def flatten(table):
        for r, row in enumerate(table.rows):
            for c, cell in enumerate(row):
                yield (r, c, row, cell)

    def query(table):
        for (r, c, row, cell) in flatten(table):
            if search_query.matches(cell.value):
                return (r, c)

This process is generalizable, you can pretty much always separate the iterate and test steps by following this pattern.

(1) I dare say this is less readable than the original. Not un-readable, but definitely less readable. Just put them side-by-side if this isn't obvious.

(2) You probably don't want to call it something generic like "flatten" if it's going to do something so specific like concatenate table.rows. Note that I'm not just nitpicking on the name here. Rather, my bigger point is your factored-out functions aren't really as reusable as your name suggests. In this particular case you can trivially get around it by passing table.rows directly, but that obviously wouldn't generalize if you needed anything else in the table in the process.

> This process is generalizable

(3) I'd question that too. Imagine if you had a per-row operation too. Like maybe:

  found = None
  for r, row in enumerate(table.rows):
    for c, cell in enumerate(row):
      if search_query.matches(cell.value):
        found = (r, c)
        break 2
    logger.log("Finished row #{}".format(r + 1))

Are you really going to factor it out like this?

  def flatten(table):
    for r, row in enumerate(table.rows):
      for c, cell in enumerate(row):
        yield (r, c, row, cell)
    logger.log("Finished row #{}".format(r + 1))

(1) yes my way is clearer, side by side this is very obvious. Certainly it is not worse.

(2) sure, but this doesn't change anything. Factoring a big ugly function out into two smaller easy functions is...fine, even if each is only used once. The name I picked wasn't to imply generalization, it was because I needed a name.

(3) yes.

But again the whole reason this is unreadable at all is because you've created an intentionally obtuse api. We're returning the row and column, so clearly either were able to index into this thing normally, or it's a temp we've created, in which case we can modify it's api and index into it normally.

> But again the whole reason this is unreadable at all is because you've created an intentionally obtuse api. [...] we can modify its API

Do you never consume APIs you have no control over? And is every sequence you use an indexable list?

And no, there was nothing intentionally obtuse in my comment. It was incredibly straightforward and readable code. We obviously disagree, but regardless, I don't enjoy the bad-faith accusations, so I'll stop here.

> Do you never consume APIs you have no control over? And is every sequence you use an indexable list?

I already explained this. You're returning an index. There's no reason to do that unless.you can use the index. Somewhere, the api allowed you to use the index, so in this case, you've got control over the relevant parts of the api that don't!

> And is every sequence you use an indexable list?

Yes, or it's small enough that I can store or transform it into one, or it's big enough that I need to use specialized tooling that doesn't play well with break or continue.

> It was incredibly straightforward and readable code

I agree, there's nothing wrong with the code as written. It's fine. The obtuseness was the example. I'll reiterate: you're returning an index into some table. What are you going to do with that index, as you're claiming the table you pulled that index from cannot be indexed into for some reason? The code is fine, the example is obtuse.

Throw it in a function and return early:

  found = findIt(search_query, table.rows, logger)

  def findIt(query, rows, logger):
    for r, row in enumerate(rows):
      for c, cell in enumerate(row):
        if query.matches(cell.value):
          logger.log("Found cell: {}".format(found))
          return (r, c)
          break 2

    return None

You can use a generator expression:

  all_matches = (
    (r, c)
    for r, row in enumerate(table.rows)
    for c, cell in enumerate(row)
    if search_query.matches(cell.value)
  )
  found = next(all_matches, None)
  if found:
    logger.log("Found cell: {}".format(found))
  return found

It is lazy, so it stops as soon as the first match is found.

This doesn't generalize well. Add a statement or something in the loop and it clashes with this.

I suspect the Pythonistas would suggest something like the below is somehow better. I'm not confident in it's superiority, particularly since python doesn't really optimize.

  def searchRow(row,r):
    found = None
    for c, cell in enumerate(row):
      if cell.value == expected:
        found = (r, c)
        break
    return found

  found = None
  for r, row in enumerate(self.rows):
    found = searchRow(row, r)
    if found is not None:
      break
  logger.log("Found item: {}".format(found))
  return found

Reply on news.ycombinator.com