# Code Review Report: AFASystems Presence Services

**Review Date:** February 11, 2025  
**Scope:** Four services (bridge, server, location, decoder) and their internal packages  
**Reviewer:** Automated Code Review

---

## Executive Summary

This report reviews the custom packages used across the four main services of the presence detection system. The codebase demonstrates a coherent architecture with clear separation of concerns, but several reliability issues, unused code paths, and refactoring opportunities were identified.

**Overall Rating: 6.5/10**

---

## 1. Package Inventory by Service

### Bridge (`cmd/bridge/main.go`)

| Package                          | Purpose                                            |
| -------------------------------- | -------------------------------------------------- |
| `internal/pkg/common/appcontext` | Shared application state (beacon lookup, settings) |
| `internal/pkg/config`            | Environment-based configuration                    |
| `internal/pkg/kafkaclient`       | Kafka consumer/producer management                 |
| `internal/pkg/logger`            | Structured logging setup                           |
| `internal/pkg/model`             | Data structures                                    |

### Server (`cmd/server/main.go`)

| Package                          | Purpose                                       |
| -------------------------------- | --------------------------------------------- |
| `internal/pkg/apiclient`         | External API authentication and data fetching |
| `internal/pkg/common/appcontext` | Shared application state                      |
| `internal/pkg/config`            | Configuration                                 |
| `internal/pkg/controller`        | HTTP handlers for REST API                    |
| `internal/pkg/database`          | PostgreSQL connection (GORM)                  |
| `internal/pkg/kafkaclient`       | Kafka management                              |
| `internal/pkg/logger`            | Logging                                       |
| `internal/pkg/model`             | Data structures                               |
| `internal/pkg/service`           | Business logic (location, parser)             |

### Location (`cmd/location/main.go`)

| Package                          | Purpose                |
| -------------------------------- | ---------------------- |
| `internal/pkg/common/appcontext` | Beacon state, settings |
| `internal/pkg/common/utils`      | Distance calculation   |
| `internal/pkg/config`            | Configuration          |
| `internal/pkg/kafkaclient`       | Kafka management       |
| `internal/pkg/logger`            | Logging                |
| `internal/pkg/model`             | Data structures        |

### Decoder (`cmd/decoder/main.go`)

| Package                          | Purpose                            |
| -------------------------------- | ---------------------------------- |
| `internal/pkg/common/appcontext` | Beacon events state                |
| `internal/pkg/common/utils`      | AD structure parsing, flag removal |
| `internal/pkg/config`            | Configuration                      |
| `internal/pkg/kafkaclient`       | Kafka management                   |
| `internal/pkg/logger`            | Logging                            |
| `internal/pkg/model`             | Data structures, parser registry   |

---

## 2. Critical Issues (Must Fix)

### 2.1 Tracker Delete Method Case Mismatch (Bug)

Resolved

### 2.2 Potential Panic in Location Algorithm

**Location:** `cmd/location/main.go:99-101`

Resolved

### 2.3 Hardcoded Config Path

**Location:** `cmd/server/main.go:60`

```go
configFile, err := os.Open("/app/cmd/server/config.json")
```

This path is Docker-specific and fails in local development or other deployment environments.

**Fix:** Use configurable path (e.g., `CONFIG_PATH` env var) or relative path based on executable location.

---

## 3. Security Concerns

### 3.1 Hardcoded Credentials

**Locations:**

- `internal/pkg/config/config.go`: Default values include `ClientSecret`, `HTTPPassword` with production-like strings
- `internal/pkg/apiclient/auth.go`: GetToken() hardcodes credentials in formData (lines 21-24) instead of using `cfg.HTTPClientID`, `cfg.ClientSecret`, etc.
- Config struct has `HTTPClientID`, `ClientSecret`, `HTTPUsername`, etc., but `auth.go` ignores them

**Recommendation:** Wire config values into auth; never commit production credentials.

### 3.2 TLS Verification Disabled

**Location:** `internal/pkg/apiclient/updatedb.go:21-22`

```go
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
```

**Recommendation:** Use proper certificates or make this configurable for dev only.

---

## 4. Unused Code & Directories

### 4.1 Orphaned Packages (Not Imported)

Resolved

### 4.2 Unused Functions / Methods

Resolved

### 4.3 Dead Code in bridge/mqtthandler

**Location:** `internal/pkg/bridge/mqtthandler/mqtthandler.go:75-83`

Resolved

### 4.4 Unnecessary Compile-Time Assertion

**Location:** `cmd/server/main.go:31`

```go
var _ io.Writer = (*os.File)(nil)
```

Redundant; `*os.File` implements `io.Writer`. Safe to remove.

### 4.5 Unused go.mod Dependencies

| Package                        | Notes                           |
| ------------------------------ | ------------------------------- |
| `github.com/boltdb/bolt`       | Not imported in any source file |
| `github.com/yosssi/gmq`        | Not imported                    |
| `github.com/gorilla/websocket` | Not imported                    |

**Recommendation:** Run `go mod tidy` after removing dead imports, or explicitly remove if kept for future use.

---

## 5. Reliability & Error Handling

### 5.1 Kafka Consumer Error Handling

**Location:** `internal/pkg/kafkaclient/consumer.go:20-23`

On `ReadMessage` or `Unmarshal` error, the consumer logs and continues. For `context.Canceled` or partition errors, this may cause tight loops. Consider backoff or bounded retries.

### 5.2 KafkaManager GetReader/GetWriter Lock Usage

**Location:** `internal/pkg/kafkaclient/manager.go:101-111`

`GetReader` and `GetWriter` hold the lock for the entire call including return. If the returned pointer is used after the lock is released, that's fine, but the pattern holds the lock longer than necessary. Prefer:

```go
func (m *KafkaManager) GetReader(topic string) *kafka.Reader {
    m.kafkaReadersMap.KafkaReadersLock.RLock()
    defer m.kafkaReadersMap.KafkaReadersLock.RUnlock()
    return m.kafkaReadersMap.KafkaReaders[topic]
}
```

Use `RLock` for read-only access.

### 5.3 Logger File Handle Leak

**Location:** `internal/pkg/logger/logger.go`

The opened file `f` is never closed. For long-running processes this is usually acceptable (log files stay open), but worth documenting. If multiple loggers are created, each holds a file descriptor.

### 5.4 Silent JSON Unmarshal

**Location:** `cmd/server/main.go:68`

```go
json.Unmarshal(b, &configs)
```

Error is ignored. Invalid JSON would leave `configs` empty without feedback.

---

## 6. Code Quality & Maintainability

### 6.1 Inconsistent Logging

- Mix of `log.Printf`, `fmt.Println`, `fmt.Printf`, and `slog.Info/Error`
- Italian message: "Messaggio CSV non valido" in bridge
- Typo: "Beggining" → "Beginning" (bridge, location, decoder, server)

### 6.2 Magic Numbers

- Channel sizes: 200, 500, 2000 without named constants
- RSSI weights in location: `seenW := 1.5`, `rssiW := 0.75`
- Ticker intervals: 1s, 2s without configuration

### 6.3 Duplication

- Bridge defines `mqtthandler` inline while `internal/pkg/bridge/mqtthandler` exists with similar logic
- Both use `appcontext.BeaconExists` for lookup; bridge version also sets `adv.ID` from lookup

### 6.4 Parser ID Inconsistency

**Decoder** expects `msg.ID` values: `"add"`, `"delete"`, `"update"`.  
**ParserDeleteController** sends `ID: "delete"` ✓  
**ParserAddController** sends `ID: "add"` ✓  
**ParserUpdateController** sends `ID: "update"` ✓

Decoder’s update case re-registers; add and update are effectively the same.

---

## 7. AppContext Thread Safety

`AppState` mixes safe and unsafe access:

- `beaconsLookup` (map) has no mutex; `AddBeaconToLookup`, `RemoveBeaconFromLookup`, `CleanLookup`, `BeaconExists` are not thread-safe
- Bridge goroutines (Kafka consumers + event loop) and MQTT handler may access it concurrently

**Recommendation:** Protect `beaconsLookup` with `sync.RWMutex` or use `sync.Map`.

---

## 8. Refactoring Suggestions

1. **Unify config loading:** Support JSON config file path via env; keep env overrides for sensitivity.
2. **Extract constants:** Kafka topic names, channel sizes, ticker intervals.
3. **Consolidate MQTT handling:** Use `internal/pkg/bridge/mqtthandler` and fix it, or remove the package and keep logic in bridge.
4. **API client:** Use config for URLs and credentials; add timeouts to HTTP client.
5. **Controllers:** Add request validation, consistent error responses, and structured error types.
6. **Service layer:** `formatMac` in `beacon_service.go` could move to `internal/pkg/common/utils` for reuse and testing.

---

## 9. Directory Structure Notes

| Directory                                  | Status                                                |
| ------------------------------------------ | ----------------------------------------------------- |
| `internal/app/_your_app_/`                 | Placeholder with `.keep`; safe to remove or repurpose |
| `internal/pkg/model/.keep`                 | Placeholder; low impact                               |
| `web/app/`, `web/static/`, `web/template/` | Empty except `.keep`; clarify if planned              |
| `build/package/`                           | Contains Dockerfiles; structure is reasonable         |

---

## 10. Summary of Recommendations

| Priority | Action                                                             |
| -------- | ------------------------------------------------------------------ |
| **P0**   | Fix TrackerDelete `"Delete"` → `"DELETE"`                          |
| **P0**   | Guard empty `BeaconMetrics` in location                            |
| **P1**   | Make config.json path configurable                                 |
| **P1**   | Fix `beaconsLookup` concurrency in AppState                        |
| **P2**   | Remove or integrate `internal/pkg/redis` and `internal/pkg/bridge` |
| **P2**   | Remove unused functions (ValidateRSSI, EventToBeaconService, etc.) |
| **P2**   | Replace hardcoded credentials in apiclient with config             |
| **P3**   | Unify logging (slog), fix typos, extract constants                 |
| **P3**   | Run `go mod tidy` and drop unused dependencies                     |

---

## 11. Rating Breakdown

| Category         | Score | Notes                                                                   |
| ---------------- | ----- | ----------------------------------------------------------------------- |
| Architecture     | 7/10  | Clear service boundaries; some shared-state issues                      |
| Reliability      | 5/10  | Critical bugs (case mismatch, panic risk); error handling could improve |
| Security         | 4/10  | Hardcoded credentials; disabled TLS verification                        |
| Maintainability  | 6/10  | Duplication, magic numbers, inconsistent logging                        |
| Code Cleanliness | 5/10  | Unused code, dead packages, redundant assertions                        |

**Overall: 6.5/10**

The system has a solid foundation and sensible separation of concerns. Addressing the critical bugs, security issues, and removing dead code would materially improve reliability and maintainability.