Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor method CSV #22

Open
JJ opened this issue Dec 31, 2019 · 6 comments
Open

Refactor method CSV #22

JJ opened this issue Dec 31, 2019 · 6 comments

Comments

@JJ
Copy link
Collaborator

JJ commented Dec 31, 2019

Not that we should clean-code it to 15 lines, but it's 400 lines long, lots of decisions in there, we should factor out common code and do a multi on $in, at least. Otherwise it's going to be almost impossible to fix stuff like #21
This might include also totally eliminate method csv, which only checks, and deletes, an arg, and does some stuff with meta which I don't really understand. Lots of is copy, too, which are really unnecessary.
That arg, also, should probably get out of the *%args pile and get a definition on its own.

@Tux
Copy link
Owner

Tux commented Dec 31, 2019

There are several reasons for not refactoring:

  • Easy to port from perl5 Text::CSV_XS
  • Keep the code unchanged so the speedometer will be of value: if you change the code to be more effective or faster while refactoring, you might gain speed and get different numbers. (I know, not good enough a reason to not refactor)
  • I do not have any problems with bigger code chunks (at all). I rather have a 400 line block of code that I understnd than 400 different files with the code scattered all around just because someone likes smaller code chunks. This is why I do not like Java, where it is common practice

I heard from @lizmat that there are plans to try to optimize next and continue where possible (them not being exceptions than). The code as is would be a wonderful demonstration of how that speedup works out.

Note that CSV parsing is a state-machine, and with all the options I support, getting "common code" out is harder than you think.

If there are things you don't understand, ask! I might be completely wrong too :)

@JJ
Copy link
Collaborator Author

JJ commented Dec 31, 2019 via email

@Tux
Copy link
Owner

Tux commented Jan 2, 2020

At the moment I disagree: Text::CSV_XS and Text::CSV are written for performance. Losing performance is a big deal. Usability is on the other side of the API. Evolvability is something else, and I seriously doubt if there are may new ideas floating around in adding new features to Text::CSV, but please prove me wrong.

@JJ
Copy link
Collaborator Author

JJ commented Jan 2, 2020 via email

@Tux
Copy link
Owner

Tux commented Jan 2, 2020

Feel free to create a branch en see if there is noticeable speed-loss.
I know it will be hard, but please try to keep my style.

@JJ
Copy link
Collaborator Author

JJ commented Jan 2, 2020 via email

@JJ JJ added the enhancement label Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants