Hi!
I returned to learning Go. And I wrote gorespect as my learning project. I want to ask community to review my code:
If you have some suggestions I will be glad to see it.
Thank you!
Hi!
I returned to learning Go. And I wrote gorespect as my learning project. I want to ask community to review my code:
If you have some suggestions I will be glad to see it.
Thank you!
I have read through your code. Good job.
There is one concrete problem about codes that occurs in github.go, func (g *GithubRespecter) FilterRespectable
It seems to me you can do the filter within a loop, just add append
to where you set the map true. And you can just use the value of pMap[p]
, since it is false
in default, so you don’t need to use the two-value-assignment.
I have other thoughts about designs, and they are heavily opionion-based.
First, I would complain about your config
, both the data structure and the code. I understand the urge to keep the config flexible and hence making the structure dynamic by using map[string]interface{}
, but I would argue that in a simple app like this, it is very unlikely that config structure would change, and using map[string]interface{}
and wrapping around it is ugly, slow, and in-expressive; it also makes API hard to understand and find.
I would suggest that it is best to just make a strcut
with fixed fields (and types) to present the config, making the code much clearer.
And about GithubRespecter
uses Config
as a field, I also advice against it. In that way, what the GithubRespecter
is really interested in (like username
, usertoken
) is hidden beneath the Config
, which reduce its expressiveness. I would suggest you just putting all the required params into the type, as fields.
You may argue that in standard libs, especially net
packages, there are plenty of function and structure use a config
as param, but it is different. Most of those use case accepts a specified config, like, TLSConfig
, which is pretty clear what they contain and what is required. However, here, the Config
struct shall serve as a whole package level config, though it might not be implented yet. Consider the possibility that in the future you let the config specify an i18n option which has nothing to do about the core logic of GithubRespecter
, it would be more obvious that it does not make that much sense to pass the whole config into the GithubRespecter
.
Another part I want to complaint is about the one-in-all type GithubRespecter
. Though it works with no trouble and is really tiny in terms of lines of codes, I would consider it too big as it violates single resposiblity principal. Methods like normalizePackageName
, promptToken
and promptUsername
clearly does not belongs to GithubRespecter
. Even CanProcess
is somehow devised from the core logic, and SetUp
seems more like a process that should be handled by the main
func.
I would make a type PackageName string
and put CanProcess
and normalizePackageName
under its responsiblity. I would also move the promptUsername
and promptToken
into Config
as they seems to me as a part of getting config, just not from the config file but from the user.
I think that is all of it. Please don’t get discouraged by it. You did an awesome job to create an interesting app, not to mention as your first go project.
Thank you very much for review! I will reread it more careful and answer Thank you
There is one concrete problem about codes that occurs in github.go, func (g *GithubRespecter) FilterRespectable
Do you mean function body like that in your comment: Go Playground - The Go Programming Language ?
First, I would complain about your config, both the data structure and the code
I completely agree with you on that point. I am going to fix it. I am going to extract config to Config
structure with fixed fields and ConfigStorage
structure which will load and save config structure.
And about
GithubRespecter
usesConfig
as a field, I also advice against it
…
I would suggest you just putting all the required params into the type, as fields
I agree with you on that point. I will fix it.
Another part I want to complaint is about the one-in-all type
GithubRespecter
…
Methods likenormalizePackageName
,promptToken
andpromptUsername
clearly does not belongs toGithubRespecter
I thought that we could have another respecters, for example: GitlabRespecter
, BitbucketRespecter
. I agree with you about func normalizePackageName
in future. But if we followed to logic about different respecters, then we would see that they can have different SetUp()
process (it calls promptToken
and promptUsername
). That is why I included it to GithubRespecter
.
But now I think about making respecter’s constructor, which will include the logic of method SetUp
and returns ready to usage respecter.
@Bebop_Leaf, thank you for your review! You really helped me.
Yes.
Fair point. But I still think while it is related to the type, it is a user interface’s responsibilty to check and get params. Consider that the app is going to have another frontend, maybe a GUI, or maybe another config file via some options (not quite the UI in the tradional sense, but it is auctually the same thing), you may want better control on the data, like when to prompt (or notify, alert, whatever) the user to enter data.
A pattern I come up for now is to imitate the standard lib flag
: Register params needed for each respecter and let the UI part to handle the registered params.
And as you mentioned the possibility of having more respecters, I think even FilterRespectable
is out of a respecter’s resposnibilty. Instead, I think it is proper to have a filter (or router, more accurately) function or type instead, to filter all the packages to its respecter, once and for all. This way, you can also deal all “active” respecters’ setup together.
Yes, you are right. I didn’t think about it in this way. I see it now.
I have to think about it. Thank you! You gave me great thoughts.