search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.48k stars 2.83k forks source link

[Search v2.4] Refactor and simplify SearchUtils #50787

Open Kicu opened 3 days ago

Kicu commented 3 days ago

I wanted to suggest a piece of refactoring that could be done to simplify the code.

Problem

We can all see that some of the ***Utils files can grow to be much too big. Examples

In these files it is hard to comprehend what specific function does, and they stop having a single well defined responsibility. Developers tend to just clone existing functions and change them for their specific use case, instead of using functions that are already there, because nobody can reasonably understand the file with 2k lines.

In Search we were putting all the helpers into SearchUtils file, which has started to grow to over 1000 lines. However there is a lot of functions there that are unrelated to each other and there is no benefit in keeping them together in one file.

Solution

After reviewing all the functions inside mentioned utils, I noticed that they fall into 2 categories:

I would like to create a simple PR where I will split SearchUtils into two files:SearchUtils and SearchQueryUtils, each responsible only for the part described above☝️. There will be no common code shared between the two, these files can stay quite decoupled.

I hope thanks to this these files will be easier to work with and in the future.

CC @luacmartins @rayane-djouah can I get your opinion and if it's ok get the 👍 I expect this refactor to take no more than 2-3hrs and I have time to do it right now.

Platforms:

N/A

Screenshots/Videos

Add any screenshot/video evidence
Issue OwnerCurrent Issue Owner: @Kicu
melvin-bot[bot] commented 3 days ago

Triggered auto assignment to @zanyrenney (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

rayane-djouah commented 3 days ago

I agree, this will definitely improve our code structure.

luacmartins commented 3 days ago

Sounds good. Let's do it. As I mentioned in Slack, I think it'd be good to use more descriptive names for each file so people know what they are about, e.g. SearchUIUtils, SearchSyntaxUtils.

melvin-bot[bot] commented 3 days ago

Current assignee @zanyrenney is eligible for the NewFeature assigner, not assigning anyone new.

melvin-bot[bot] commented 3 days ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

melvin-bot[bot] commented 3 days ago

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

Kicu commented 2 days ago

PR ready, names that I chosen were SearchUIUtils and SearchQueryUtils because I believe that the name query is used a lot throughout the search code, whereas the word syntax is not used inside the code