PhpSpreadsheet
PhpSpreadsheet copied to clipboard
WIP Refactor Xls Reader
I have been having some time-out problems with php-cs-fixer in my environment (no problems yet in Github). Bringing that up with them, the first thing they suggested was that it might be due to very large modules. The largest module they use in testing is a bit over 1,000 lines, and we have about 16 that exceed that. The biggest of these is Xls Reader; at 7,647 lines, it is more than 2,000 lines longer than its nearest competitor (Calculation), and at least 5 times larger than fixer's max.
It's not clear to me that breaking it up will actually solve my problem. On the other hand, perhaps it is time to do some re-factoring anyhow; as an example, changing the parsing of tfunc/tfuncv values from enormous select statements to indexing constant arrays, possibly external, is something that ought to have happened long ago. This turned out to be easier than I had thought. Breaking it into sub-modules each of which can access each other's protected properties and methods was fairly straightforward. There's a bit of overhead in having to allocate new classes, but Xlsx Reader has been doing that all along (without the protected access part).
I've managed to remove about 2,900 lines from Reader/Xls, scattering those among 2 existing and 6 new source modules. I'm not sure I've chosen the best possible, or most maintainable, approach. I'm not sure that I'm done (there may be opportunities to move the parsing to its own module). But I do want something in place as a contingency. There's no need to rush it into production; I plan to leave this in draft status for a while, at least until after release 3.0.0.
This is:
- [ ] a bugfix
- [ ] a new feature
- [x] refactoring
- [ ] additional unit tests
Checklist:
- [ ] Changes are covered by unit tests
- [x] Changes are covered by existing unit tests
- [ ] New unit tests have been added
- [x] Code style is respected
- [x] Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
- [ ] CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
- [ ] Documentation is updated as necessary
Why this change is needed?
Provide an explanation of why this change is needed, with links to any Issues (if appropriate). If this is a bugfix or a new feature, and there are no existing Issues, then please also create an issue that will make it easier to track progress with this PR.