You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The fact there's an And in the function name immediately flags that this should be split. Looking at it deeper, the first part of the function is just a selection of strings, and the actual work of the function is two lines. Similarly, only one of these groups of lines is ever used more than once.
I can kind of understand want to have a single place to abstract error reporting, but I don't think this is really necessary. Given that this is only even used in three places, that also indicates this is probably needless abstraction. I could still see an argument for making it easy to write and reference our error reporting strings, so it might make sense to just define some struct like this, and define a few of these at the top of some file:
Then you could go to the three places this function is currently used and replace it with the following, which avoids us falling into the trap of constraining ourselves to an abstraction format we might not consistently need:
If you agree with this assessment, this can also come in as a future PR from you or someone else. And don't take these examples as design, this was just off the top of my head. If you don't, that's valid, let's hear why.
The fact there's an
And
in the function name immediately flags that this should be split. Looking at it deeper, the first part of the function is just a selection of strings, and the actual work of the function is two lines. Similarly, only one of these groups of lines is ever used more than once.I can kind of understand want to have a single place to abstract error reporting, but I don't think this is really necessary. Given that this is only even used in three places, that also indicates this is probably needless abstraction. I could still see an argument for making it easy to write and reference our error reporting strings, so it might make sense to just define some struct like this, and define a few of these at the top of some file:
Then you could go to the three places this function is currently used and replace it with the following, which avoids us falling into the trap of constraining ourselves to an abstraction format we might not consistently need:
If you agree with this assessment, this can also come in as a future PR from you or someone else. And don't take these examples as design, this was just off the top of my head. If you don't, that's valid, let's hear why.
Originally posted by @jarrpa in #7 (comment)
The text was updated successfully, but these errors were encountered: