Feature/search optimisation refactor

wacl-york / mcm-web

Code for the MCM web application

1 stars 2 forks source link

Feature/search optimisation refactor #204

Closed stulacy closed 8 months ago

stulacy commented 10 months ago

This PR aims to refactor the search functionality for multiple reasons:

Improve code legibility and reuse by moving the main functionality from the routes folder into Modules in lib
Add unit tests and regression tests
Having the both 2 changes in place will make it easier to make changes to the search logic to speed up queries

This aims to address #176

stulacy commented 9 months ago

The search refactoring is complete. The speed gains are modest - a search for 'C2H4' is now 19% quicker returning in 0.055s rather than 0.068s, but large searches are still costly with 'CH' taking 0.49s in the old version and 0.47 in the new - but the biggest improvement is from the increased code legibility.

Not only is the code more structured than before by using Modules rather than having a massive routes file, but the search function itself has been refactored to use smaller descriptive functions. Unit tests have also been added to ensure that future changes don't unexpectedly alter this critical piece of functionality.

I'm not going to pursue further search optimisations at this current stage as I can't see any low-hanging fruit, but in the future these will be much easier to implement for the above reasons.

stulacy commented 9 months ago

@kilicomu I've finished refactoring the search code, hopefully you'll agree it's a lot more legible now! I've also added unit tests, which was much easier to do now the functionality is organised into modules. Unfortunately the search speed hasn't improved by as much as I was hoping for, but having the code better structured will make it easier to refactor this in the future.

kilicomu commented 9 months ago

Sure does look much nicer, and thanks for adding the tests.

Are you wanting a review?

stulacy commented 9 months ago

Now that it's actually (relatively) understandable, I would appreciate a review of lib/mcm/search/basic_search.rb as this is one of the core functionalities of the app. In particular I would welcome thoughts on the searching logic & ranking, and ideas for any potential optimisations.

stulacy commented 8 months ago

Merged PR, but would still welcome a review of lib/mcm/search/basic_search.rb.