Identify code duplication in matter.js #2731
Conversation
…ations This commit adds detailed analysis documentation for code duplication found in the matter.js codebase, along with prioritized refactoring recommendations and quick-win action items. Analysis findings: - 390+ files contain duplication patterns - ~8,171+ lines of duplicated code identified - Potential 6.5% code reduction (~25,000 lines) - 15-20% reduction possible in high-impact areas Documentation includes: - CODE_DUPLICATION_ANALYSIS.md: Complete analysis with categorized findings - REFACTORING_RECOMMENDATIONS.md: Phased implementation strategy - QUICK_WINS.md: Immediate action items for quick wins Key categories identified: 1. Exact duplication (149+ files, 2,280+ lines) - Empty measurement server classes (13 files) - Client implementation boilerplate (130+ files) - Session type guards (3 files) - Helper methods (3 files) 2. Semantic duplication (12 files, 441 lines) - Operational state validation (3 files, 95%+ similar) - Mode server implementations (6 files, 70-90% similar) - Session lifecycle management (3 files) - Session crypto operations (2 files) 3. Generated code patterns (204+ files, 5,000+ lines) - Behavior.ts templates (130+ files) - Device/endpoint definitions (74 files) 4. Codec patterns (18+ files, 300+ lines) - DataReader/DataWriter setup - Flag extraction patterns - BDX schema encode/decode Implementation strategy organized in 4 phases: - Phase 1: Quick wins (1-2 weeks, 392 lines) - Phase 2: Semantic consolidation (2-3 weeks, 512 lines) - Phase 3: Generator optimization (1 week, 7,000+ lines) - Phase 4: Codec utilities (1-2 weeks, 250 lines) All recommendations include implementation examples, testing strategies, and risk assessments.
| import { createMeasurementServer } from "../utils/MeasurementServerFactory.js"; | ||
|
|
||
| export const TemperatureMeasurementServer = createMeasurementServer(TemperatureMeasurementBehavior); | ||
| export type TemperatureMeasurementServer = InstanceType<typeof TemperatureMeasurementServer>; |
There was a problem hiding this comment.
This... seems more verbose? Also using "type" vs class or interface will explode type definition sizes
| this.assert = guard.assert.bind(guard); | ||
| } | ||
|
|
||
| static is: (session: unknown) => session is SecureSession; |
There was a problem hiding this comment.
This redundancy can be handled more simply by creating single methods on the base class with type parameter for this
| import { OperationalStateValidation } from "../utils/OperationalStateValidation.js"; | ||
|
|
||
| export class OperationalStateServer extends OperationalStateBehavior { | ||
| #validation = new OperationalStateValidation(this.state, this.events, this.context); |
There was a problem hiding this comment.
This will create even if unused. Instead create on demand as needed?
Also, don't see benefit of passing properties as separate parameters. Just pass this instead
| export class OperationalStateServer extends OperationalStateBehavior { | ||
| #validation = new OperationalStateValidation(this.state, this.events, this.context); | ||
|
|
||
| #assertOperationalStateList() { |
There was a problem hiding this comment.
These methods seem unnecessary. Just invoke on utility class directly
| import { ModeServerBase } from "../utils/ModeServerBase.js"; | ||
| import { DishwasherMode } from "#types"; | ||
|
|
||
| export class DishwasherModeServer extends ModeServerBase<DishwasherModeBehavior> { |
There was a problem hiding this comment.
This loses inheritance of *Behavior. Should be handled with composition of utility class instead
| * Creates a simple schema for BDX messages with automatic encoding/decoding. | ||
| * Eliminates boilerplate for schemas with primitive field types. | ||
| */ | ||
| export function createSimpleSchema<T extends Record<string, any>>( |
There was a problem hiding this comment.
Interesting but this didn't seem BDX specific.
Also prefer names like SimpleSchema over createSimpleScema for factories
| import { createClientBehavior } from "../utils/ClientBehaviorFactory.js"; | ||
| import { OnOff } from "#types"; | ||
|
|
||
| export const OnOffClient = createClientBehavior(OnOff.Complete); |
There was a problem hiding this comment.
No, this change simply moves away from preferred naming convention
| import { OnOff } from "#types"; | ||
|
|
||
| export const OnOffClient = createClientBehavior(OnOff.Complete); | ||
| export type OnOffClient = InstanceType<typeof OnOffClient>; |
There was a problem hiding this comment.
No... This is semantically equivalent and indeed how we originally implemented. Practical result would be massive duplication in generated type definition files, resulting in order-of-magnitude increase in file size and 2x or 3x compile time penalty
| ``` | ||
|
|
||
| **Generator Changes**: | ||
| - Simplify template to use type aliases instead of interface + const |
There was a problem hiding this comment.
No, same issue as previous suggestion
|
|
||
| ```typescript | ||
| // /packages/node/src/devices/utils/LightingDeviceFactory.ts | ||
| export function createLightingDevice(config: { |
There was a problem hiding this comment.
No, this loses critical type information. And inferring the common patterns would be a non-trivial undertaking akin to coding all devices manually... With no runtime benefit that I can discern
…ations
This commit adds detailed analysis documentation for code duplication found in the matter.js codebase, along with prioritized refactoring recommendations and quick-win action items.
Analysis findings:
Documentation includes:
Key categories identified:
Exact duplication (149+ files, 2,280+ lines)
Semantic duplication (12 files, 441 lines)
Generated code patterns (204+ files, 5,000+ lines)
Codec patterns (18+ files, 300+ lines)
Implementation strategy organized in 4 phases:
All recommendations include implementation examples, testing strategies, and risk assessments.