zalgonoise / eljoth-go-code-review Goto Github PK

The service interface is being declared in the API directory (which is responsible for the transport-layer). It should be moved to the internal/service directory (in service.go).

This raises an issue with the types declared in internal/service/service.go considering that type Service struct already exists. To keep the standard of accept interfaces, return structs, that Service struct would be renamed to CouponService.

Like the title implies, not initializing the map will cause a runtime panic (taking into account that the map is never initialized, elsewhere)

• As covered in #6, the request-specific objects can be declared within the HTTP endpoint methods. In this case, the CouponRequest type can be declared within the function body. Besides this, it's a good opportunity to distinguish the type names a bit better.
• Also, when the call errors, the client does not get a response. An appropriate response message and code should be written before the return.

• The type is storing both the config and the multiplexer (as CFG and MUX fields) while they are only used to initialize the server. As such, they can be consumed in the New function and not necessarily be kept.
• It's not exactly the best practice to name a struct element something in all-caps.

The Coupon Repository interface is redeclared in this package. Like in #8, it should be removed from this package in lieu of a internal/discount package.

• There is no point in accepting a Basket type and then returning a pointer to it. Since the method will mutate the elements of the Basket, it should just accept a pointer of a Basket, returning an error only (ApplyCoupon(basket *Basket, code string) error)
• Named returns make it really confusing on this (initial) design. It's best to remove them.
• Returned error from the repo.FindByCode call should ideally be wrapped (fmt.Errorf("%w: failed to apply discount", err))
• Input validation should be the first thing to do (checking if value is zero or below zero). Validation errors will short-circuit the method call before the repository call. Also, there is no validation for the coupon code string (if it's empty). Minimum basket value is not checked either.
• Negative value error should be a declared one, and the fmt.Errorf call should include the input value.

Tests are currently very shallow. Without a tests overkill, it would be good to have more coverage.

• a.srv is a pointer to a HTTP server. It's important to check if it's nil to avoid nil pointer dereference errors.
• the method should return an error (Start() error), where the body of the method is returning a call to a.srv.ListenAndServe(), only.

The reason for both the verification and the panic are unclear, on why the application is scoping the execution to machines with 32 CPUs.

If the intention is to scope the runtime on 32-bit CPU architectures, the process should be rectified -- starting by adapting the code if possible (using uint32 / int32 for example) before just panicking out of the process.

The Save method is simply adding the input coupon to the Repository's map. While validation such as checking for empty fields should be done on the service layer, this method should verify if the code already exists in the map, returning an appropriate error if so. Otherwise, it risks overwriting user data by using different Coupon with the same code.

Like so:

var ErrAlreadyExists = errors.New("already exists")

func (r *Repository) Save(coupon entity.Coupon) error {
	if _, ok := r.entries[coupon.Code]; ok {
		return fmt.Errorf("%w: coupon with code %s", ErrAlreadyExists, coupon.Code)
	}
	r.entries[coupon.Code] = coupon
	return nil
}

This is a placeholder issue to review the codebase after the most meaningful changes are done, in order to rectify any present runtime issues and ensure the app is in a running condition.

• As covered in #6, the request-specific objects can be declared within the HTTP endpoint methods. In this case, the Coupon type can be declared within the function body.
• Also, when the call errors, the client does not get a response. An appropriate response message and code should be written before the return.

Currently the Config struct in this package is declared but not used, as well as being empty: type Config struct{}

Noting that there is no specific reason on why this should be defined, the best course of action is to simply remove it

• If the method is set to support multiple codes at a time, it may be best to declare it as a variatic function. This would allow it to accept any amount of codes (one-to-many) without actually having to write two functions, or adapting to either implementation: GetCoupons(codes ...string) ([]Coupon, error)
• Error e is initialized with a nil value. This is not necessary as it's its default value already as var e error
• Errored calls to FindByCode will lose its underlying error value:
  • the raised error is not being included in the fmt.Errorf call to define e when it is nil. It should always be present
  • when e is not nil and a new error is appended to it, it's shadowing the previous error. While the error string will expand for each error, only one (locally declared) error is returned.
• To mitigate the situation above one could:
  • A) update to Go 1.20 and leverage errors.Join; by initializing a list of errors, appending non-nil errors to it, and if longer than zero, returning the list as return nil, errors.Join(e...)
  • B) if Go 1.18 is required, implement errors.Join locally, as it's simple and affordable (see zalgonoise/x/errors)
  • C) short-circuit out on the first error, returning it
• Checking for the error is still very important, and should be done before the last return (returning nil, e instead of coupons, e)

• The returned error should not have Cupon capitalized (Go errors should start with lowercase unless it's an acronym or similar)
• The returned error is not actually leveraging fmt.Errorf(). It should:
  • Actually be a declared error (var ErrNotFound = errors.New("not found")
  • The fmt.Errorf() call should wrap this error with a custom message pointing to the input cupon: return nil, fmt.Errorf("%w: no cupon with code %s", ErrNotFound, code)
• While it is fine as is, the method could be simplified to:

var ErrNotFound = errors.New("not found")

func (r *Repository) FindByCode(code string) (*entity.Coupon, error) {
	if coupon, ok := r.entries[code]; ok {
		return &coupon, nil
	}
	return nil, fmt.Errorf("%w: no cupon with code %s", ErrNotFound, code)
}

• Similar to the Start method, it should check if a.srv is nil.
• Similar to the Start method, the signature should include an error in its returns: Close() error
• There is a 5 second wait before the function actually does anything that could be removed (with the time.After call)
• This implementation has no other shutdown procedures (taking into account an in-memory database); otherwise it would contain those shutdown procedures as well (for the repository).
• The last action should simply be a return a.srv.Shutdown(ctx) call

The New function is too convoluted for what it is doing, and could be greatly simplified.

• The function signature is scoping to a type parameter (of type Service, an interface). Although generics are great, this particular type parameter is not improving the code's readability, nor does it make any difference if the function were to simply accept an interface as input. Type parameters are extra useful when working with concrete types, and only slightly useful when working with complex generic objects (structs and interfaces alike). Proposing changing the signature to: New(port int, svc Service) API
• According to Gin's documentation, best practices would be to create the engine from a designated function for it, like gin.New() or gin.Default(), instead of new(gin.Engine). The variable is immediately shadowed right after by a (correct) call to gin.New()
• The configuration for r.Use(gin.Recovery()) should be documented in particular for added clarity. I understand now that it will recover from panics and return 500-status-code HTTP responses, but I had to browse the methods documentation to know that.
• The return statement of this function is a method call of a struct built in-place. This is not exactly friendly to the eye, and even less if the type happened to be a pointer. While the reason for the method call is covered below, these two steps should be done separately simply for readability.

The API type exposes two private methods, withServer and withRoutes; both of which return an API (itself). These methods imply these are optional procedures when configuring the server, like one would order a hotdog with cheese or with fries. However, the actual purpose of these methods is to configure the HTTP server in two distinct manners, both required regardless: configuring the HTTP server, and configuring the routes in the multiplexer. Note the issues in these methods:

• the API type is not a pointer. This means that the methods mutating these fields are not actually performing any lasting changes unless the returned object is used. The answer is not to make it a pointer either, but to include its actions in the New function instead.
• The withRoutes method is never called, so the multiplexer is never configured in the first place
• The withServer method is overly complex. It creates a channel of API and configures the HTTP server in a go routine, to then return the same object (the receiver!) to the channel. Only the a.srv definition matters, here.

The repository interface type is being declared within the implementation package, so there is no point in even declaring it. Moreover, it's currently set as private to this package which additionally makes it even less useful.

This issue is a placeholder to move the Repository interface (exported) to a new top-level directory within internal, for Cupons. This will allow to declare both the entity and this repository interface in a reasonably separate location where this package will implement that same interface.

The current in-memory implementation is fine, however it's not concurrency-ready. The service may receive many requests simultaneously that may potentially raise data races.

To mitigate this type of issue, it's best to swap the regular map[string]entity.Coupon type with a sync.Map type.

There is no reason for entities to be grouped all in the same package, instead of being split in different locations of the codebase. Also, Cupons, for instance, is (almost) repeated.

This is a placeholder issue to move the entities to a top-level folder in the internal folder.

• As covered in #6, the request-specific objects can be declared within the HTTP endpoint methods. In this case, the ApplicationRequest type can be declared within the function body.
• When the request errors, the client does not get a response. An appropriate response message and status code should be included before the returns

• Function signature is returning an any type when an error type was meant
• Creating the coupon would be easier and more readable if the internal/coupon package contained a New function for it too. Optionally this function could do the necessary validation for its elements, or it could be done in this service method.
• Returned error should be more descriptive (return fmt.Errorf("%w: coupon creation failed", err)

The current Dockerfile is starting by building the application in a builder stage, but never actually adds the built binary to a final deployment container.

Ref: https://docs.docker.com/build/building/multi-stage/

• brumhard/alligotor is a dependency that is being used to load the server configuration (host and port) for the application; however there is no explanation or reason for using it. Fetching these configuration values could be done in a simpler manner using standard library tools (os.Getenv() / flags.Parse()) considering the surface area of the configuration (literally two elements)
• Config is nesting a struct, however that struct is its only element. It should be best to simply work with the api.Config object directly, in this case.
• The New function assumes a successful execution by not returning an error, but in this case it could potentially raise one. It should be best to either:
  • A) initialize a Config with defaults that are overwritten with user's input
  • B) (less pretty) returning a Config and an error
• Taking into account that this config is solely setting HTTP server values, it should be kept in the internal/api directory It's best to keep the config in its own package, in this case under cmd

• The package-scoped variable declarations are not being used anywhere beyond the main function. They would be better scoped within the main function.
• API is being initialized under the variable with name 本. Beyond the readability hiccup it causes, it could also raise other potential issues. It would be best to keep the variable names simple and in ASCII characters, considering that the entire code-base is also in English
• Closing the server after one year is not exactly the best approach to running a web app. It would be best to leave the service running:
  • indefinitely (capturing a signal terminate / interrupt to gracefully stop the server)
  • stop at a certain time / date reference (in case of an event).
  • having a configuration delimiting this duration, if set

Currently, go.mod exports this module as: module coupon_service

This doesn't work well when importing the package, and should point to a URL instead. Ref: https://go.dev/ref/mod#go-mod-file-module

HTTP objects like ApplicationRequest and CouponRequest should be declared on the transport layer where they are used, and not declared as an entity (as positioned in issue #4).

Ideally, these objects are declared within the function that uses them (if possible) to have the most reduced and isolated scope possible. These objects are exclusively used in the lifetime of a request, within the transport layer.

Exported functions, methods, types and declarations are not documented. It's important to add these for added readability.