CODE_REVIEW_REPORT.md

# Code Review & Refactoring Report

## Overview

This report analyzes the current codebase (`sockets` project) and provides actionable feedback to improve maintainability, performance, and structure. The project uses **Express** and **ws** for a simple WebSocket server with room management.

---

## 1. Architectural Improvements

### Issue: specialized "Service" instantiated per message

Currently, a new `MessageService` instance is created for every single incoming message.

```typescript
// src/index.ts
new MessageService({ msg: msg.toString(), ... })
```

This is inefficient and semantically incorrect for a "Service". A Service should ideally be a singleton or a static utility that handles logic, rather than an ephemeral object representing a single request.

**Recommendation:**
Refactor `MessageService` to contain static methods or be a singleton instance that accepts the message and socket as arguments to a `handleMessage` method.

### Issue: Tight Coupling and Logic in Constructor

Critical business logic (parsing, routing, executing side effects) happens inside the `constructor` of `MessageService`.

```typescript
// src/services/MessageService.ts
constructor(...) {
    // ... logic ...
    if(action === 'join') ...
}
```

Constructors should primarily initialize state. Side effects (like broadcasting messages) should happen in explicit methods.

**Recommendation:**
Move logic out of the constructor. Create a `handleIncomingMessage(data: string, socket: WebSocket)` method.

---

## 2. Robustness and Error Handling

### Issue: Unsafe `JSON.parse`

If a client sends an invalid JSON string, `JSON.parse(msg)` inside `MessageService` will throw an error. Since this is not wrapped in a `try-catch` block, it could crash the application or unexpectedly terminate the socket connection.

**Recommendation:**
Wrap the parsing logic in a `try-catch` block. If parsing fails, send an error response to the sender and abort processing.

### Issue: Missing Input Validation

The code assumes `room_id`, `username`, and `action` always exist in the parsed payload.

```typescript
this.room_id = layers['room_id'] // Could be undefined
```

Accessing properties on undefined or unexpected data types can lead to runtime errors.

**Recommendation:**
Use a schema validation library (like **Zod**) or manual checks to ensure incoming data matches the expected structure (Interface) before processing.

---

## 3. Type Safety and TypeScript Best Practices

### Issue: `any` types implicitly

`JSON.parse` returns `any`. You should define an interface for your message structure.

```typescript
interface WebSocketMessage {
    room_id: string
    username: string
    message: string
    action: 'join' | 'leave' | 'message'
}
```

### Issue: Encapsulation

In `RoomServices`, the `map` property is public.

```typescript
map: Map<string, Map<string, WebSocket>>
```

This allows external code to modify the state directly, bypassing helper methods like `leaveRoom`.

**Recommendation:**
Make `map` private: `private map: Map<...>`.

---

## 4. Code Cleanliness & Manageability

### Magic Strings

Strings like `'join'` and `'leave'` are hardcoded in multiple places.
**Recommendation:** use an `enum` or constants for Action types.

```typescript
enum SocketAction {
    JOIN = 'join',
    LEAVE = 'leave',
    MESSAGE = 'message',
}
```

### Duplicate Logic

In `MessageService.broadcast`, the check for room existence is repeated redundant calls.

---

## Proposed Refactored Structure

### `src/types.ts`

Define shared interfaces and enums here.

### `src/services/RoomManager.ts` (Renamed from RoomServices)

Keep it focused on state management (add, remove, get users). Ensure data is private.

### `src/handlers/MessageHandler.ts` (Renamed from MessageService)

A stateless function or class that:

1.  Parses the message (safely).
2.  Validates the schema.
3.  Calls the appropriate action on `RoomServices`.

### `src/index.ts`

Initialize the `App` and pass the singletons/handlers to the WebSocket event listener.

---

## Summary of Next Steps

1.  **Safety**: Add `try-catch` around JSON parsing.
2.  **Organization**: Separate "Data Types" from "Logic".
3.  **Pattern**: Stop `new MessageService(...)` pattern. Use `MessageController.handle(...)`.
4.  **Resilience**: Validate all inputs before using them.