Add clean code example #2701
Add clean code example #2701
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. |
WalkthroughThe pull request introduces several significant changes to the codebase, including the addition of a Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
🧹 Outside diff range and nitpick comments (39)
clean-code/app/main.go (1)
3-11: Consider enhancing application lifecycle managementWhile the imports are well-organized, consider adding support for graceful shutdown and more robust error handling:
- Add
os/signalandsyscallfor handling shutdown signals- Consider using a more robust logging package like
zerologorzapExample implementation:
import ( "context" "log" "os" "os/signal" "syscall" "app/datasources" "app/datasources/database" "app/server" )clean-code/app/config.go (3)
1-3: Consider moving configuration to a dedicated packageFor better separation of concerns and adherence to clean architecture principles, consider moving this configuration code to a dedicated package (e.g.,
configorinternal/config). This would make it more reusable and maintainable, especially in larger applications.-package main +package config import "os"
5-8: Add field documentation for better clarityConsider adding documentation comments for the struct and its fields to improve code clarity and maintainability.
+// Configuration holds the application configuration settings type Configuration struct { + // Port specifies the server port to listen on Port string + // DatabaseURL specifies the connection string for the database DatabaseURL string }
17-22: Consider adding debug loggingThe environment variable lookup implementation is correct, but adding debug logging could help with troubleshooting configuration issues in production.
func getEnvOrDefault(key, defaultValue string) string { if value, exists := os.LookupEnv(key); exists { + log.Printf("Config: %s=%s\n", key, value) return value } + log.Printf("Config: %s not set, using default: %s\n", key, defaultValue) return defaultValue }clean-code/app/server/server_test.go (1)
14-16: Consider improving context and resource management.The test initializes the server with an empty context and DataSources, which might not represent real-world scenarios accurately.
Consider these improvements:
func TestGetStatus(t *testing.T) { - app := NewServer(context.Background(), &datasources.DataSources{}) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + ds := &datasources.DataSources{} + defer ds.Close() // if applicable + + app := NewServer(ctx, ds)clean-code/app/server/services/books_mock.go (3)
11-13: Add documentation for the mock struct.Consider adding a doc comment explaining the purpose of this mock and how it should be used in tests.
+// BooksServiceMock provides a mock implementation of the BooksService interface +// for testing purposes. It uses testify/mock for implementing mock behavior. type BooksServiceMock struct { mock.Mock }
15-21: Add safe type assertion to prevent potential panics.The current type assertion could panic if the mock is set up incorrectly. While this is a test mock, safer type assertions would make tests more maintainable.
func (m *BooksServiceMock) GetBooks(ctx context.Context) ([]domain.Book, error) { args := m.Called(ctx) if args.Get(0) == nil { return nil, args.Error(1) } - return args.Get(0).([]domain.Book), args.Error(1) + books, ok := args.Get(0).([]domain.Book) + if !ok { + return nil, fmt.Errorf("invalid mock setup: expected []domain.Book, got %T", args.Get(0)) + } + return books, args.Error(1) }
23-26: Consider adding validation for the newBook parameter.While the implementation is correct, consider adding validation to ensure the mock behaves similarly to the real service when receiving invalid input.
func (m *BooksServiceMock) SaveBook(ctx context.Context, newBook domain.Book) error { + if newBook.ID == "" { + return fmt.Errorf("mock validation: book ID cannot be empty") + } args := m.Called(ctx, newBook) return args.Error(0) }clean-code/app/datasources/database/db_mock.go (1)
26-27: Enhance mock cleanup methodThe empty implementation misses opportunities for better testing support.
Consider this enhanced implementation:
+// CloseConnections is a mock implementation that tracks connection closure. +// In tests, you can verify if cleanup was called: +// mock.AssertCalled(t, "CloseConnections") func (m *DatabaseMock) CloseConnections() { + m.Called() }clean-code/app/server/server.go (2)
3-11: Consider adding error handling utilitiesFor a production-ready clean code example, consider importing error handling utilities (like
errorspackage or a custom error package) to properly handle and wrap errors that might occur during database operations or request processing.
15-16: Consider adding essential middleware and documenting versioning strategyFor a production-ready example, consider adding essential middleware for:
- Request logging
- Panic recovery
- CORS handling
- Request timeout
- Authentication/Authorization
Also, document the API versioning strategy (currently using path-based v1).
Example middleware setup:
app := fiber.New() + app.Use(recover.New()) + app.Use(logger.New()) + app.Use(cors.New()) + app.Use(timeout.New(timeout.Config{ + TimeoutHandler: func(c *fiber.Ctx) error { + return c.Status(fiber.StatusRequestTimeout).JSON(fiber.Map{ + "error": "Request Timeout", + }) + }, + Timeout: 5 * time.Second, + })) apiRoutes := app.Group("/api")clean-code/app/datasources/database/memory_db.go (2)
5-11: Consider improving documentation and visibility.While the implementation is good, consider these improvements:
- Add godoc-style comments explaining the purpose and usage of this constructor
- Consider exporting the constructor if it's meant to be used by other packages for testing purposes
-// This is just an example and not for production use +// NewMemoryDB returns an in-memory implementation of the Database interface. +// It is intended for testing and example purposes only, not for production use. +// The implementation stores books in memory and provides basic CRUD operations. -func newMemoryDB() Database { +func NewMemoryDB() Database {
31-32: Document the empty CloseConnections method.Add documentation to explain why this method is empty for the in-memory implementation.
+// CloseConnections is a no-op for memory database as there are no connections to close. +// It exists to satisfy the Database interface. func (db *memoryDB) CloseConnections() { }clean-code/app/datasources/database/db.go (3)
9-12: Consider enhancing the Book struct with tags and documentation.The struct could benefit from:
- Field tags for JSON/DB mapping
- GoDoc comments
- Title field validation
Consider applying these improvements:
+// Book represents a book entity in the system type Book struct { - ID int - Title string + ID int `json:"id" db:"id"` + Title string `json:"title" db:"title"` } +// ValidateBook ensures the book data is valid +func (b Book) ValidateBook() error { + if strings.TrimSpace(b.Title) == "" { + return errors.New("title cannot be empty") + } + return nil +}
14-18: Consider enhancing the Database interface with documentation and pagination.The interface follows good practices with context usage and error handling. However, consider these improvements:
- Add GoDoc comments
- Implement pagination for LoadAllBooks to handle large datasets
- Return the created book from CreateBook for immediate use
Consider applying these changes:
+// Database defines the interface for database operations type Database interface { - LoadAllBooks(ctx context.Context) ([]Book, error) + // LoadAllBooks retrieves a paginated list of books + LoadAllBooks(ctx context.Context, offset, limit int) ([]Book, error) - CreateBook(ctx context.Context, newBook Book) error + // CreateBook adds a new book and returns the created entity + CreateBook(ctx context.Context, newBook Book) (*Book, error) + // CloseConnections closes all active database connections CloseConnections() }
20-34: Consider implementing a database factory pattern with dependency injection.The current implementation could be improved by:
- Using a factory pattern with registered database drivers
- Implementing dependency injection for better testability
- Adding support for database configuration beyond just the URL
Consider this alternative approach:
type DBDriver interface { Connect(ctx context.Context, config DBConfig) (Database, error) } type DBConfig struct { URL string MaxConns int Timeout time.Duration // Add other configuration options } type DatabaseFactory struct { drivers map[string]DBDriver } func (f *DatabaseFactory) Register(name string, driver DBDriver) { f.drivers[name] = driver } func (f *DatabaseFactory) Create(ctx context.Context, driverName string, config DBConfig) (Database, error) { driver, exists := f.drivers[driverName] if !exists { return nil, fmt.Errorf("unknown database driver: %s", driverName) } return driver.Connect(ctx, config) }clean-code/app/datasources/database/db_test.go (2)
11-15: Consider enhancing the memory DB test coverage.While the type check is useful, consider adding:
- Verification that the database is properly initialized and operational.
- Interface compliance check instead of concrete type check.
func TestNewDatabase_MemoryDB(t *testing.T) { ctx := context.Background() db := NewDatabase(ctx, "") - assert.Equal(t, "*database.memoryDB", reflect.TypeOf(db).String()) + // Verify interface compliance + _, ok := db.(Database) + assert.True(t, ok) + + // Verify database is operational + book := Book{Title: "Test Book"} + _, err := db.AddBook(ctx, book) + assert.NoError(t, err) }
31-34: Improve helper function documentation and completeness.The helper function could be enhanced:
- Add documentation explaining its purpose and usage.
- Consider validating all Book fields.
- Use a more specific name indicating what's being asserted.
-func assertBook(t *testing.T, book Book, expectedID int, expected Book) { +// assertBookEquals is a test helper that verifies if a book matches the expected values +// by comparing all relevant fields +func assertBookEquals(t *testing.T, actual Book, expectedID int, expected Book) { + t.Helper() // Mark as test helper for better error reporting assert.Equal(t, expectedID, book.ID) assert.Equal(t, expected.Title, book.Title) + // Add other relevant field comparisons + assert.Equal(t, expected.Author, book.Author) + assert.Equal(t, expected.ISBN, book.ISBN) }clean-code/app/config_test.go (2)
22-30: Consider enhancing test efficiency and coverage.Suggestions for improvement:
- Enable parallel test execution using
t.Parallel()- Convert to table-driven tests to cover multiple scenarios
- Add assertions for other potential default values
Example refactor:
func TestNewConfiguration_Defaults(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + unsetEnv []string + want Configuration + }{ + { + name: "all defaults", + unsetEnv: []string{"PORT", "DATABASE_URL"}, + want: Configuration{Port: "3000", DatabaseURL: ""}, + }, + // Add more test cases here + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for _, env := range tt.unsetEnv { + os.Unsetenv(env) + } + got := NewConfiguration() + assert.Equal(t, tt.want, got) + }) + } - os.Unsetenv("PORT") - os.Unsetenv("DATABASE_URL") - - conf := NewConfiguration() - - assert.Equal(t, "3000", conf.Port) - assert.Equal(t, "", conf.DatabaseURL) }
32-41: Add test cases for edge cases.The current test covers basic scenarios, but consider adding test cases for:
- Empty string values
- Whitespace-only values
- Special characters in environment variables
Example additional test cases:
func TestGetEnvOrDefault(t *testing.T) { t.Parallel() tests := []struct { name string envKey string envValue string defaultValue string want string }{ { name: "existing env", envKey: "TEST_ENV", envValue: "value", defaultValue: "default", want: "value", }, { name: "empty string", envKey: "TEST_ENV_EMPTY", envValue: "", defaultValue: "default", want: "", }, { name: "whitespace only", envKey: "TEST_ENV_SPACE", envValue: " ", defaultValue: "default", want: " ", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { if tt.envValue != "" { os.Setenv(tt.envKey, tt.envValue) defer os.Unsetenv(tt.envKey) } got := getEnvOrDefault(tt.envKey, tt.defaultValue) assert.Equal(t, tt.want, got) }) } }clean-code/app/server/services/books.go (2)
11-14: Add interface documentation.Consider adding GoDoc comments to document the interface and its methods. This will improve code maintainability and help other developers understand the service's responsibilities.
+// BooksService defines the interface for book management operations type BooksService interface { + // GetBooks retrieves all books from the storage GetBooks(ctx context.Context) ([]domain.Book, error) + // SaveBook persists a new book to the storage SaveBook(ctx context.Context, newBook domain.Book) error }
16-22: Add nil check in constructor.Consider adding validation to prevent nil database injection, which could cause runtime panics.
func NewBooksService(db database.Database) BooksService { + if db == nil { + panic("database is required") + } return &booksService{db: db} }clean-code/app/datasources/database/memory_db_test.go (2)
10-15: Enhance test coverage and naming.While the basic happy path is covered, consider these improvements:
- Rename test to describe the scenario (e.g.,
TestMemoryDB_LoadBooks_EmptyDatabase)- Add test cases for error scenarios
- Test context cancellation handling
Example improvement:
-func TestMemoryDB_LoadBooks(t *testing.T) { +func TestMemoryDB_LoadBooks_EmptyDatabase(t *testing.T) { db := newMemoryDB() books, err := db.LoadAllBooks(context.Background()) assert.Nil(t, err) assert.Equal(t, 0, len(books)) } + +func TestMemoryDB_LoadBooks_CancelledContext(t *testing.T) { + db := newMemoryDB() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + books, err := db.LoadAllBooks(ctx) + assert.Error(t, err) + assert.Nil(t, books) +}
1-44: Consider enhancing test organization for clean architecture.For a clean code example, consider these architectural improvements:
- Add a test suite using
suite.Suitefrom testify for better organization- Include test fixtures or factories for book creation
- Add integration tests with the actual database interface
- Document test scenarios in BDD style (Given/When/Then)
clean-code/app/server/handlers/books.go (2)
12-24: Consider enhancing error handling and request validation.The handler implementation is solid with good separation of concerns and proper context propagation. However, there are a few potential improvements:
- The error message "internal error" could be more specific while still not exposing internal details
- Consider adding request validation for query parameters (if any)
- Consider adding context timeout handling
Here's a suggested improvement:
func GetBooks(service services.BooksService) fiber.Handler { return func(c *fiber.Ctx) error { + // Add timeout to prevent long-running queries + ctx, cancel := context.WithTimeout(c.UserContext(), 5*time.Second) + defer cancel() + - books, err := service.GetBooks(c.UserContext()) + books, err := service.GetBooks(ctx) if err != nil { - log.Printf("GetBooks failed: %v", err) + log.Printf("Failed to retrieve books: %v", err) return sendError(c, fiber.StatusInternalServerError, "internal error") } return c.JSON(domain.BooksResponse{ Books: books, }) } }
43-47: Consider enhancing error response structure.The error helper is clean but could be more informative for API consumers.
Consider this enhancement:
-func sendError(c *fiber.Ctx, code int, message string) error { +func sendError(c *fiber.Ctx, code int, message string) error { + c.Set("Content-Type", "application/json") return c.Status(code).JSON(domain.ErrorResponse{ - Error: message, + Error: message, + Code: code, + RequestID: c.GetRespHeader("X-Request-ID"), }) }clean-code/app/server/services/books_test.go (5)
14-22: Enhance test coverage and documentation.Consider the following improvements:
- Add a test description using
t.Run()for better test organization- Verify the actual content of the returned book, not just the length
- Validate that the context is properly passed to the database layer
Here's a suggested improvement:
func TestGetBooks(t *testing.T) { - mockDB := new(database.DatabaseMock) - mockDB.On("LoadAllBooks", mock.Anything).Return([]database.Book{{Title: "Title"}}, nil) - - service := NewBooksService(mockDB) - books, err := service.GetBooks(context.Background()) - assert.Nil(t, err) - assert.Len(t, books, 1) + t.Run("successfully retrieves books from database", func(t *testing.T) { + expectedBook := database.Book{Title: "Title"} + mockDB := new(database.DatabaseMock) + mockDB.On("LoadAllBooks", mock.MatchedBy(func(ctx context.Context) bool { + return ctx != nil + })).Return([]database.Book{expectedBook}, nil) + + service := NewBooksService(mockDB) + books, err := service.GetBooks(context.Background()) + + assert.NoError(t, err) + assert.Len(t, books, 1) + assert.Equal(t, expectedBook.Title, books[0].Title) + mockDB.AssertExpectations(t) + })
24-31: Improve error testing specificity.The test could be more robust by:
- Using a specific error instead of generic
assert.AnError- Adding a test description
- Verifying the error type/message returned to ensure proper error handling
Here's a suggested improvement:
func TestGetBooks_Fails(t *testing.T) { - mockDB := new(database.DatabaseMock) - mockDB.On("LoadAllBooks", mock.Anything).Return(nil, assert.AnError) - - service := NewBooksService(mockDB) - _, err := service.GetBooks(context.Background()) - assert.NotNil(t, err) + t.Run("returns error when database fails", func(t *testing.T) { + expectedErr := errors.New("database connection failed") + mockDB := new(database.DatabaseMock) + mockDB.On("LoadAllBooks", mock.Anything).Return(nil, expectedErr) + + service := NewBooksService(mockDB) + _, err := service.GetBooks(context.Background()) + + assert.Error(t, err) + assert.Equal(t, expectedErr.Error(), err.Error()) + mockDB.AssertExpectations(t) + })
33-40: Enhance save book test coverage.Consider improving the test by:
- Adding test cases for different book data
- Verifying all book fields being passed to the database
- Adding proper test descriptions
Here's a suggested improvement:
func TestSaveBook(t *testing.T) { - mockDB := new(database.DatabaseMock) - mockDB.On("CreateBook", mock.Anything, database.Book{Title: "Title"}).Return(nil) - - service := NewBooksService(mockDB) - err := service.SaveBook(context.Background(), domain.Book{Title: "Title"}) - assert.Nil(t, err) + testCases := []struct { + name string + book domain.Book + }{ + { + name: "successfully saves book with title only", + book: domain.Book{Title: "Title"}, + }, + { + name: "successfully saves book with all fields", + book: domain.Book{Title: "Title", Author: "Author", ISBN: "1234567890"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + mockDB := new(database.DatabaseMock) + mockDB.On("CreateBook", mock.Anything, mock.MatchedBy(func(b database.Book) bool { + return b.Title == tc.book.Title && + b.Author == tc.book.Author && + b.ISBN == tc.book.ISBN + })).Return(nil) + + service := NewBooksService(mockDB) + err := service.SaveBook(context.Background(), tc.book) + + assert.NoError(t, err) + mockDB.AssertExpectations(t) + }) + }
42-49: Improve error handling test coverage.The test could be enhanced by:
- Testing different error scenarios (e.g., validation errors, database constraints)
- Using specific errors instead of generic
assert.AnError- Adding proper test descriptions
Here's a suggested improvement:
func TestSaveBook_Fails(t *testing.T) { - mockDB := new(database.DatabaseMock) - mockDB.On("CreateBook", mock.Anything, database.Book{Title: "Title"}).Return(assert.AnError) - - service := NewBooksService(mockDB) - err := service.SaveBook(context.Background(), domain.Book{Title: "Title"}) - assert.NotNil(t, err) + testCases := []struct { + name string + book domain.Book + expectedError error + }{ + { + name: "fails when database error occurs", + book: domain.Book{Title: "Title"}, + expectedError: errors.New("database error"), + }, + { + name: "fails when duplicate book exists", + book: domain.Book{Title: "Title"}, + expectedError: errors.New("duplicate book"), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + mockDB := new(database.DatabaseMock) + mockDB.On("CreateBook", mock.Anything, mock.MatchedBy(func(b database.Book) bool { + return b.Title == tc.book.Title + })).Return(tc.expectedError) + + service := NewBooksService(mockDB) + err := service.SaveBook(context.Background(), tc.book) + + assert.Error(t, err) + assert.Equal(t, tc.expectedError.Error(), err.Error()) + mockDB.AssertExpectations(t) + }) + }
1-49: Consider architectural improvements for better test coverage.While the test structure is good, consider these architectural improvements:
- Add separate test cases for domain.Book to database.Book mapping
- Implement more thorough context handling (timeouts, cancellation)
- Create custom error types for better error handling and verification
- Consider adding integration tests alongside these unit tests
Would you like assistance in implementing any of these improvements?
clean-code/app/datasources/database/postgres_db.go (3)
12-16: Consider enhancing the interface with transaction supportWhile the current interface covers basic operations, consider adding transaction support for maintaining data consistency in complex operations. Also, the
Closemethod should return an error to handle cleanup failures.type PostgresPool interface { Query(ctx context.Context, sql string, args ...interface{}) (pgx.Rows, error) Exec(ctx context.Context, sql string, args ...interface{}) (pgconn.CommandTag, error) - Close() + Close() error + Begin(ctx context.Context) (pgx.Tx, error) }
52-58: Add support for batch operations in CreateBookThe current implementation doesn't support batch inserts, which could be inefficient for multiple book creations.
+const queryCreateBook = "INSERT INTO books (title) VALUES ($1)" + func (db *postgresDB) CreateBook(ctx context.Context, newBook Book) error { - _, err := db.pool.Exec(ctx, "INSERT INTO books (title) VALUES ($1)", newBook.Title) + _, err := db.pool.Exec(ctx, queryCreateBook, newBook.Title) if err != nil { return fmt.Errorf("failed to insert book: %w", err) } return nil } + +func (db *postgresDB) CreateBooks(ctx context.Context, books []Book) error { + batch := &pgx.Batch{} + for _, book := range books { + batch.Queue(queryCreateBook, book.Title) + } + + results := db.pool.SendBatch(ctx, batch) + defer results.Close() + + return results.Close() +}
1-62: Consider implementing repository pattern more strictlyWhile the current implementation is clean, consider these architectural improvements:
- Move SQL queries to a separate constants file for better maintenance
- Add a proper domain model layer to separate database entities from domain entities
- Consider adding a transaction middleware for handling complex operations
Would you like me to provide an example of how to implement these architectural improvements?
clean-code/app/datasources/database/postgres_db_test.go (4)
15-17: Consider using parameterized queries in test expectationsWhile the current query works, consider using parameterized queries in the test expectations to better reflect secure coding practices that should be used in the actual implementation.
-mockPool.ExpectQuery("SELECT id, title FROM books"). +mockPool.ExpectQuery("SELECT id, title FROM books WHERE 1=$1"). + WithArgs(1).
11-28: Consider using table-driven testsThe test could be more comprehensive by using table-driven tests to cover multiple scenarios (empty result, multiple books, etc.).
Here's a suggested refactor:
func TestPostgresDB_GetBooks(t *testing.T) { tests := []struct { name string mockRows func() *pgxmock.Rows want []Book wantErr bool }{ { name: "single book", mockRows: func() *pgxmock.Rows { return pgxmock.NewRows([]string{"id", "title"}).AddRow(1, "book1") }, want: []Book{{ID: 1, Title: "book1"}}, }, { name: "multiple books", mockRows: func() *pgxmock.Rows { return pgxmock.NewRows([]string{"id", "title"}). AddRow(1, "book1"). AddRow(2, "book2") }, want: []Book{{ID: 1, Title: "book1"}, {ID: 2, Title: "book2"}}, }, { name: "no books", mockRows: func() *pgxmock.Rows { return pgxmock.NewRows([]string{"id", "title"}) }, want: []Book{}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { mockPool, err := pgxmock.NewPool() assert.Nil(t, err) mockPool.ExpectQuery("SELECT id, title FROM books"). WillReturnRows(tt.mockRows()) db := postgresDB{pool: mockPool} result, err := db.LoadAllBooks(context.Background()) if tt.wantErr { assert.Error(t, err) return } assert.Nil(t, err) assert.Equal(t, tt.want, result) assert.Nil(t, mockPool.ExpectationsWereMet()) }) } }
34-35: Consider using specific database errorsInstead of using
assert.AnError, consider using specific database errors (likepgx.ErrNoRowsor custom errors) to make the test more realistic and meaningful. Also, the error message assertion could be more specific.- WillReturnError(assert.AnError) + WillReturnError(&pgconn.PgError{ + Code: "42P01", + Message: "relation \"books\" does not exist", + }) - assert.ErrorContains(t, err, "failed to query books table") + assert.ErrorContains(t, err, "failed to query books table: relation \"books\" does not exist")Also applies to: 41-42
51-53: Make SQL expectations more explicitThe current SQL expectation is too permissive. Consider making it more explicit to catch potential query changes.
- mockPool.ExpectExec("INSERT INTO books"). + mockPool.ExpectExec("INSERT INTO books (title) VALUES ($1)"). WithArgs("book1"). WillReturnResult(pgxmock.NewResult("INSERT", 1))clean-code/app/server/handlers/books_test.go (1)
20-33: Consider adding more test cases for GetBooks.While the happy path is tested, consider adding test cases for:
- Multiple books in response
- Empty books list
func TestGetBooks(t *testing.T) { testCases := []struct { name string books []domain.Book expected int }{ { name: "single book", books: []domain.Book{{Title: "Title"}}, expected: 1, }, { name: "multiple books", books: []domain.Book{{Title: "Title1"}, {Title: "Title2"}}, expected: 2, }, { name: "empty list", books: []domain.Book{}, expected: 0, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { mockService := new(services.BooksServiceMock) mockService.On("GetBooks", mock.Anything).Return(tc.books, nil) app := fiber.New() app.Get(booksRoute, GetBooks(mockService)) resp, err := app.Test(httptest.NewRequest("GET", booksRoute, nil)) assert.Nil(t, err) assert.Equal(t, 200, resp.StatusCode) body := bodyFromResponse[domain.BooksResponse](t, resp) assert.Len(t, body.Books, tc.expected) }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
clean-code/app/go.modis excluded by!**/*.modclean-code/app/go.sumis excluded by!**/*.sum,!**/*.sumclean-code/docker-compose.ymlis excluded by!**/*.yml
📒 Files selected for processing (24)
clean-code/.gitignore(1 hunks)clean-code/Dockerfile-local(1 hunks)clean-code/README.md(1 hunks)clean-code/app/config.go(1 hunks)clean-code/app/config_test.go(1 hunks)clean-code/app/datasources/data_sources.go(1 hunks)clean-code/app/datasources/database/db.go(1 hunks)clean-code/app/datasources/database/db_mock.go(1 hunks)clean-code/app/datasources/database/db_test.go(1 hunks)clean-code/app/datasources/database/memory_db.go(1 hunks)clean-code/app/datasources/database/memory_db_test.go(1 hunks)clean-code/app/datasources/database/postgres_db.go(1 hunks)clean-code/app/datasources/database/postgres_db_test.go(1 hunks)clean-code/app/main.go(1 hunks)clean-code/app/server/domain/books.go(1 hunks)clean-code/app/server/domain/errors.go(1 hunks)clean-code/app/server/handlers/books.go(1 hunks)clean-code/app/server/handlers/books_test.go(1 hunks)clean-code/app/server/server.go(1 hunks)clean-code/app/server/server_test.go(1 hunks)clean-code/app/server/services/books.go(1 hunks)clean-code/app/server/services/books_mock.go(1 hunks)clean-code/app/server/services/books_test.go(1 hunks)clean-code/db/init_db.sql(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- clean-code/.gitignore
- clean-code/Dockerfile-local
- clean-code/app/datasources/data_sources.go
- clean-code/app/server/domain/books.go
- clean-code/app/server/domain/errors.go
- clean-code/db/init_db.sql
🧰 Additional context used
🪛 LanguageTool
clean-code/README.md
[uncategorized] ~16-~16: Loose punctuation mark.
Context: ... ### Endpoints - GET /api/v1/books: Retrieves a list of all books. ```sh ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~21-~21: Loose punctuation mark.
Context: ...i/v1/books ``` - POST /api/v1/books: Adds a new book to the collection. ``...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint (0.35.0)
clean-code/README.md
3-3: null
Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (26)
clean-code/app/main.go (1)
14-14: Verify configuration implementation details
Let's verify the implementation of NewConfiguration() and its error handling capabilities.
✅ Verification successful
Configuration implementation is properly structured and safe
The NewConfiguration() function is well-implemented with:
- A clear struct definition with Port and DatabaseURL fields
- Safe environment variable handling using
getEnvOrDefault - Default values for required configurations
- Proper test coverage in
config_test.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NewConfiguration implementation
ast-grep --pattern 'func NewConfiguration() $$$'
# Check if configuration uses environment variables
rg -l "os.Getenv" clean-code/
Length of output: 418
Script:
#!/bin/bash
# Let's check the Configuration struct definition and getEnvOrDefault implementation
rg -A 5 "type Configuration struct" clean-code/app/
# Search for getEnvOrDefault implementation
ast-grep --pattern 'func getEnvOrDefault($$$) $$$'
# Check if there are any direct usages of Configuration
rg "Configuration" clean-code/app/
Length of output: 1318
clean-code/app/config.go (1)
1-22: Verify configuration usage across the application
Let's verify how this configuration is used across the application to ensure proper error handling and consistent usage.
✅ Verification successful
Let me gather more information about the database connection handling to ensure proper error handling is in place.
Configuration implementation is well-structured with proper error handling
The verification reveals a well-implemented configuration system with appropriate error handling:
- Configuration is properly centralized with no direct environment variable access in the clean-code app
- Database connection has proper error handling with fallback to in-memory database when URL is empty
- Implementation includes comprehensive test coverage for both valid and invalid database configurations
- The system gracefully handles missing environment variables with sensible defaults
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Configuration struct usage
echo "Checking Configuration struct usage:"
rg -l "Configuration" --type go
# Search for environment variable usage to ensure centralization
echo -e "\nChecking for direct environment variable access that should use Configuration:"
rg -l "os.Getenv|os.LookupEnv" --type go | grep -v "config.go"
# Check for proper error handling around database connection
echo -e "\nChecking database connection handling:"
rg -B 2 -A 2 "DatabaseURL" --type go
Length of output: 2661
Script:
#!/bin/bash
# Check database connection implementation
echo "Checking database connection implementation:"
ast-grep --pattern 'func NewDatabase($_, $url string) $_'
# Check for error handling in database connection
echo -e "\nChecking error handling in database connection:"
rg -B 3 -A 3 "NewDatabase" --type go
# Check if there's any validation for DatabaseURL
echo -e "\nChecking for DatabaseURL validation:"
rg -B 2 -A 2 "DATABASE_URL" --type go
Length of output: 4054
clean-code/app/server/server_test.go (2)
1-12: LGTM! Package and imports are well-organized.
The package declaration and import organization follow Go best practices, with standard library imports, internal packages, and external dependencies properly grouped.
17-24: 🛠️ Refactor suggestion
Enhance test coverage with table-driven tests.
The current test only covers the happy path. Consider adding test cases for different scenarios and improving the response structure.
Here's a suggested improvement:
-func TestGetStatus(t *testing.T) {
- app := NewServer(context.Background(), &datasources.DataSources{})
-
- resp, err := app.Test(httptest.NewRequest("GET", "/api/status", nil))
- assert.Nil(t, err)
- assert.Equal(t, 200, resp.StatusCode)
-
- body, err := io.ReadAll(resp.Body)
- assert.Nil(t, err)
- assert.Equal(t, "ok", string(body))
+func TestGetStatus(t *testing.T) {
+ tests := []struct {
+ name string
+ method string
+ expectedStatus int
+ expectedBody string
+ }{
+ {
+ name: "success case",
+ method: "GET",
+ expectedStatus: http.StatusOK,
+ expectedBody: "ok",
+ },
+ {
+ name: "wrong method",
+ method: "POST",
+ expectedStatus: http.StatusMethodNotAllowed,
+ expectedBody: "",
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+
+ app := NewServer(ctx, &datasources.DataSources{})
+ req := httptest.NewRequest(tt.method, "/api/status", nil)
+ resp, err := app.Test(req)
+
+ assert.NoError(t, err)
+ assert.Equal(t, tt.expectedStatus, resp.StatusCode)
+
+ if tt.expectedBody != "" {
+ body, err := io.ReadAll(resp.Body)
+ assert.NoError(t, err)
+ assert.Equal(t, tt.expectedBody, string(body))
+ }
+ })
+ }
}Additionally, consider:
- Using structured JSON responses instead of plain text
- Adding test cases for server shutdown scenarios
- Testing response headers
Let's verify if the status endpoint is defined in the server implementation:
clean-code/app/server/services/books_mock.go (2)
1-9: LGTM!
The package declaration and imports are well-organized and follow Go best practices.
11-26: Verify interface compliance.
Let's ensure this mock correctly implements all methods of the BooksService interface.
✅ Verification successful
Mock implementation matches the interface perfectly
The BooksService interface defines exactly two methods:
GetBooks(ctx context.Context) ([]domain.Book, error)SaveBook(ctx context.Context, newBook domain.Book) error
The BooksServiceMock implements both methods with matching signatures, making it a complete and correct implementation of the interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that BooksServiceMock implements all methods of BooksService interface
# Search for BooksService interface definition
echo "Searching for BooksService interface definition..."
ast-grep --pattern 'type BooksService interface {
$$$
}'
# Search for any additional methods that might need to be mocked
echo "Searching for method calls on BooksService..."
rg -t go "BooksService\." -A 2
Length of output: 621
clean-code/app/datasources/database/db_mock.go (3)
1-8: LGTM!
The package name and imports are appropriate for a database mock implementation.
9-11: LGTM!
The mock struct is correctly defined with embedded mock.Mock.
1-27: Verify interface implementation completeness
Let's ensure this mock fully implements the Database interface.
✅ Verification successful
Based on the shell script results, I can see the Database interface definition in clean-code/app/datasources/database/db.go which shows the following methods:
LoadAllBooks(ctx context.Context) ([]Book, error)CreateBook(ctx context.Context, newBook Book) errorCloseConnections()
Comparing this with the mock implementation in db_mock.go, we can see that all methods are properly implemented:
LoadAllBooks- Implemented with correct signature and mock behaviorCreateBook- Implemented with correct signature and mock behaviorCloseConnections- Implemented as an empty method (which is valid for a mock)
Mock implementation is complete and correct
The DatabaseMock struct fully implements all methods of the Database interface with proper signatures and appropriate mock behaviors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Database interface implementation
# Expected: Find Database interface definition and compare methods
# Find the interface definition
echo "Looking for Database interface definition..."
rg -p "type\s+Database\s+interface\s*{" -A 10
# Find any other methods that might need mocking
echo "Looking for interface method usages..."
ast-grep --pattern 'interface { $$$ }'
Length of output: 17310
clean-code/app/server/server.go (1)
1-24: Verify test coverage for server setup
Let's verify the existence of tests for the server setup and endpoints.
✅ Verification successful
Test coverage is comprehensive and well-structured
The codebase has thorough test coverage across all layers:
- Server setup and health check endpoint (/api/status) are tested in
server_test.go - Book handlers have extensive tests in
handlers/books_test.gocovering success and error cases - Book service layer is well tested in
services/books_test.gowith proper mocking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files related to server setup
echo "Searching for server tests..."
fd -e go -g "*server*test.go" clean-code/app/server
# Search for specific test cases
echo "Searching for specific test cases..."
rg -l "TestNewServer|TestHealthCheck|TestGetBooks|TestAddBook" clean-code/app/server
Length of output: 419
Script:
#!/bin/bash
# Let's check the content of server_test.go to verify test coverage
echo "Content of server_test.go:"
cat clean-code/app/server/server_test.go
# Check test coverage for handlers and services
echo -e "\nContent of handlers/books_test.go:"
cat clean-code/app/server/handlers/books_test.go
echo -e "\nContent of services/books_test.go:"
cat clean-code/app/server/services/books_test.go
Length of output: 5010
clean-code/app/datasources/database/memory_db.go (1)
1-32: Verify Database interface implementation.
Let's ensure that memoryDB correctly implements all methods of the Database interface.
✅ Verification successful
The output shows the Database interface but seems incomplete in showing memoryDB methods. Let me try a different pattern to ensure we capture all the methods.
Database interface implementation is complete and correct
The memoryDB type correctly implements all methods required by the Database interface:
- LoadAllBooks(ctx context.Context) ([]Book, error)
- CreateBook(ctx context.Context, newBook Book) error
- CloseConnections()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Database interface implementation
# Expected: Find the Database interface definition and compare with memoryDB methods
# Find the Database interface definition
echo "Database interface definition:"
ast-grep --pattern 'type Database interface {
$$$
}'
# Find all methods of memoryDB
echo -e "\nmemoryDB method implementations:"
ast-grep --pattern 'func (db *memoryDB) $$$'
Length of output: 632
Script:
#!/bin/bash
# Search for memoryDB methods with a more specific pattern
echo "memoryDB methods:"
rg "func \(db \*memoryDB\)" -A 1
# Also verify the struct definition
echo -e "\nmemoryDB struct definition:"
ast-grep --pattern 'type memoryDB struct {
$$$
}'
Length of output: 1018
clean-code/app/datasources/database/db.go (1)
1-8: LGTM! Clean package structure and imports.
The package name matches the directory name and imports are minimal.
clean-code/app/datasources/database/db_test.go (1)
1-9: LGTM!
The package declaration and imports are appropriate for the test file.
clean-code/app/server/services/books.go (2)
1-9: LGTM! Clean package structure and imports.
The package organization follows Go best practices with clear separation between standard library and project imports.
24-38: Verify complete domain model mapping.
The current implementation only maps the Title field. Ensure this is intentional and that no other important book fields (e.g., ID, Author, ISBN) are missing from the domain model.
✅ Verification successful
The domain model is intentionally simplified to only include the Title field
Looking at the relevant models in the codebase:
- Domain model (
clean-code/app/server/domain/books.go) only definesTitlefield - Database model (
clean-code/app/datasources/database/db.go) hasIDandTitlefields
The current implementation correctly maps all fields from the domain model, and the simplification appears to be intentional for this clean code example. The other Book structs found in the codebase are from different example projects and are not related to this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other potential book fields in the domain and database models
rg -A 5 "type Book struct"
Length of output: 3738
clean-code/app/datasources/database/memory_db_test.go (2)
1-8: LGTM! Clean imports and proper package naming.
The imports are minimal and appropriate for the testing requirements.
17-27: Verify the assertBook helper function implementation.
The test uses an assertBook helper function that isn't visible in the current context. Let's verify its implementation to ensure proper assertions.
Additionally, consider enhancing the test coverage:
- Validate book ID assignment logic
- Test error scenarios (e.g., duplicate books if that's a constraint)
✅ Verification successful
assertBook helper function implementation is verified and adequate
The helper function correctly validates both the ID assignment and title matching:
- Verifies book ID matches the expected ID value
- Confirms book title matches the expected title
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for assertBook function implementation
ast-grep --pattern 'func assertBook($$$)'
Length of output: 402
clean-code/app/server/handlers/books.go (2)
1-10: LGTM! Clean package structure and imports.
The package organization follows clean architecture principles with clear separation of concerns.
1-47: Verify consistent error handling across handlers.
Let's ensure that the error handling pattern is consistent across all handlers in the application.
clean-code/app/server/services/books_test.go (1)
1-12: LGTM! Clean imports and proper package organization.
clean-code/app/datasources/database/postgres_db.go (2)
3-10: LGTM! Appropriate imports for PostgreSQL operations
The imports are well-organized and include the necessary packages from pgx v5 for PostgreSQL operations.
29-31: LGTM! Clean type definition
The struct definition is minimal and follows interface segregation principle.
clean-code/app/datasources/database/postgres_db_test.go (1)
68-70: Use specific database errors (similar to GetBooks_Fail)
Similar to the feedback for GetBooks_Fail, consider using specific database errors and more precise error message assertions.
Also applies to: 75-76
clean-code/app/server/handlers/books_test.go (3)
1-16: LGTM! Well-organized imports with appropriate testing dependencies.
The imports are clean and well-structured, properly separating standard library imports from external and internal dependencies.
18-18: LGTM! Good use of constant for API route.
Defining the route as a constant reduces duplication and makes maintenance easier.
1-102: Verify handler implementations exist.
Let's ensure the handlers being tested are properly implemented.
✅ Verification successful
Handler implementations are properly implemented and match test coverage
The handlers GetBooks and AddBook are correctly implemented in clean-code/app/server/handlers/books.go and their implementations align perfectly with the test scenarios:
GetBookshandles successful retrieval and error cases, matching test cases inTestGetBooksandTestGetBooks_ServiceFailsAddBookhandles successful creation, invalid requests, and service errors, matching test cases inTestAddBook,TestAddBook_InvalidRequest, andTestAddBook_ServiceFails
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GetBooks and AddBook handler implementations
ast-grep --pattern 'func GetBooks($_) fiber.Handler {
$$$
}'
ast-grep --pattern 'func AddBook($_) fiber.Handler {
$$$
}'
Length of output: 4835
| ) | ||
|
|
||
| func main() { | ||
| ctx := context.Background() |
There was a problem hiding this comment.
Replace background context with cancellable context
Using a background context without cancellation can lead to resource leaks. Consider implementing a cancellable context for proper resource management.
- ctx := context.Background()
+ ctx, cancel := context.WithCancel(context.Background())
+ defer cancel()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ctx := context.Background() | |
| ctx, cancel := context.WithCancel(context.Background()) | |
| defer cancel() |
| conf := NewConfiguration() | ||
| dataSources := &datasources.DataSources{ | ||
| DB: database.NewDatabase(ctx, conf.DatabaseURL), | ||
| } | ||
| defer dataSources.DB.CloseConnections() |
There was a problem hiding this comment.
Add error handling for configuration and database initialization
The code doesn't handle potential errors from configuration loading and database initialization. This could lead to silent failures or unexpected behavior.
- conf := NewConfiguration()
+ conf, err := NewConfiguration()
+ if err != nil {
+ log.Fatalf("failed to load configuration: %v", err)
+ }
dataSources := &datasources.DataSources{
- DB: database.NewDatabase(ctx, conf.DatabaseURL),
+ DB: database.NewDatabase(ctx, conf.DatabaseURL),
}
+ if err := dataSources.DB.Ping(ctx); err != nil {
+ log.Fatalf("failed to connect to database: %v", err)
+ }Committable suggestion skipped: line range outside the PR's diff.
| app := server.NewServer(ctx, dataSources) | ||
| log.Fatal(app.Listen(":" + conf.Port)) | ||
| } |
There was a problem hiding this comment.
Implement graceful shutdown handling
The server lacks graceful shutdown handling, which could lead to connection leaks and incomplete request processing during shutdown.
Here's a suggested implementation:
- app := server.NewServer(ctx, dataSources)
- log.Fatal(app.Listen(":" + conf.Port))
+ app := server.NewServer(ctx, dataSources)
+
+ // Server shutdown channel
+ shutdownChan := make(chan os.Signal, 1)
+ signal.Notify(shutdownChan, os.Interrupt, syscall.SIGTERM)
+
+ // Start server in goroutine
+ go func() {
+ if err := app.Listen(":" + conf.Port); err != nil {
+ log.Printf("server error: %v", err)
+ close(shutdownChan)
+ }
+ }()
+
+ // Wait for shutdown signal
+ <-shutdownChan
+ log.Println("shutting down server...")
+
+ // Create shutdown context with timeout
+ shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 10*time.Second)
+ defer shutdownCancel()
+
+ // Attempt graceful shutdown
+ if err := app.Shutdown(shutdownCtx); err != nil {
+ log.Printf("server shutdown error: %v", err)
+ }Committable suggestion skipped: line range outside the PR's diff.
| func NewConfiguration() *Configuration { | ||
| return &Configuration{ | ||
| Port: getEnvOrDefault("PORT", "3000"), | ||
| DatabaseURL: getEnvOrDefault("DATABASE_URL", ""), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for DatabaseURL
The constructor should validate the DatabaseURL to ensure it's not empty in production environments. Additionally, consider using a more specific default port format (":3000") to maintain consistency with common Go web server patterns.
func NewConfiguration() *Configuration {
+ dbURL := getEnvOrDefault("DATABASE_URL", "")
+ if dbURL == "" {
+ log.Println("Warning: DATABASE_URL is not set")
+ }
+
return &Configuration{
- Port: getEnvOrDefault("PORT", "3000"),
- DatabaseURL: getEnvOrDefault("DATABASE_URL", ""),
+ Port: ":" + getEnvOrDefault("PORT", "3000"),
+ DatabaseURL: dbURL,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func NewConfiguration() *Configuration { | |
| return &Configuration{ | |
| Port: getEnvOrDefault("PORT", "3000"), | |
| DatabaseURL: getEnvOrDefault("DATABASE_URL", ""), | |
| } | |
| } | |
| func NewConfiguration() *Configuration { | |
| dbURL := getEnvOrDefault("DATABASE_URL", "") | |
| if dbURL == "" { | |
| log.Println("Warning: DATABASE_URL is not set") | |
| } | |
| return &Configuration{ | |
| Port: ":" + getEnvOrDefault("PORT", "3000"), | |
| DatabaseURL: dbURL, | |
| } | |
| } |
| func (m *DatabaseMock) CreateBook(ctx context.Context, newBook Book) error { | ||
| args := m.Called(ctx, newBook) | ||
| return args.Error(0) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation and documentation
Similar to LoadAllBooks, this implementation needs validation and documentation.
Consider this improved implementation:
+// CreateBook is a mock implementation that simulates book creation.
+// Example usage:
+// mock.On("CreateBook", ctx, book).Return(nil)
func (m *DatabaseMock) CreateBook(ctx context.Context, newBook Book) error {
+ if ctx == nil {
+ return fmt.Errorf("context is required")
+ }
+ if newBook.ID == 0 {
+ return fmt.Errorf("invalid book: ID is required")
+ }
args := m.Called(ctx, newBook)
return args.Error(0)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *DatabaseMock) CreateBook(ctx context.Context, newBook Book) error { | |
| args := m.Called(ctx, newBook) | |
| return args.Error(0) | |
| } | |
| // CreateBook is a mock implementation that simulates book creation. | |
| // Example usage: | |
| // mock.On("CreateBook", ctx, book).Return(nil) | |
| func (m *DatabaseMock) CreateBook(ctx context.Context, newBook Book) error { | |
| if ctx == nil { | |
| return fmt.Errorf("context is required") | |
| } | |
| if newBook.ID == 0 { | |
| return fmt.Errorf("invalid book: ID is required") | |
| } | |
| args := m.Called(ctx, newBook) | |
| return args.Error(0) | |
| } |
| func (db *postgresDB) LoadAllBooks(ctx context.Context) ([]Book, error) { | ||
| rows, err := db.pool.Query(ctx, "SELECT id, title FROM books") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to query books table: %w", err) | ||
| } | ||
| defer rows.Close() | ||
|
|
||
| var books []Book | ||
| for rows.Next() { | ||
| var record Book | ||
| err := rows.Scan(&record.ID, &record.Title) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to scan rows: %w", err) | ||
| } | ||
| books = append(books, record) | ||
| } | ||
| return books, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and resource management in LoadAllBooks
The current implementation has several areas for improvement:
- Missing error check for rows.Err() after iteration
- Consider using pgx.CollectRows for cleaner scanning
- SQL query should be a constant
+const queryLoadAllBooks = "SELECT id, title FROM books"
+
func (db *postgresDB) LoadAllBooks(ctx context.Context) ([]Book, error) {
- rows, err := db.pool.Query(ctx, "SELECT id, title FROM books")
+ rows, err := db.pool.Query(ctx, queryLoadAllBooks)
if err != nil {
return nil, fmt.Errorf("failed to query books table: %w", err)
}
defer rows.Close()
- var books []Book
- for rows.Next() {
- var record Book
- err := rows.Scan(&record.ID, &record.Title)
- if err != nil {
- return nil, fmt.Errorf("failed to scan rows: %w", err)
- }
- books = append(books, record)
+ books, err := pgx.CollectRows(rows, pgx.RowToStructByName[Book])
+ if err != nil {
+ return nil, fmt.Errorf("failed to collect rows: %w", err)
}
return books, nil
}Committable suggestion skipped: line range outside the PR's diff.
| func newPostgresDB(ctx context.Context, databaseURL string) (Database, error) { | ||
| dbpool, err := pgxpool.New(ctx, databaseURL) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to create connection pool: %v", err) | ||
| } | ||
|
|
||
| return &postgresDB{ | ||
| pool: dbpool, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance connection pool initialization with validation and configuration
The constructor should validate the connection and allow customization of pool settings. Also, consider implementing a ping check to ensure the connection is valid.
-func newPostgresDB(ctx context.Context, databaseURL string) (Database, error) {
+func newPostgresDB(ctx context.Context, databaseURL string, opts *pgxpool.Config) (Database, error) {
+ if opts == nil {
+ config, err := pgxpool.ParseConfig(databaseURL)
+ if err != nil {
+ return nil, fmt.Errorf("invalid database URL: %w", err)
+ }
+ // Set default pool settings
+ config.MaxConns = 10
+ config.MinConns = 2
+ opts = config
+ }
- dbpool, err := pgxpool.New(ctx, databaseURL)
+ dbpool, err := pgxpool.NewWithConfig(ctx, opts)
if err != nil {
return nil, fmt.Errorf("unable to create connection pool: %v", err)
}
+
+ // Validate connection
+ if err := dbpool.Ping(ctx); err != nil {
+ dbpool.Close()
+ return nil, fmt.Errorf("failed to ping database: %w", err)
+ }
return &postgresDB{
pool: dbpool,
}, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func newPostgresDB(ctx context.Context, databaseURL string) (Database, error) { | |
| dbpool, err := pgxpool.New(ctx, databaseURL) | |
| if err != nil { | |
| return nil, fmt.Errorf("unable to create connection pool: %v", err) | |
| } | |
| return &postgresDB{ | |
| pool: dbpool, | |
| }, nil | |
| } | |
| func newPostgresDB(ctx context.Context, databaseURL string, opts *pgxpool.Config) (Database, error) { | |
| if opts == nil { | |
| config, err := pgxpool.ParseConfig(databaseURL) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid database URL: %w", err) | |
| } | |
| // Set default pool settings | |
| config.MaxConns = 10 | |
| config.MinConns = 2 | |
| opts = config | |
| } | |
| dbpool, err := pgxpool.NewWithConfig(ctx, opts) | |
| if err != nil { | |
| return nil, fmt.Errorf("unable to create connection pool: %v", err) | |
| } | |
| // Validate connection | |
| if err := dbpool.Ping(ctx); err != nil { | |
| dbpool.Close() | |
| return nil, fmt.Errorf("failed to ping database: %w", err) | |
| } | |
| return &postgresDB{ | |
| pool: dbpool, | |
| }, nil | |
| } |
| package database | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/pashagolub/pgxmock/v4" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestPostgresDB_GetBooks(t *testing.T) { | ||
| mockPool, err := pgxmock.NewPool() | ||
| assert.Nil(t, err) | ||
|
|
||
| mockPool.ExpectQuery("SELECT id, title FROM books"). | ||
| WillReturnRows(pgxmock.NewRows([]string{"id", "title"}). | ||
| AddRow(1, "book1")) | ||
|
|
||
| db := postgresDB{ | ||
| pool: mockPool, | ||
| } | ||
| result, err := db.LoadAllBooks(context.Background()) | ||
| assert.Nil(t, err) | ||
| assert.Equal(t, 1, len(result)) | ||
| assertBook(t, result[0], 1, Book{Title: "book1"}) | ||
|
|
||
| assert.Nil(t, mockPool.ExpectationsWereMet()) | ||
| } | ||
|
|
||
| func TestPostgresDB_GetBooks_Fail(t *testing.T) { | ||
| mockPool, err := pgxmock.NewPool() | ||
| assert.Nil(t, err) | ||
|
|
||
| mockPool.ExpectQuery("SELECT id, title FROM books"). | ||
| WillReturnError(assert.AnError) | ||
|
|
||
| db := postgresDB{ | ||
| pool: mockPool, | ||
| } | ||
| result, err := db.LoadAllBooks(context.Background()) | ||
| assert.Nil(t, result) | ||
| assert.ErrorContains(t, err, "failed to query books table") | ||
|
|
||
| assert.Nil(t, mockPool.ExpectationsWereMet()) | ||
| } | ||
|
|
||
| func TestPostgresDB_CreateBook(t *testing.T) { | ||
| mockPool, err := pgxmock.NewPool() | ||
| assert.Nil(t, err) | ||
|
|
||
| mockPool.ExpectExec("INSERT INTO books"). | ||
| WithArgs("book1"). | ||
| WillReturnResult(pgxmock.NewResult("INSERT", 1)) | ||
|
|
||
| db := postgresDB{ | ||
| pool: mockPool, | ||
| } | ||
| err = db.CreateBook(context.Background(), Book{Title: "book1"}) | ||
| assert.Nil(t, err) | ||
|
|
||
| assert.Nil(t, mockPool.ExpectationsWereMet()) | ||
| } | ||
|
|
||
| func TestPostgresDB_CreateBook_Fail(t *testing.T) { | ||
| mockPool, err := pgxmock.NewPool() | ||
| assert.Nil(t, err) | ||
|
|
||
| mockPool.ExpectExec("INSERT INTO books"). | ||
| WithArgs("book1"). | ||
| WillReturnError(assert.AnError) | ||
|
|
||
| db := postgresDB{ | ||
| pool: mockPool, | ||
| } | ||
| err = db.CreateBook(context.Background(), Book{Title: "book1"}) | ||
| assert.ErrorContains(t, err, "failed to insert book") | ||
|
|
||
| assert.Nil(t, mockPool.ExpectationsWereMet()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for context handling and input validation
The test suite would benefit from additional test cases:
- Context cancellation/timeout scenarios
- Input validation tests (empty title, invalid characters, etc.)
Example test for context cancellation:
func TestPostgresDB_GetBooks_ContextCancelled(t *testing.T) {
mockPool, err := pgxmock.NewPool()
assert.Nil(t, err)
ctx, cancel := context.WithCancel(context.Background())
cancel() // Cancel context immediately
db := postgresDB{pool: mockPool}
result, err := db.LoadAllBooks(ctx)
assert.Nil(t, result)
assert.ErrorIs(t, err, context.Canceled)
}| func TestAddBook(t *testing.T) { | ||
| mockService := new(services.BooksServiceMock) | ||
| mockService.On("SaveBook", mock.Anything, domain.Book{Title: "Title"}).Return(nil) | ||
|
|
||
| app := fiber.New() | ||
| app.Post(booksRoute, AddBook(mockService)) | ||
|
|
||
| resp, err := app.Test(postRequest(booksRoute, `{"title":"Title"}`)) | ||
| assert.Nil(t, err) | ||
| assert.Equal(t, 201, resp.StatusCode) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation test cases for AddBook.
Consider adding test cases for input validation:
- Empty book title
- Very long book title
- Special characters in title
func TestAddBook_ValidationCases(t *testing.T) {
testCases := []struct {
name string
requestBody string
expectedCode int
}{
{
name: "empty title",
requestBody: `{"title":""}`,
expectedCode: 400,
},
{
name: "very long title",
requestBody: fmt.Sprintf(`{"title":"%s"}`, strings.Repeat("a", 1001)),
expectedCode: 400,
},
{
name: "title with special chars",
requestBody: `{"title":"<script>alert('xss')</script>"}`,
expectedCode: 400,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
mockService := new(services.BooksServiceMock)
app := fiber.New()
app.Post(booksRoute, AddBook(mockService))
resp, err := app.Test(postRequest(booksRoute, tc.requestBody))
assert.Nil(t, err)
assert.Equal(t, tc.expectedCode, resp.StatusCode)
})
}
}| func bodyFromResponse[T any](t *testing.T, resp *http.Response) T { | ||
| var body T | ||
| err := json.NewDecoder(resp.Body).Decode(&body) | ||
| assert.Nil(t, err) | ||
| return body | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and add response body cleanup.
The helper function should:
- Close the response body to prevent resource leaks
- Handle JSON decoding errors more gracefully
-func bodyFromResponse[T any](t *testing.T, resp *http.Response) T {
+func bodyFromResponse[T any](t *testing.T, resp *http.Response) T {
+ defer resp.Body.Close()
var body T
err := json.NewDecoder(resp.Body).Decode(&body)
- assert.Nil(t, err)
+ if err != nil {
+ t.Fatalf("failed to decode response body: %v", err)
+ }
return body
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func bodyFromResponse[T any](t *testing.T, resp *http.Response) T { | |
| var body T | |
| err := json.NewDecoder(resp.Body).Decode(&body) | |
| assert.Nil(t, err) | |
| return body | |
| } | |
| func bodyFromResponse[T any](t *testing.T, resp *http.Response) T { | |
| defer resp.Body.Close() | |
| var body T | |
| err := json.NewDecoder(resp.Body).Decode(&body) | |
| if err != nil { | |
| t.Fatalf("failed to decode response body: %v", err) | |
| } | |
| return body | |
| } |
| @@ -0,0 +1,26 @@ | |||
| ## Clean code example for Fiber and PostgreSQL | |||
|
|
|||
| This is an example of a RESTful API built using the Fiber framework (https://gofiber.io/) and PostgreSQL as the database. | |||
There was a problem hiding this comment.
please describe more about the part of the clean code here
| @@ -0,0 +1,26 @@ | |||
| ## Clean code example for Fiber and PostgreSQL | |||
There was a problem hiding this comment.
pls add a link in the README.md in root folder to this file
| DatabaseURL string | ||
| } | ||
|
|
||
| func NewConfiguration() *Configuration { |
There was a problem hiding this comment.
https://github.com/Pungyeon/clean-go-article#Comments
can you add comments to all public vars/functions/structs
This PR provides alternative clean code version to clean-architecture and hexagonal recipes. This includes:
pgxlibrary can be used and unit testedSummary by CodeRabbit
New Features
Documentation
Tests