Hi Eugene

[fabriziomoscon]

Thank you for submitting the test.

I think it is a quite good submission and I like many part of it. As we would like to make as much as the interview as possible offline, could you please find some time to address a fe concerns that I have?

I understand you have worked with Hexagonal architecture, which is great! At 90poe we bundle together the following patterns:

• hexagonal for the code organisation and testability
• DDD for naming and semantic code structure
• clean code and SOLID for enhanced code maintainability

I don't quite understand why you are using multiple subpackages. I think that this exercise is small enough to simply have:

• internal/config
• internal/core or domain
• internal/repository/port
• internal/transport/http
  I would suggest to think about placing all models and ports inside the domain package, in this way other packages will depend on it, and it can contain all your hexagonal business logic.

To follow DDD and help to differentiate each service from each other we don't use terms like model, adapter, port, common, util, etc. Instead we use names of things as much as possible like in this case MaritimePort (or port), for all the other variables their place should make clear if the are ports (interfaces) or adapters (implementations). Also you should find a home for utils and common code.

I appreciate your knowledge of CQRS, but I would say that it is overkill for this exercise.

It seems that you use pointers returns and pointers receiver many times, even if you don't need to mutate the received, so I would advise to consider returning values and using value receivers whenever you can, this will help avoid nil pointer checks as well as avoiding memory escaping to the heap.

Please let me know if this makes sense, and if you would like to have inline comments in the PR to clarify some of the points.

Thank you again for taking the time for interviewing with 90poe.

Fabrizio

Chief of Engineering

[evt]

Hi Fabrizio,

I appreciate your feedback. Currently, I have a busy schedule and cannot provide immediate attention. But no need for inline comments. I'll address your concerns today in the evening and reply then.

Cheers
Eugene

[evt]

I don't quite understand why you are using multiple subpackages. I think that this exercise is small enough to simply have:

• internal/config
• internal/core or domain
• internal/repository/port
• internal/transport/http

I was trying to make as as a mono repository ready for extension so that new apps with own domain, services and adapters could be added into the same internal directory as internal/users, internal/vessels, internal/commodities (and internal/app renamed then to internal/ports). Anyway, I see what you mean, will make necessary changes.

I would suggest to think about placing all models and ports inside the domain package, in this way other packages will depend on it, and it can contain all your hexagonal business logic.

We have at least 3 different layers with own models: domain, adapter (=repository) and API. The same port model can have different fields in different layers or the same fields but with different data types. Thus, we have converters from one layer to another for the same model. So, I'm not sure what you mean by placing all models into domain package, sorry, would be great to clarify or may be schedule a call and discuss it.

To follow DDD and help to differentiate each service from each other we don't use terms like model, adapter, port, common, util, etc. Instead we use names of things as much as possible like in this case MaritimePort (or port), for all the other variables their place should make clear if the are ports (interfaces) or adapters (implementations). Also you should find a home for utils and common code.

I appreciate your knowledge of CQRS, but I would say that it is overkill for this exercise.

yeah, it was :) I'll simplify

It seems that you use pointers returns and pointers receiver many times, even if you don't need to mutate the received, so I would advise to consider returning values and using value receivers whenever you can, this will help avoid nil pointer checks as well as avoiding memory escaping to the heap.

no prob, will do

[fabriziomoscon]

Thank you for your answers.

We have at least 3 different layers with own models: domain, adapter (=repository) and API. The same port model can have different fields in different layers or the same fields but with different data types. Thus, we have converters from one layer to another for the same model. So, I'm not sure what you mean by placing all models into domain package, sorry, would be great to clarify or may be schedule a call and discuss it.

Good answer, we usually have code struct tags for helping with validation from API and marshalling into repository, but this is only possible when the fields coincide (like in your case), if the input contains more fields than needed in core, then a mapping is placed into the transport package to make a domain object from the input, with validation, same from the right side of the hexagon.

[evt]

@fabriziomoscon okay, I've removed commands, queries and adapters. New structure is as follows:

• internal/app holds the application
• internal/domain includes domain models
• internal/repository/inmem holds all in-memory repositories including the port repository. I specifically didn't add another subpackage level (internal/repository/port/inmem or internal/repository/inmem/port) as it would make things complicated from my POV. All in-memory packages can be on the same level along with errors and utils if any needed in future.
• internal/services holds all services (port service only for now)
• internal/transport holds all transports (http server only for now)

HTTP server has the app injected:

type HttpServer struct {
	app app.Application
}

App has now the port service injected:

type PortService interface {
	GetPort(ctx context.Context, id string) (*domain.Port, error)
	CountPorts(ctx context.Context) (int, error)
	CreateOrUpdatePort(ctx context.Context, port *domain.Port) error
}

type Application struct {
	Service PortService
}

and port service has the port repository injected:

type PortRepository interface {
	CreateOrUpdatePort(ctx context.Context, port *domain.Port) error
	CountPorts(ctx context.Context) (int, error)
	GetPort(ctx context.Context, id string) (*domain.Port, error)
}

type Service struct {
	repo PortRepository
}

Port repository accepts and returns pointers to the port domain models. I specifically left the pointers as otherwise it would be too many stack copies (port model is already not a small one) even though they escape to the heap (final decision can be made after benchmarks added). Plus we have have portHttpToDomain, portDomainToStore and portStoreToDomain utilities anyway that convert one layer's model into another with a proper slice-append technique to make sure underlying array is not shared.

Also worth mentioning that port in-memory repository uses data map with pointers (not structs) otherwise we wouldn't be able to update them later.

NewPortService returns a struct, NewApplication returns a struct aswell, while inmem.NewPortStore returns a pointer as sync.RWMutex can't be copied.

P.S. I also changed data type for port alias and regions from []any to []string. It was []any before because I didn't have even a single example of port with alias/regions filled from ports.json provided, thus didn't know if it was string or int or anything else but then realised most likely they should be strings.

[fabriziomoscon]

Nice, thank you. This makes a lot of sense to me now.
Last comment (that does not need to be addressed), app does not do anything, we don't have this concept, and we consider the service part of core and it is the app.

[fabriziomoscon]

Our policy for pointers is not primarily rooted into performance, but in building robust software that is not prone to nil pointer exceptions. We use pointers only when we need to modify the values and for mutex, in all other cases when we deal with simple variables and struct values are safer. You are right that performance consideration should be done with a benchmark test.

[evt]

Nice, thank you. This makes a lot of sense to me now. Last comment (that does not need to be addressed), app does not do anything, we don't have this concept, and we consider the service part of core and it is the app.

removed the app and injected port service directly into http server :) bfcd238

[evt]

Our policy for pointers is not primarily rooted into performance, but in building robust software that is not prone to nil pointer exceptions. We use pointers only when we need to modify the values and for mutex, in all other cases when we deal with simple variables and struct values are safer. You are right that performance consideration should be done with a benchmark test.

I see. Well, I'd say even with structs you may end up in unexpected behaviour. It wouldn't be a panic but some missing struct value instead. Worth aiming for 300% test coverage ;) 100% unit tests, 100% integration tests and 100% e2e tests :)