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

Simplification of the Fetcher class #1

Open
RonnieTaz opened this issue Jul 7, 2023 · 3 comments
Open

Simplification of the Fetcher class #1

RonnieTaz opened this issue Jul 7, 2023 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@RonnieTaz
Copy link

RonnieTaz commented Jul 7, 2023

Hello there, this is a beautiful project. I want to contribute to it and add a refactor that can wrap the fetcher and make it able to fetch from any school type without calling a specific school-type class.
Also, I want to add a feature that will enable returning the schools as an array/collection of objects, making it easier for consumers to consume the data.

My proposed code refactors will look like this:

$fetcher  = new Fetcher(SchoolType::CSEE, 2023);
@MegamindAme
Copy link
Owner

Hello,
Thanks for your appreciation.

The fetcher classes are already returning arrays of schools. in a format where the key is the school registration number. I don't know what else you meant by that.

About the proposed code refactors it would be very nice and easier for the users. The only thing you should consider in your implementation is how different the structures of the Primary results pages and secondary results pages are. The primary school pages are arranged by region and then district. The Secondary ones aren't arranged that way. So when using the PrimaryFetcher->getNodes(false) you can choose to pass false as an argument to get non-arranged nodes ( a single-level array) or leave it to get arranged school nodes the same way it is arranged on the website.

Consider also the possibility of adding results fetching from the returned schools. That is why there is a getNodes() function in all the classes that returns the nodes, and the user can fetch results link using the code
foreach($fetcher->getNodes() as $node){ $link = $node->getAttribute('href'); }

I think it can be worked out.

@RonnieTaz
Copy link
Author

Let me work on the proposed refactors. I'll open a pull request with some tests soon.

@MegamindAme
Copy link
Owner

I'll wait for it to continue with the development of the results fetching package.

@MegamindAme MegamindAme added the enhancement New feature or request label Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants