Review of github.com/iamsalnikov/gorespect

iamsalnikov · February 4, 2018, 7:20pm

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!

Bebop_Leaf · February 6, 2018, 12:19pm

I have read through your code. Good job.

There is one concrete problem about codes that occurs in github.go, func (g *GithubRespecter) FilterRespectable

github.com

iamsalnikov/gorespect/blob/af74ee3edb48d51d024163a5e6d550c3f523ff89/github.go#L43


	Out    io.Writer
	In     io.Reader
}


// CanProcess func checks if we can work with this package
func (g *GithubRespecter) CanProcess(p string) bool {
	return strings.Index(p, githubHost) == 0
}


// FilterRespectable func returns packages which with we can work
func (g *GithubRespecter) FilterRespectable(pkgs []string) []string {
	pMap := make(map[string]bool)
	for _, p := range pkgs {
		p = g.normalizePackageName(p, true)
		if _, ok := pMap[p]; ok {
			continue
		}


		if g.CanProcess(p) {
			pMap[p] = true
		}

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.

iamsalnikov · February 6, 2018, 2:40pm

Thank you very much for review! I will reread it more careful and answer Thank you

iamsalnikov · February 7, 2018, 7:05am

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: https://play.golang.org/p/JgTyuJxzcPf ?

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 uses Config 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 like normalizePackageName, promptToken and promptUsername clearly does not belongs to GithubRespecter

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.

Bebop_Leaf · February 7, 2018, 11:52am

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.

iamsalnikov · February 7, 2018, 12:50pm

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.