Skip to content

CODE_REVIEW.md

Latest commit

 

History

History
213 lines (147 loc) · 5.55 KB

CODE_REVIEW.md

File metadata and controls

213 lines (147 loc) · 5.55 KB

Code Review Analysis

Last Updated: 2026-02-05

Overview

This is a well-structured Go CLI for the Rebrickable LEGO API. The architecture follows good separation of concerns with distinct cmd and api packages. However, there are several areas that need improvement.

Progress: 7 issues fixed, 3 remaining.

Critical Issues

1. Login Function Returns Success on Failure ✅ FIXED

Location: cli/cmd/user.go:61-66

Status: This issue has been resolved. The login function now properly returns errors:

if err != nil {
    return nil, fmt.Errorf("login request failed: %w", err)
}
if resp.StatusCode() != 200 {
    return nil, fmt.Errorf("login failed with status %d", resp.StatusCode())
}

2. Silent Error Discarding Throughout API Package ✅ FIXED

Status: Resolved during API client refactoring. All API methods now return error and callers handle them properly.

3. Silent Error Discarding in Commands ✅ FIXED

Status: Resolved. Command handlers now check and return errors from API calls and JSON marshaling.

Design Issues

4. Weak Type Safety for Results ✅ FIXED

Location: cli/cmd/api/api.go:100-123

Status: This issue has been resolved. Proper typed structs now replace the untyped map[string]any:

type Set struct {
    SetNum         string `json:"set_num"`
    Name           string `json:"name"`
    Year           int    `json:"year"`
    ThemeID        int    `json:"theme_id"`
    NumParts       int    `json:"num_parts"`
    SetImgURL      string `json:"set_img_url"`
    SetURL         string `json:"set_url"`
    LastModifiedDt string `json:"last_modified_dt"`
}

type UserSet struct {
    ListID        int  `json:"list_id"`
    Quantity      int  `json:"quantity"`
    IncludeSpares bool `json:"include_spares"`
    Set           Set  `json:"set"`
}

type SetsResponse struct {
    Count    int       `json:"count"`
    Next     string    `json:"next"`
    Previous string    `json:"previous"`
    Results  []UserSet `json:"results"`
}

5. Unsafe Context Type Assertions

Location: cli/cmd/sets.go (8 places)

authToken := cmd.Context().Value(AuthToken).(string)
apiKey := cmd.Context().Value(ApiKey).(string)

Problem: If context values are missing, this panics.

Fix: Use safe type assertions with error handling:

authToken, ok := cmd.Context().Value(AuthToken).(string)
if !ok || authToken == "" {
    return fmt.Errorf("not authenticated")
}

6. Context Keys Should Be Typed

Location: cli/cmd/user.go:17-20

const (
    ApiKey    = "api_key"
    AuthToken = "auth_token"
)

Problem: String keys can collide with other packages. Go idiom is to use unexported typed keys.

Fix:

type contextKey string

const (
    apiKeyKey    contextKey = "api_key"
    authTokenKey contextKey = "auth_token"
)

Code Duplication

7. HTTP Client Created Multiple Times ✅ FIXED

Status: Resolved. The api.Client struct now encapsulates the HTTP client, created once via api.NewClient(). Command handlers use newAPIClient(cmd) helper.

8. Repeated Header Setup ✅ FIXED

Status: Resolved. Headers are now configured once in NewClient():

func NewClient(apiKey, authToken string) *Client {
    http := resty.New().
        SetBaseURL(apiBaseURI).
        SetHeader("Content-Type", "application/json").
        SetHeader("Authorization", fmt.Sprintf("key %s", apiKey))
    return &Client{http: http, authToken: authToken}
}

Minor Issues

9. Unnecessary fmt.Sprintf ✅ FIXED

Status: Resolved during API client refactoring. Now uses direct concatenation and fmt.Printf.

10. Global Mutable State for Flags

Location: cli/cmd/sets.go:12-13

var setNumber string
var setListName string

Problem: Module-level mutable variables can cause issues with testing and concurrent usage.

11. Undocumented Magic Suffix

Location: cli/cmd/sets.go:126-132

func adjustedSetNumber() string {
    if !strings.HasSuffix(setNumber, "-1") {
        return setNumber + "-1"
    }
    return setNumber
}

Problem: The "-1" suffix requirement is unexplained. A comment would help explain this is the Rebrickable set variant convention.

12. Missing Required Flag Validation

Commands like saveSetsCmd don't validate that required flags are provided before making API calls.

Summary Table

Priority Issue Location Status
Critical Login returns nil on failure user.go:60-63 ✅ Fixed
Critical Errors silently discarded api.go ✅ Fixed
High Unsafe type assertions sets.go (8 places) ❌ Open
High Weak type safety api.go:100-123 ✅ Fixed
Medium Code duplication HTTP client/headers ✅ Fixed
Medium Untyped context keys user.go:17-20 ❌ Open
Low Global flag variables sets.go:12-13 ❌ Open
Low Unnecessary fmt.Sprintf api.go ✅ Fixed

What's Done Well

  • Clean separation between cmd and api packages
  • Proper use of Cobra's PersistentPreRunE for authentication
  • Credentials kept in environment variables (not hardcoded)
  • Bazel build setup with proper dependency management
  • Integration tests using testscript framework
  • Token masking in debug output (api.go:89)

Maintenance

To refresh this document after making code changes:

check CODE_REVIEW.md, review the code and update the doc