4 ms·

(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) Yo

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

joshuamorton 4y ago
(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.
- dataflow 4y ago
  > 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.
  - joshuamorton 4y ago
    > 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.

joshuamorton 4y ago

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

dataflow 4y ago
> 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.
- joshuamorton 4y ago
  > 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.