search

muesli4 / table-layout

Layout data in grids and pretty tables. Provides a lot of tools to get the cell formatting right (positional alignment, alignment on specific characters and limiting of cell width)..
BSD 3-Clause "New" or "Revised" License
38 stars 11 forks source link

Minor cleanups and efficiency improvements. #22

Closed Xitian9 closed 3 years ago

Xitian9 commented 3 years ago

Avoid reversing strings twice.

muesli4 commented 3 years ago

Those are nice improvements. However, I would like that reflected in the commit message. For example:

Improve performance of Cell and StringBuilder instances

I've been following a specific style in my later commits. The title starts with a verb in infinitive form and does not end with a dot. If you want to specify details you are encouraged to do so (although preferably for complex commits) after a blank line.

muesli4 commented 3 years ago

I'm not sure whether the improvement for dropRight is actually faster. And after all, most strings are relatively short, i.e. one could consider this premature optimization, not that performance matters anyway with String or we are printing tables a lot. ;)

Xitian9 commented 3 years ago

I'm happy to drop this PR if you want. They were just things I noticed as potential (and potentially small) improvements. I think you're likely correct that there's little to know real performance difference in most cases.

Xitian9 commented 3 years ago

I presume you mean imperative form rather than infinitive? (Infinitive would be “To improve performance of Cell and StringBuilder instances”).

Xitian9 commented 3 years ago

The advantage of this dropRight here is that it avoids reversing the list twice, and is lazier. In fact, this version will actually work on infinite lists. How much of a benefit this is is questionable, but I believe it's strictly better. (See discussion here https://www.reddit.com/r/haskell/comments/f072u/sample_solution_to_an_early_project_euler_problem/c1ceny6?utm_source=share&utm_medium=web2x&context=3)

muesli4 commented 3 years ago

I'm happy to drop this PR if you want. They were just things I noticed as potential (and potentially small) improvements. I think you're likely correct that there's little to know real performance difference in most cases.

No, I like it.

I presume you mean imperative form rather than infinitive? (Infinitive would be “To improve performance of Cell and StringBuilder instances”).

Yes, my bad.

In fact, this version will actually work on infinite lists. How much of a benefit this is is questionable, but I believe it's strictly better.

I guess then length would be our next problem. But I found the argument with constant space convincing.