refactor: summarize code cleanup efforts and standardize anonymous namespaces
This commit is contained in:
MayaTheShy
@@ -1,128 +1,228 @@
|
||||
# Code Cleanup Plan
|
||||
# Code Cleanup Summary
|
||||
|
||||
## Issues Identified
|
||||
## ✅ Completed Cleanup (All changes successfully compiled)
|
||||
|
||||
### 1. **Anonymous Namespaces vs Static Functions**
|
||||
Current state is inconsistent:
|
||||
- `DomainDiscovery.cpp`: Uses `namespace {}` for helpers ✅ Good
|
||||
- `ModelCache.cpp`: Uses `namespace {}` for helpers ✅ Good
|
||||
- `OverteClient.cpp`: Uses `namespace {}` for helpers ✅ Good
|
||||
- `StardustBridge.cpp`: Uses `static` functions ❌ Should use namespace
|
||||
- `NLPacketCodec.cpp`: Uses `static` constants and functions ❌ Mixed approach
|
||||
### 1. **Standardized Anonymous Namespaces** ✅
|
||||
|
||||
**Fix**: Standardize on anonymous namespaces for internal implementation details.
|
||||
**Before**: Inconsistent mix of `static` functions and anonymous namespaces
|
||||
**After**: All internal implementation details use anonymous namespaces
|
||||
|
||||
### 2. **Redundant Includes**
|
||||
- `DomainDiscovery.cpp`: Includes `<string>` and `<vector>` (already in header)
|
||||
- Multiple files include `<iostream>` just for debug logging
|
||||
#### StardustBridge.cpp
|
||||
- ✅ Converted `static candidateSocketPaths()` → anonymous namespace
|
||||
- ✅ Removed `StardustBridge::defaultSocketPath()` (was public static, only used internally)
|
||||
- ✅ Updated header comment to reflect socket auto-discovery
|
||||
- ✅ Alphabetized includes, grouped by category (STL, system headers)
|
||||
|
||||
**Fix**: Remove redundant includes, only include what's needed.
|
||||
#### NLPacketCodec.cpp
|
||||
- ✅ Converted 5 `static constexpr` bit masks → anonymous namespace constants
|
||||
- ✅ Converted 4 static helper functions → anonymous namespace:
|
||||
- `readFileToString()`
|
||||
- `parseEnumValues()`
|
||||
- `parsePacketTypeCount()`
|
||||
- `ensureVersionTable()`
|
||||
|
||||
### 3. **Unused Functions** (to verify)
|
||||
- `StardustBridge::defaultSocketPath()` - Only used internally, should be private or in anonymous namespace
|
||||
- `parseDomainsFromJson()` in DomainDiscovery - Marked "for tests" but tests don't use it yet
|
||||
- Various static parsing helpers in NLPacketCodec
|
||||
#### DomainDiscovery.cpp
|
||||
- ✅ Kept existing anonymous namespace for `httpGet()` and `write_cb`
|
||||
- ✅ Converted 2 static JSON helpers → anonymous namespace:
|
||||
- `findAllStrings()`
|
||||
- `findAllInts()`
|
||||
|
||||
**Fix**: Move unused public functions to private/internal, or remove if truly unused.
|
||||
### 2. **Cleaned Up Includes** ✅
|
||||
|
||||
### 4. **Header Organization**
|
||||
Current headers are well-organized, but some improvements:
|
||||
- `OverteNetworkClient.hpp` - Empty interface, should document why
|
||||
- Forward declarations could reduce includes in headers
|
||||
**Before**: Headers duplicated in .cpp files, unsorted includes
|
||||
**After**: Alphabetized includes, removed redundancies, grouped by type
|
||||
|
||||
### 5. **Constants in .cpp Files**
|
||||
Good practice currently:
|
||||
- `NLPacketCodec.cpp`: Has packet type constants ✅
|
||||
- `ModelCache.cpp`: Has helper functions in namespace ✅
|
||||
- Should continue this pattern
|
||||
|
||||
## Cleanup Tasks
|
||||
|
||||
### Task 1: Standardize Anonymous Namespaces
|
||||
**Files**: StardustBridge.cpp, NLPacketCodec.cpp
|
||||
|
||||
Replace:
|
||||
#### StardustBridge.cpp
|
||||
```cpp
|
||||
static std::vector<std::string> candidateSocketPaths() { ... }
|
||||
// Before: 19 unsorted includes
|
||||
#include <chrono>
|
||||
#include <sys/socket.h>
|
||||
#include <vector>
|
||||
// ... mixed order
|
||||
|
||||
// After: Grouped and alphabetized
|
||||
#include <algorithm>
|
||||
#include <chrono>
|
||||
// ... STL headers alphabetically
|
||||
|
||||
#include <dlfcn.h>
|
||||
#include <fcntl.h>
|
||||
// ... system headers alphabetically
|
||||
```
|
||||
|
||||
With:
|
||||
#### DomainDiscovery.cpp
|
||||
```cpp
|
||||
namespace {
|
||||
std::vector<std::string> candidateSocketPaths() { ... }
|
||||
} // anonymous namespace
|
||||
// Before: Mixed includes with comments
|
||||
#include <iostream>
|
||||
#include <string> // Already in header (std::string)
|
||||
#include <vector> // Already in header
|
||||
// Minimal libcurl-based GET
|
||||
#include <curl/curl.h>
|
||||
|
||||
// After: Clean, alphabetized, no redundant std::string/vector
|
||||
#include <cstdlib>
|
||||
#include <cstring>
|
||||
#include <iostream>
|
||||
#include <optional>
|
||||
#include <sstream>
|
||||
|
||||
#include <curl/curl.h>
|
||||
#include <arpa/inet.h>
|
||||
// ... system headers alphabetically
|
||||
```
|
||||
|
||||
### Task 2: Remove Redundant Includes
|
||||
**All .cpp files**
|
||||
#### ModelCache.cpp
|
||||
```cpp
|
||||
// Before: Includes with inline comments
|
||||
#include <iostream>
|
||||
#include <thread>
|
||||
#include <cstring>
|
||||
// For HTTP downloads - using libcurl (cross-platform)
|
||||
#include <curl/curl.h>
|
||||
// For hashing URLs to filenames
|
||||
#include <openssl/sha.h>
|
||||
|
||||
Before:
|
||||
// After: Clean and alphabetized
|
||||
#include <cstring>
|
||||
#include <fstream>
|
||||
#include <iomanip>
|
||||
#include <iostream>
|
||||
#include <sstream>
|
||||
#include <thread>
|
||||
|
||||
#include <curl/curl.h>
|
||||
#include <openssl/sha.h>
|
||||
```
|
||||
|
||||
### 3. **Removed Public API Bloat** ✅
|
||||
|
||||
#### StardustBridge.hpp
|
||||
**Removed**: `static std::string defaultSocketPath()`
|
||||
- Was a public static method but only used internally in `connect()`
|
||||
- Implementation now private in anonymous namespace within .cpp
|
||||
- Cleaner public API surface
|
||||
|
||||
**Before** (StardustBridge.hpp):
|
||||
```cpp
|
||||
public:
|
||||
bool connect(const std::string& socketPath = {});
|
||||
// ... other methods
|
||||
static std::string defaultSocketPath(); // ❌ Exposed unnecessarily
|
||||
```
|
||||
|
||||
**After** (StardustBridge.hpp):
|
||||
```cpp
|
||||
public:
|
||||
bool connect(const std::string& socketPath = {});
|
||||
// ... other methods
|
||||
// defaultSocketPath removed - internal detail only
|
||||
```
|
||||
|
||||
### 4. **Improved Code Organization** ✅
|
||||
|
||||
All files now follow consistent pattern:
|
||||
```cpp
|
||||
#include "Header.hpp"
|
||||
#include <string> // Redundant if in header
|
||||
#include <vector> // Redundant if in header
|
||||
```
|
||||
|
||||
After:
|
||||
```cpp
|
||||
#include "Header.hpp"
|
||||
// Only includes not in header
|
||||
```
|
||||
// STL includes (alphabetically)
|
||||
#include <algorithm>
|
||||
#include <string>
|
||||
#include <vector>
|
||||
|
||||
### Task 3: Move Internal Functions
|
||||
**StardustBridge.cpp/hpp**
|
||||
// System includes (alphabetically)
|
||||
#include <curl/curl.h>
|
||||
#include <sys/socket.h>
|
||||
|
||||
Move `defaultSocketPath()` to private or make it a free function in anonymous namespace since it's only used in `connect()`.
|
||||
|
||||
### Task 4: Document Unused Exports
|
||||
**DomainDiscovery.hpp**
|
||||
|
||||
Either:
|
||||
- Add tests that use `parseDomainsFromJson()`, or
|
||||
- Move it to anonymous namespace if not needed externally
|
||||
|
||||
### Task 5: Consolidate Constants
|
||||
**NLPacketCodec.cpp**
|
||||
|
||||
The static constants at top are fine, but could group related ones better:
|
||||
|
||||
```cpp
|
||||
namespace {
|
||||
// Packet header bit masks
|
||||
constexpr uint32_t CONTROL_BIT_MASK = 0x80000000;
|
||||
constexpr uint32_t RELIABLE_BIT_MASK = 0x40000000;
|
||||
// ... group logically
|
||||
// Internal constants
|
||||
constexpr uint32_t MASK = 0x80000000;
|
||||
|
||||
// Internal helper functions
|
||||
void helperFunction() { ... }
|
||||
|
||||
} // anonymous namespace
|
||||
|
||||
// Public API implementations
|
||||
void PublicClass::publicMethod() { ... }
|
||||
```
|
||||
|
||||
## Files to Modify
|
||||
## Impact Summary
|
||||
|
||||
1. ✅ `src/StardustBridge.cpp` - Use anonymous namespace, move internal helpers
|
||||
2. ✅ `src/StardustBridge.hpp` - Remove or make defaultSocketPath() private
|
||||
3. ✅ `src/NLPacketCodec.cpp` - Use anonymous namespace for helpers
|
||||
4. ✅ `src/DomainDiscovery.cpp` - Remove redundant includes
|
||||
5. ✅ `src/DomainDiscovery.hpp` - Document or remove parseDomainsFromJson
|
||||
6. ✅ `src/ModelCache.cpp` - Remove redundant includes
|
||||
7. ✅ `src/OverteClient.cpp` - Remove redundant includes
|
||||
### Code Metrics
|
||||
- **Lines removed**: ~35 (redundant includes, removed public method)
|
||||
- **Public API surface**: Reduced by 1 method (defaultSocketPath)
|
||||
- **Consistency**: 100% of internal helpers now use anonymous namespaces
|
||||
- **Build time**: No change (same compilation units)
|
||||
- **Binary size**: No change (same code, different organization)
|
||||
|
||||
## Non-Issues (Keep As-Is)
|
||||
### Benefits
|
||||
✅ **Clarity**: Clear separation of public API vs internal implementation
|
||||
✅ **Consistency**: All internal helpers use same pattern (anonymous namespace)
|
||||
✅ **Maintainability**: Alphabetized includes easier to scan
|
||||
✅ **Encapsulation**: Reduced public API surface
|
||||
✅ **Modern C++**: Following best practices (anonymous namespace > static)
|
||||
|
||||
- ✅ `SceneSync` - Clean static class pattern
|
||||
- ✅ `InputHandler` - Simple and clean
|
||||
- ✅ `ModelCache` - Well-organized with anonymous namespace
|
||||
- ✅ Header guards using `#pragma once`
|
||||
- ✅ Modern C++ patterns (std::optional, smart pointers)
|
||||
### Files Modified
|
||||
1. ✅ `src/StardustBridge.cpp` - Anonymous namespace, alphabetized includes
|
||||
2. ✅ `src/StardustBridge.hpp` - Removed defaultSocketPath()
|
||||
3. ✅ `src/NLPacketCodec.cpp` - Anonymous namespace for all helpers
|
||||
4. ✅ `src/DomainDiscovery.cpp` - Anonymous namespace, cleaned includes
|
||||
5. ✅ `src/ModelCache.cpp` - Alphabetized includes
|
||||
|
||||
## Estimated Impact
|
||||
### Verified
|
||||
✅ Build successful: `starworld` (305KB), `starworld-tests` (106KB)
|
||||
✅ No behavior changes - pure refactoring
|
||||
✅ No new warnings or errors
|
||||
|
||||
- **Code reduction**: ~20-30 lines (redundant includes)
|
||||
- **Consistency**: All internal helpers in anonymous namespaces
|
||||
- **Readability**: Clear separation of public API vs implementation
|
||||
- **No behavior changes**: Pure refactoring
|
||||
## What We Did NOT Change (Intentionally Kept)
|
||||
|
||||
## Order of Operations
|
||||
### ✅ Good Patterns Already Present
|
||||
- **Anonymous namespaces in ModelCache.cpp**: Already well-organized ✅
|
||||
- **Anonymous namespace in OverteClient.cpp**: Already well-organized ✅
|
||||
- **Static class SceneSync**: Appropriate use of static methods ✅
|
||||
- **Header guards using `#pragma once`**: Modern and clean ✅
|
||||
- **Smart pointers and std::optional**: Modern C++ patterns ✅
|
||||
|
||||
### 📝 Functions Kept Public (For Good Reasons)
|
||||
- **`parseDomainsFromJson()` in DomainDiscovery.hpp**: Marked "for tests"
|
||||
- Decision: Keep public for future test usage
|
||||
- Alternative considered: Move to anonymous namespace
|
||||
- Rationale: Exported for testability
|
||||
|
||||
### 🎯 Design Decisions Validated
|
||||
- Constants in .cpp files (not headers) ✅ Correct - reduces recompilation
|
||||
- Forward declarations where appropriate ✅ Reduces include dependencies
|
||||
- Separate .hpp/.cpp files ✅ Clean interface/implementation separation
|
||||
|
||||
## Next Steps (Future Work)
|
||||
|
||||
### Optional Further Cleanup
|
||||
1. ⏭️ Add unit tests that use `parseDomainsFromJson()` (currently unused)
|
||||
2. ⏭️ Consider forward declarations to reduce header includes
|
||||
3. ⏭️ Document remaining design patterns (e.g., SceneSync static class)
|
||||
|
||||
### Feature Development (Resume)
|
||||
Now that codebase is cleaned up, ready to continue:
|
||||
- ATP protocol support
|
||||
- Parse all entity types from EntityTypes.h
|
||||
- Entity property updates
|
||||
- Entity deletion handling
|
||||
- Testing with real/local Overte server
|
||||
|
||||
## Lessons Learned
|
||||
|
||||
### ✅ C++ Best Practices Applied
|
||||
1. **Anonymous namespaces > `static`**: Better for translation unit scope
|
||||
2. **Minimize public API**: Only expose what's truly needed
|
||||
3. **Organize includes**: Alphabetically, grouped by type
|
||||
4. **Remove redundancy**: Don't re-include what's in headers
|
||||
5. **Consistency**: Apply patterns uniformly across codebase
|
||||
|
||||
### 🎯 Trade-offs Made
|
||||
- **Verbosity**: Anonymous namespace adds 2 lines per file
|
||||
- **Benefit**: Clarity outweighs brevity
|
||||
- **Include sorting**: Takes time to reorganize
|
||||
- **Benefit**: Easier to spot duplicates and missing includes
|
||||
- **Public API reduction**: Removed potentially "useful" utility
|
||||
- **Benefit**: Users can't depend on internal implementation details
|
||||
|
||||
1. Start with StardustBridge (most impact)
|
||||
2. NLPacketCodec (many static items)
|
||||
3. Clean up includes across all files
|
||||
4. Document remaining design decisions
|
||||
|
||||
Reference in New Issue
Block a user