Some violations of CodeReviewComments

[spy16]

1. Repeatation
   • Package helloworld contains a function NewHelloWorld.
   • So the client would initialize the HelloWorld type as helloworld.NewHelloWorld which is simply repeatation of the fact that helloworld package contains a HelloWorld and is not recommended Package Names
   • This should instead be helloworld.New()
2. Interfaces
   • Package helloworld contains an interface HelloWorld and function NewHelloWorld returns this interface type instead of concrete type. Interfaces on implementor side as an abstraction has no meaning
   • Package helloworld should contain a concrete type HelloWorld which is returned by helloworld.New function as recommended here
   • Clients who need to use features of HelloWorld type can define interfaces with only the features they really need and inject instance of HelloWorld as an implementation.

[sagikazarmark]

Hey @spy16,

Thanks for the review.

his should instead be helloworld.New()

Yeah, this sounds right. Actually, the naming in this package as a whole seems to be quite unfortunate, so I will probably work on that, maybe find better examples.

Interfaces on implementor side as an abstraction has no meaning

You are totally right. The reason why I chose this though is that I'm trying to implement Clean Architecture where the input boundary interface is defined by the use case layer. Since the interface refers to other interfaces/concrete types, the consumer side cannot avoid importing the package.

Also, I tend to avoid exporting structs with unexported fields, because it leaves it open to invalid initialization. But golint complains when exported methods return unexported types. I haven't been able to resolve this conflict of interest to be honest. Is there an official recommendation here? Is the risk of invalid initialization of an exported struct negligible? I tend to be defensive, when it comes to that.

I will try to work on this issue as well, if you have any insights to the problem above, I'm happy to hear them.

Thanks for the review again!

[spy16]

Main rule of Clean Architecture is that dependencies always point inward. Any dependencies outward should be done through interfaces.

So if a presentation layer (e.g. REST API for HelloWorld) requires to use a domain entity (i.e., HelloWorld), then it is free to do so by directly referring to HelloWorld struct or by creating an interface. But if inner layer (i.e., helloworld package) needs to use something from outer layer (e.g., database) then, helloworld should define an interface and the actual implementation should be injected later. In this particular project, only external component helloworld has is the logger and is being imported from go-kit directly which actually violates Clean Code. Instead, helloworld should define a Logger interface and the go-kit logger should be injected as a dependency from an external layer (most likely main.go)..

In other words, this particular recommendation for interfaces does not really violate Clean Code in anyway. It actually enforces it.

Also, interfaces recommendation reduces decoupling. If helloworlddriver had defined an interface HelloWorld and helloworld package had a concrete type DefaultHelloWorld then

• helloworld package is independent of helloworlddriver package
• helloworlddriver package is also independent of helloworld package
• only the outermost layer (i.e., main.go) will have to worry about injecting DefaultHelloWorld into objects inside helloworlddriver package

About invalid initialization of unexported fields: this was a concern for me as well. But what i have seen is, the convention of having NewX is a well-known one and generally developers with considerable golang knowledge will avoid using structs directly if there is a New function. Also, proper unit-tests help there too.

[sagikazarmark]

So if a presentation layer (e.g. REST API for HelloWorld) requires to use a domain entity

Actually, it's not that simple. Although there is nothing that permits doing that, it's a bad practice to use the domain entity outside of the use case layer.

In this particular project, only external component helloworld has is the logger and is being imported from go-kit directly which actually violates Clean Code

While this may be technically true, it is not practical not to import the go-kit logger here. The package provides a number of helpers which are used by the implementations (like level package). Building a full boundary around the logger would require a large amount of code for literally no advantage. The go-kit logger package/interface is quite small and stable, so I'm comfortable with importing it. This is where common sense should be applied IMHO.

In other words, this particular recommendation for interfaces does not really violate Clean Code in anyway. It actually enforces it.

Sure, my problem is rather with the struct exporting.

Also, interfaces recommendation reduces decoupling. If helloworlddriver had defined an interface HelloWorld and helloworld package had a concrete type DefaultHelloWorld then

I tend to avoid the word default in names: it does not mean anything, doesn't help understand what the thing is.

helloworlddriver package is also independent of helloworld package

Actually, this isn't true. Even if the interface is defined in the driver package, it still depends on the interfaces and structs defined in the use case package.

About invalid initialization of unexported fields: this was a concern for me as well.

I wish Go had a tool to enforce initialization compile time, otherwise this is always going to be a risk. The worst thing is that preventing this error to affect you requires full e2e tests, because it only causes issues runtime.

[spy16]

Actually, it's not that simple. Although there is nothing that permits doing that, it's a bad practice to use the domain entity outside of the use case layer.

I actually meant Use Case layer here. Apologies. But the point is, there is no reason not to point inward. But avoiding pointing outward is the only rule in Clean Architecture.

While this may be technically true, it is not practical not to import the go-kit logger here.

I agree that this is an opinionated and use-case based decision.

type Logger interface {
      Debugf(msg string, args ...interface{})
      Infof(msg string, args ...interface{})
      Warnf(msg string, args ...interface{})
      Errorf(msg string, args ...interface{})
       
      // WithFields is for annotated loggers
      WithFields(fields map[string]interface{}) Logger
}

I prefer having above interface over importing log package, because:

• WithFields provides structural logging option where necessary while maintaining a simple interface.
• When it comes to log levels, generally levels are configured at application level not at the log statement level. So, consumers of Logger implementation can safely assume that the logger they are getting is already configured appropriately with proper log levels.
• If the gotkit/log package changes a contract my innermost core layers do not need to change which is the whole point of Clean Architecture (i will still be able to create a wrapper around gokit logger and implement Logger once and inject it everywhere instead of changing code in all the different layers. [e.g., since go-kit/log is specifically designed for gokit micro-services, what if something changes in this package to incorporate a requirement specific to gokit framework.?])

I tend to avoid the word default in names: it does not mean anything, doesn't help understand what the thing is.

Agreed. That was just an example name to avoid confusion between HelloWorld interface and HelloWorld implementation. But same goes for helloWorld and HelloWorld. :)

Actually, this isn't true. Even if the interface is defined in the driver package, it still depends on the interfaces and structs defined in the use case package.

That depends. It should not depend on an interface defined in use-case layer. It might depend on structs which are just domain entities to represent data. But then again, this generally means you have distinct entities layer and use case layer which is a different scenario. (Explained under What data crosses the boundaries. heading in the above link)

I wish Go had a tool to enforce initialization compile time, otherwise this is always going to be a risk.

Agreed.

[spy16]

Let's say helloworld package had 2 clients A and B. Client B requested for a feature called SayReverse in addition to already existing SayHello feature. You added this to helloWorld struct. But since this struct is unexported, client B will still not be able to use the added feature. How do you solve this ? If you thought you could add SayReverse signature to HelloWorld interface, you would be violating Interface Segregation Principle, also possibly breaking client A. (because, even though, client A doesn't need the new feature it's going to be forced since it's importing your interface instead of defining an interface matching it's requirements)

This is where Go's structural typed interfaces shine because, client can define interfaces anytime it wants without really breaking anything since implementations are truly independent of any interface name. (i.e., no implements X in implementation)

That's why the recommendation Accept interfaces, return structs

[sagikazarmark]

Okay, first round is here: #17

[sagikazarmark]

Here is round two: #18

[sagikazarmark]

I was looking at the Logger interface you posted. Actually I had such interface a while back and I realized why I dropped it in the end:

      // WithFields is for annotated loggers
      WithFields(fields map[string]interface{}) Logger

Because of this line you either have to have an adapter for each interface or have one single, global interface with a single adapter.

I believe the first option simply isn't worth the effort.

The second option kinda defeats the whole purpose. It can protect you from API changes of third-party libraries but from the interface point of view it's the same. That being said, I might just add that global interface anyway, to separate the application from go-kit, but right now the implicit interface doesn't help anything in this case.

[spy16]

Yes. That is true. This is a trade off usually. Either you can choose not to have structured logging and remove WithFields , in which case logger interface is already compatible with many logging libraries. If you require structured logging, then adaptors become a necessary. But in my experience, library of Choice for logging doesn't really change. So a single adaptor for that is enough. The main advantage of this is just decoupling external library from your core logic.

Imagine changing go-kit logging statements across the codebase vs changing one global adaptor which is wrapping gokit logging functions.

[sagikazarmark]

Here is an experimental PR, but I'm gonna stare at it for a while to decide whether I like it, before I merge it: #19 🙂

[sagikazarmark]

I decided to merge the PR. It looks fine, time will tell if it works well.

Thanks for your comments @spy16 again, I think all of them were addressed!

[spy16]

@sagikazarmark thanks for considering the suggestions.

I have been planning to setup a sample project myself. My idea was to build a full blown application ( droplets ) which could be used as a reference project by gophers. But wasn't able to actively work on it due to time constraints. So kudos for your effort.

[sagikazarmark]

I'm planning to add some more examples alongside the greeting one.

[spy16]

That would be good. Also, might be a good idea to add godoc and goreportcard badges.

[sagikazarmark]

Good idea, created an issue #20 for that

[spy16]

Everything from this discussion has been addressed.. closing this.

[sagikazarmark]

Thanks again @spy16