21jake / nimble-scraper

4 stars 0 forks source link

[Chore] Improve source code organization #38

Closed longnd closed 1 year ago

longnd commented 1 year ago

Issue

While the application code is organized with several services, some of them violate the single responsibility principle. for example: https://github.com/21jake/nimble-scraper/blob/f0673eb1420deabbd48ad2491bc10633254e908a/backend/src/services/file.service.ts#L41-L65

This function name (`saveBatch) is misleading as it do many thing:

Shouldn't it be better to separate the part of initializing the Batch entity (and persist it into DB) and fetching for updated Batch into their own functions?


or, in the scraper.utils.ts, putting the pickLeastUsedProxy() function and SinglePageManipulator class in the same file isn't appropriate

https://github.com/21jake/nimble-scraper/blob/f0673eb1420deabbd48ad2491bc10633254e908a/backend/src/utils/scraper.utils.ts#L21-L40

the class SinglePageManipulator itself isn't utility, it has few functions to extract the search result from a page.

could you share some insights why you decided to organize the function and the class under the same file?

21jake commented 1 year ago

This function name (`saveBatch) is misleading as it do many thing

You're right about this. I should've spent more time on refactoring. A handleFileUpload() should call each function respectively. I will make a chore/refactor branch to try getting everything organized.

Could you share some insights why you decided to organize the function and the class under the same file?

  1. The file name is scraper.utils.ts and it has the purpose of providing utilities solely for the scraper logic. The class SinglePageManipulator is an utility class helping scraped page extract required info. It makes sense to place the class within the scraper.utils.ts file.
  2. The class SinglePageManipulator is solely used for scraping purposes. It's not used within other services, let say FileService or AuthService. It's appropriate to centralized scraping utilities into one scraper.utils.ts file.

Can you elaborate on why it's not appropriate to put utility function and utility class within a same utility file? Because it honestly sounds more of a personal preference.

longnd commented 1 year ago

thank you for spending the effort on the fix.