Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CECI - Implementation #21

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

ceciivanov
Copy link

No description provided.

@wiz-gwi
Copy link

wiz-gwi bot commented Jul 15, 2024

Wiz Scan Summary

IaC Misconfigurations 0C 1H 0M 1L 0I
Vulnerabilities 0C 0H 0M 0L 0I
Sensitive Data 0C 0H 0M 0L 1I
Total 0C 1H 0M 1L 1I
Secrets 0🔑

Copy link

@teliaz teliaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for submitting the test @ceciivanov ! Can you answer the following alongside my comments?

Are you following a specific design pattern on you submission?
How do you value code comments and when do you think that they are necessary?

thanks for the effort you put in this assignment!

@@ -0,0 +1,23 @@
# Start with a base image that includes Go
FROM golang:1.22.4
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for providing a containerized version here. To reduce the image size, could you consider using a multi-stage build? Are there any other techniques you might consider for minimizing the container image size?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I could use multi-stage build for example builder stage for the build step and then a final stage with small base image like alpine which will copy the built artifact from the previous stage. Another technique to minimise the container image size is to remove unnecessary files, using smaller base images and using single RUN commands to reduce layers.

)

// Asset interface to be implemented by all asset types
type Asset interface {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the reasoning for introducing this abstraction?
Which are the pros and cons of this approach?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using interface as an asset provides a flexible way to handle various of asset types, which can be extended in the future adding new assets. Also with the interface we can define reusable and unified operations upon them (like get details etc.) and this way we reduce the complexity of having multiple structs with many different operations.

A possible con that I can think of is that for simple applications with small number of assets and not many common and complex operations, this can add some overhead to the overall logic of the code.

}

// NewInMemoryUserRepository creates a new instance of InMemoryUserRepository
func NewInMemoryUserRepository() *InMemoryUserRepository {
Copy link

@teliaz teliaz Jul 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great approach. Could you explain if it would be beneficial/achievable on horizontal scaling?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By trying to implement horizontal scaling with this approach I think that we will have problem with data inconsistency because each instance of application will have it's own in memory data and this could lead to synchronisation challenges & issues, for example user's favorites may differ across different instances.

To achieve horizontal scaling a good approach will be following service-oriented architecture where the application will break to micro services each handling different domain and will be independent of any other.

@ceciivanov
Copy link
Author

ceciivanov commented Jul 20, 2024

Thank you for submitting the test @ceciivanov ! Can you answer the following alongside my comments?

Are you following a specific design pattern on you submission? How do you value code comments and when do you think that they are necessary?

thanks for the effort you put in this assignment!

The approach is following the Repository/Service design which decouples the data access layer (how data is stored) from the business logic (what operations upon the data are implemented).

The Repository layer provides us flexibility to implement different type of repositories for example in memory which can be used more for development and test purposes and a let's say an PostgreSQL_Repository which can be used for productive environments where the data will be stored in a database, and an easy way to swap between repository implementations without changing at all the business/application logic.

The Service layer is an intermediary between the repository layer (data access layer) and the application layer. It encapsulates the business logic. The service layer can expose business logic as services or APIs that can be consumed by different clients, such as web, mobile, or third-party applications. For example if in the future we want to add a User Notification service, we can add it as a new service without worrying that it will affect any other part of the application.

Code Comments are highly valued as they can enhance the readability by providing explanations that aren't obvious from the code. But I believe that good approach is not to fill up the code with comments everywhere especially when the code and what it does is pretty obvious. That's why I have included small comments for example one sentence of what each function does without adding any additional information on how this function is implemented if it's it doesn't have any complexity (for example deletion of an asset). A case where we would need more comments is if we are applying specific business rules which are not straightforward. Also when the code is more complex (for example the decoder function) it would be good to include comments on how is it implemented to avoid any misuse of it. Good practice also is to document any existing APIs with the expected inputs/outputs and behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants