|
| 1 | +# Simplify Controller Command Handler Creation |
| 2 | + |
| 3 | +## 📋 Overview |
| 4 | + |
| 5 | +Remove unnecessary progress step for application command handler creation across all presentation layer controllers. Handler creation is instantaneous (just object construction with no I/O), so users don't need visibility into this internal implementation detail. |
| 6 | + |
| 7 | +**Target Files:** |
| 8 | + |
| 9 | +- `src/presentation/controllers/provision/handler.rs` |
| 10 | +- `src/presentation/controllers/configure/handler.rs` |
| 11 | +- `src/presentation/controllers/register/handler.rs` |
| 12 | +- `src/presentation/controllers/release/handler.rs` |
| 13 | +- `src/presentation/controllers/run/handler.rs` |
| 14 | +- Other controllers following similar pattern |
| 15 | + |
| 16 | +**Scope:** |
| 17 | + |
| 18 | +- Remove `CreateCommandHandler` step from `ProvisionStep` enum (and similar in other controllers) |
| 19 | +- Move handler creation inside the method that uses it (e.g., `provision_infrastructure`) |
| 20 | +- Simplify execute methods to focus on user-visible operations |
| 21 | +- Maintain consistent pattern across all controllers |
| 22 | + |
| 23 | +## 📊 Progress Tracking |
| 24 | + |
| 25 | +**Total Active Proposals**: 1 |
| 26 | +**Total Postponed**: 0 |
| 27 | +**Total Discarded**: 0 |
| 28 | +**Completed**: 0 |
| 29 | +**In Progress**: 0 |
| 30 | +**Not Started**: 1 |
| 31 | + |
| 32 | +### Phase Summary |
| 33 | + |
| 34 | +- **Phase 0 - Simplify All Controllers (Medium Impact, Low Effort)**: ⏳ 0/1 completed (0%) |
| 35 | + |
| 36 | +### Discarded Proposals |
| 37 | + |
| 38 | +None |
| 39 | + |
| 40 | +### Postponed Proposals |
| 41 | + |
| 42 | +None |
| 43 | + |
| 44 | +## 🎯 Key Problems Identified |
| 45 | + |
| 46 | +### 1. Unnecessary Progress Visibility |
| 47 | + |
| 48 | +Controllers currently report handler creation as a separate step, but: |
| 49 | + |
| 50 | +- Handler creation is instantaneous (no I/O, just object construction) |
| 51 | +- Users don't need visibility into this implementation detail |
| 52 | +- It clutters the progress output with non-meaningful steps |
| 53 | +- Creates coupling between execute method and internal implementation |
| 54 | + |
| 55 | +### 2. Inconsistent Encapsulation |
| 56 | + |
| 57 | +The handler is created in the `execute` method but only used within a single sub-method: |
| 58 | + |
| 59 | +```rust |
| 60 | +pub async fn execute(&mut self, environment_name: &str) -> Result<...> { |
| 61 | + let env_name = self.validate_environment_name(environment_name)?; |
| 62 | + |
| 63 | + let handler = self.create_command_handler()?; // Created here |
| 64 | + |
| 65 | + let provisioned = self.provision_infrastructure(&handler, &env_name).await?; // Only used here |
| 66 | + |
| 67 | + // ... |
| 68 | +} |
| 69 | +``` |
| 70 | + |
| 71 | +Better encapsulation would move handler creation inside `provision_infrastructure`. |
| 72 | + |
| 73 | +## 🚀 Refactoring Phases |
| 74 | + |
| 75 | +--- |
| 76 | + |
| 77 | +## Phase 0: Simplify All Controllers (Highest Priority) |
| 78 | + |
| 79 | +Remove unnecessary progress steps and improve encapsulation by moving handler creation closer to where it's used. |
| 80 | + |
| 81 | +### Proposal #1: Consolidate Handler Creation Across All Controllers |
| 82 | + |
| 83 | +**Status**: ⏳ Not Started |
| 84 | +**Impact**: 🟢🟢 Medium |
| 85 | +**Effort**: 🔵 Low |
| 86 | +**Priority**: P0 |
| 87 | +**Depends On**: None |
| 88 | + |
| 89 | +#### Problem |
| 90 | + |
| 91 | +All controllers follow the same pattern of creating application command handlers as a separate step with progress reporting: |
| 92 | + |
| 93 | +**Before (current pattern in provision controller):** |
| 94 | + |
| 95 | +```rust |
| 96 | +/// Steps in the provision workflow |
| 97 | +#[derive(Debug, Clone, Copy, PartialEq, Eq)] |
| 98 | +enum ProvisionStep { |
| 99 | + ValidateEnvironment, |
| 100 | + CreateCommandHandler, // ← Unnecessary step |
| 101 | + ProvisionInfrastructure, |
| 102 | +} |
| 103 | + |
| 104 | +pub async fn execute(&mut self, environment_name: &str) -> Result<...> { |
| 105 | + let env_name = self.validate_environment_name(environment_name)?; |
| 106 | + |
| 107 | + let handler = self.create_command_handler()?; // ← Separate method |
| 108 | + |
| 109 | + let provisioned = self.provision_infrastructure(&handler, &env_name).await?; |
| 110 | + |
| 111 | + self.complete_workflow(environment_name)?; |
| 112 | + self.display_connection_details(&provisioned)?; |
| 113 | + |
| 114 | + Ok(provisioned) |
| 115 | +} |
| 116 | + |
| 117 | +fn create_command_handler(&mut self) -> Result<...> { |
| 118 | + self.progress.start_step(ProvisionStep::CreateCommandHandler.description())?; |
| 119 | + let handler = ProvisionCommandHandler::new(self.clock.clone(), self.repository.clone()); |
| 120 | + self.progress.complete_step(None)?; |
| 121 | + Ok(handler) |
| 122 | +} |
| 123 | + |
| 124 | +async fn provision_infrastructure( |
| 125 | + &mut self, |
| 126 | + handler: &ProvisionCommandHandler, // ← Handler passed as parameter |
| 127 | + env_name: &EnvironmentName, |
| 128 | +) -> Result<...> { |
| 129 | + self.progress.start_step(ProvisionStep::ProvisionInfrastructure.description())?; |
| 130 | + let provisioned = handler.execute(env_name).await?; |
| 131 | + self.progress.complete_step(Some("Infrastructure provisioned"))?; |
| 132 | + Ok(provisioned) |
| 133 | +} |
| 134 | +``` |
| 135 | + |
| 136 | +**Issues:** |
| 137 | + |
| 138 | +- Handler creation is not a meaningful user-visible step |
| 139 | +- Handler is created in one place but only used in one other place |
| 140 | +- Progress output is cluttered with internal details |
| 141 | +- Similar pattern repeated across all controllers |
| 142 | + |
| 143 | +#### Proposed Solution |
| 144 | + |
| 145 | +**After (simplified pattern):** |
| 146 | + |
| 147 | +```rust |
| 148 | +/// Steps in the provision workflow |
| 149 | +#[derive(Debug, Clone, Copy, PartialEq, Eq)] |
| 150 | +enum ProvisionStep { |
| 151 | + ValidateEnvironment, |
| 152 | + ProvisionInfrastructure, // ← Only user-visible steps |
| 153 | +} |
| 154 | + |
| 155 | +pub async fn execute(&mut self, environment_name: &str) -> Result<...> { |
| 156 | + let env_name = self.validate_environment_name(environment_name)?; |
| 157 | + |
| 158 | + // Handler creation moved inside provision_infrastructure |
| 159 | + let provisioned = self.provision_infrastructure(&env_name).await?; |
| 160 | + |
| 161 | + self.complete_workflow(environment_name)?; |
| 162 | + self.display_connection_details(&provisioned)?; |
| 163 | + |
| 164 | + Ok(provisioned) |
| 165 | +} |
| 166 | + |
| 167 | +// create_command_handler method removed |
| 168 | + |
| 169 | +async fn provision_infrastructure( |
| 170 | + &mut self, |
| 171 | + env_name: &EnvironmentName, // ← Simpler signature |
| 172 | +) -> Result<...> { |
| 173 | + self.progress.start_step(ProvisionStep::ProvisionInfrastructure.description())?; |
| 174 | + |
| 175 | + // Handler creation encapsulated here |
| 176 | + let handler = ProvisionCommandHandler::new(self.clock.clone(), self.repository.clone()); |
| 177 | + |
| 178 | + let provisioned = handler.execute(env_name).await?; |
| 179 | + self.progress.complete_step(Some("Infrastructure provisioned"))?; |
| 180 | + Ok(provisioned) |
| 181 | +} |
| 182 | +``` |
| 183 | + |
| 184 | +#### Rationale |
| 185 | + |
| 186 | +1. **Better Encapsulation**: Handler creation is moved closer to where it's used |
| 187 | +2. **Cleaner Progress**: Only meaningful steps visible to users |
| 188 | +3. **Simpler Code**: Fewer methods, simpler signatures, clearer intent |
| 189 | +4. **Consistent Pattern**: Apply same simplification across all controllers |
| 190 | + |
| 191 | +#### Benefits |
| 192 | + |
| 193 | +- ✅ Reduces clutter in progress output |
| 194 | +- ✅ Improves code organization and encapsulation |
| 195 | +- ✅ Simplifies controller execute methods |
| 196 | +- ✅ Makes the code more maintainable |
| 197 | +- ✅ Reduces line count across all controllers |
| 198 | + |
| 199 | +#### Implementation Checklist |
| 200 | + |
| 201 | +Apply to all controllers in this order (to catch any issues early): |
| 202 | + |
| 203 | +- [ ] Provision controller (`src/presentation/controllers/provision/handler.rs`) |
| 204 | +- [ ] Configure controller (`src/presentation/controllers/configure/handler.rs`) |
| 205 | +- [ ] Register controller (`src/presentation/controllers/register/handler.rs`) |
| 206 | +- [ ] Release controller (`src/presentation/controllers/release/handler.rs`) |
| 207 | +- [ ] Run controller (`src/presentation/controllers/run/handler.rs`) |
| 208 | +- [ ] Destroy controller (if it follows the same pattern) |
| 209 | +- [ ] Test controller (if it follows the same pattern) |
| 210 | + |
| 211 | +For each controller: |
| 212 | + |
| 213 | +1. Remove `CreateCommandHandler` variant from the step enum |
| 214 | +2. Update `count()` method to reflect new step count |
| 215 | +3. Remove `create_command_handler` method |
| 216 | +4. Move handler creation inside the method that uses it |
| 217 | +5. Update progress reporter step count in constructor |
| 218 | +6. Run tests to verify behavior unchanged |
| 219 | +7. Verify progress output still clear and informative |
| 220 | + |
| 221 | +- [ ] Verify all tests pass after each controller update |
| 222 | +- [ ] Run linter and fix any issues |
| 223 | +- [ ] Update any affected documentation |
| 224 | + |
| 225 | +#### Testing Strategy |
| 226 | + |
| 227 | +For each controller: |
| 228 | + |
| 229 | +1. Run existing unit tests to verify behavior unchanged |
| 230 | +2. Manually test progress output to ensure it's clear |
| 231 | +3. Verify step numbering is correct (e.g., "Step 1/2" instead of "Step 1/3") |
| 232 | + |
| 233 | +No new tests needed - existing tests validate the behavior remains correct. |
| 234 | + |
| 235 | +#### Expected Results |
| 236 | + |
| 237 | +Per controller: |
| 238 | + |
| 239 | +- **Lines Removed**: ~15-20 (entire method + enum variant) |
| 240 | +- **Lines Added**: ~1 (handler creation in existing method) |
| 241 | +- **Net Change**: -14 to -19 lines per controller |
| 242 | +- **Total Net Change**: ~-70 to -95 lines across 5 controllers |
| 243 | + |
| 244 | +--- |
| 245 | + |
| 246 | +## 📈 Timeline |
| 247 | + |
| 248 | +- **Start Date**: TBD |
| 249 | +- **Target Completion**: 1-2 hours (simple refactoring) |
| 250 | +- **Approach**: Incremental - one controller at a time with test verification |
| 251 | + |
| 252 | +## 🎓 Principles Alignment |
| 253 | + |
| 254 | +This refactoring aligns with: |
| 255 | + |
| 256 | +- **Observability**: Users see only meaningful progress steps |
| 257 | +- **Maintainability**: Simpler code with better encapsulation |
| 258 | +- **Clean Code**: Each method has clear, focused responsibility |
| 259 | + |
| 260 | +## 📚 Related Documentation |
| 261 | + |
| 262 | +- [Development Principles](../../development-principles.md) - Observability and maintainability |
| 263 | +- [DDD Layer Placement](../../contributing/ddd-layer-placement.md) - Controller responsibilities |
| 264 | + |
| 265 | +## 💡 Notes |
| 266 | + |
| 267 | +- This is a pure refactoring - no behavior changes |
| 268 | +- Pattern discovered during implementation of issue #242 (connection details display) |
| 269 | +- Consider documenting this pattern in controller guidelines after implementation |
| 270 | + |
| 271 | +--- |
| 272 | + |
| 273 | +**Created**: December 17, 2025 |
| 274 | +**Status**: 📋 Planning |
| 275 | +**Related Issues**: None yet - discovered during #242 implementation |
0 commit comments