Refactor Strategy classes to eliminate duplication using Template Method pattern #109
Conversation
Co-authored-by: JusterZhu <11714536+JusterZhu@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request successfully refactors the Strategy pattern implementation across GeneralUpdate.Core and GeneralUpdate.ClientCore by introducing a Template Method pattern that eliminates significant code duplication while maintaining all platform-specific functionality.
Changes:
- Created a Template Method pattern in AbstractStrategy with
ExecuteAsync()handling common execution flow - Added hook methods (
CreatePipelineContext,BuildPipeline,OnExecuteComplete,HandleExecuteException) for customization - Reduced duplicate code by 232 lines (41% reduction from 563 to 331 lines)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| AbstractStrategy.cs | Implements Template Method pattern with common execution flow, context creation, error handling, and utility methods |
| Core/WindowsStrategy.cs | Simplified to override only BuildPipeline, CreatePipelineContext (for driver config), and OnExecuteComplete (to start app) |
| Core/LinuxStrategy.cs | Similar to WindowsStrategy, adds Execute() method that wraps ExecuteAsync, fixed error message to reference LinuxStrategy |
| ClientCore/WindowsStrategy.cs | Overrides only BuildPipeline and CreatePipelineContext (minimal customization) - no OnExecuteComplete since StartApp is called externally |
| ClientCore/LinuxStrategy.cs | Adds blacklist context parameters in CreatePipelineContext, otherwise similar to ClientCore/WindowsStrategy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| var context = base.CreatePipelineContext(version, patchPath); | ||
|
|
||
| // Add ClientCore-specific context items (blacklists are not needed for Windows in Core) |
There was a problem hiding this comment.
The comment incorrectly states "blacklists are not needed for Windows in Core" when it should say "blacklists are not needed for Windows in ClientCore". The word "Core" at the end refers to GeneralUpdate.Core, but this class is in GeneralUpdate.ClientCore.
| // Add ClientCore-specific context items (blacklists are not needed for Windows in Core) | |
| // Add ClientCore-specific context items (blacklists are not needed for Windows in ClientCore) |
| EventManager.Instance.Dispatch(this, new ExceptionEventArgs(e, e.Message)); | ||
| } | ||
| }); | ||
| ExecuteAsync().Wait(); |
There was a problem hiding this comment.
Using .Wait() on an async method can potentially cause deadlocks in certain synchronization contexts and swallows AggregateException details. Consider using .GetAwaiter().GetResult() instead, which preserves exception details and is less prone to deadlocks.
| ExecuteAsync().Wait(); | |
| ExecuteAsync().GetAwaiter().GetResult(); |
| // Driver middleware (Windows-specific) | ||
| if (_configinfo.DriveEnabled == true) | ||
| { | ||
| var status = ReportType.None; | ||
| var patchPath = StorageManager.GetTempDirectory(Patchs); | ||
| foreach (var version in _configinfo.UpdateVersions) | ||
| { | ||
| try | ||
| { | ||
| var context = new PipelineContext(); | ||
| //Common | ||
| context.Add("ZipFilePath", | ||
| Path.Combine(_configinfo.TempPath, $"{version.Name}{_configinfo.Format}")); | ||
| //Hash middleware | ||
| context.Add("Hash", version.Hash); | ||
| //Zip middleware | ||
| context.Add("Format", _configinfo.Format); | ||
| context.Add("Name", version.Name); | ||
| context.Add("Encoding", _configinfo.Encoding); | ||
| //Patch middleware | ||
| context.Add("SourcePath", _configinfo.InstallPath); | ||
| context.Add("PatchPath", patchPath); | ||
| context.Add("PatchEnabled", _configinfo.PatchEnabled); | ||
| //Driver middleware | ||
| if (_configinfo.DriveEnabled == true) | ||
| { | ||
| context.Add("DriverOutPut", StorageManager.GetTempDirectory("DriverOutPut")); | ||
| context.Add("FieldMappings", _configinfo.FieldMappings); | ||
| } | ||
|
|
||
| var pipelineBuilder = new PipelineBuilder(context) | ||
| .UseMiddlewareIf<PatchMiddleware>(_configinfo.PatchEnabled) | ||
| .UseMiddleware<CompressMiddleware>() | ||
| .UseMiddleware<HashMiddleware>() | ||
| .UseMiddlewareIf<DriverMiddleware>(_configinfo.DriveEnabled); | ||
| await pipelineBuilder.Build(); | ||
| status = ReportType.Success; | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| status = ReportType.Failure; | ||
| GeneralTracer.Error( | ||
| "The Execute method in the GeneralUpdate.Core.WindowsStrategy class throws an exception.", | ||
| e); | ||
| EventManager.Instance.Dispatch(this, new ExceptionEventArgs(e, e.Message)); | ||
| } | ||
| finally | ||
| { | ||
| await VersionService.Report(_configinfo.ReportUrl | ||
| , version.RecordId | ||
| , status | ||
| , version.AppType | ||
| , _configinfo.Scheme | ||
| , _configinfo.Token); | ||
| } | ||
| } | ||
| context.Add("DriverOutPut", StorageManager.GetTempDirectory("DriverOutPut")); | ||
| context.Add("FieldMappings", _configinfo.FieldMappings); | ||
| } |
There was a problem hiding this comment.
The Driver middleware configuration logic is identical between WindowsStrategy and LinuxStrategy. Consider moving this logic to the base CreatePipelineContext method in AbstractStrategy to further reduce duplication. The base method could check DriveEnabled and add the driver context parameters, eliminating the need for both strategies to override this.
LinuxStrategy and WindowsStrategy in both GeneralUpdate.Core and GeneralUpdate.ClientCore contained ~80% duplicate code for pipeline execution, context setup, error handling, and version reporting.
Changes
AbstractStrategy Base Class
ExecuteAsync()with common execution flowCreatePipelineContext(version, patchPath)for context customizationBuildPipeline(context)for platform-specific middleware chainsOnExecuteComplete()hook for post-execution logicCheckPath()helper andHandleExecuteException()to base classStrategy Subclasses
Each now only defines platform-specific behavior:
Before (WindowsStrategy example):
After:
Impact
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.