sergiomrebelo / evo-poster

The Evolutionary Poster Composer (evo-poster) is a generative poster design system. From a given content (including text and imagery), evo-poster can generate and evolve poster designs considerating the aesthetic and semantics features of the content.
MIT License
3 stars 0 forks source link

Think about the Single Responsibility Principle #9

Closed JJ closed 1 year ago

JJ commented 1 year ago

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:

https://github.com/sergiomrebelo/evo-poster/blob/efaaaaa27ed2ca3c5365156507a9e893fc0ccc03/src/index.js#L72

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: https://github.com/sergiomrebelo/evo-poster/blob/efaaaaa27ed2ca3c5365156507a9e893fc0ccc03/src/index.js#L78 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.