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

Think about the Single Responsibility Principle #9

Closed
JJ opened this issue Jan 23, 2023 · 0 comments · Fixed by #11
Closed

Think about the Single Responsibility Principle #9

JJ opened this issue Jan 23, 2023 · 0 comments · Fixed by #11
Assignees
Labels
enhancement New feature or request

Comments

@JJ
Copy link
Collaborator

JJ commented Jan 23, 2023

In general, SOLID is a, well, solid methodology for software engineering. There are others, but this one contains a good set of principles to work on. The S is the single responsibility principle. It basically says that a function, class, module should do a single thing. This allows for loose coupling, independent testing, but a better "defensive" programming that saves you trouble when you have to refactor (which you will, sooner or later)

This:

const analysis = async (text, lang, sentences = []) => {

has several issues:

  1. Generally, having a default value for an argument is not a red flag. It is, however, if there's an if inside and it changes behavior. Here:
    if (sentences.length === 0) sentences = (await NLP.sentenceTokenizer(text)).flat();

    you're calling sentenceTokenizer. That means that it's actually a flag to work in a different way; it will tokenize the sentence or not depending on that. It's got two responsibilities then: classify, and tokenize.
  2. sentences has two responsibilities. If empty, it's a flag that will tell you wether to tokenize or not. If not, it will skip some functionality.
  3. Worst offender however is to make this return HTTP status. An analyzer should analyze. It will either work or not. If it does not work, it should throw an Error. That Error will be captured at a higher level, and interpreted in terms of the intended API, either REST or otherwise.
  4. The fact that you put everything in a single file would go hand in hand for worst offender status. This handles the routing, functionality, as well as the underlying analysis functionality.

As I said in #3, every issue should describe a problem. This describes a very general problem indeed. It's up to to you to decide on the solution, either piece-wise (focus on the file, functions mentioned) or general (think about user journeys, user stories, and how to start working on this specific funcitonality -- analysis -- responding to user's needs).

We can talk about it tomorrow anyway.

@sergiomrebelo sergiomrebelo added the enhancement New feature or request label Jan 26, 2023
@sergiomrebelo sergiomrebelo self-assigned this Jan 26, 2023
@sergiomrebelo sergiomrebelo added this to the Textual Analysis Module milestone Jan 26, 2023
sergiomrebelo added a commit that referenced this issue Jan 26, 2023
sergiomrebelo added a commit that referenced this issue Jan 26, 2023
sergiomrebelo added a commit that referenced this issue Jan 26, 2023
sergiomrebelo added a commit that referenced this issue Jan 28, 2023
sergiomrebelo added a commit that referenced this issue Jan 28, 2023
sergiomrebelo added a commit that referenced this issue Jan 28, 2023
sergiomrebelo added a commit that referenced this issue Jan 29, 2023
sergiomrebelo added a commit that referenced this issue Jan 29, 2023
sergiomrebelo added a commit that referenced this issue Jan 29, 2023
@sergiomrebelo sergiomrebelo linked a pull request Jan 29, 2023 that will close this issue
sergiomrebelo added a commit that referenced this issue Jan 29, 2023
@sergiomrebelo sergiomrebelo reopened this Feb 2, 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

Successfully merging a pull request may close this issue.

2 participants