Hacker News Reader

Comment by joshuamorton | Hacker News Reader

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.

View on HN

(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.

Reply on news.ycombinator.com