Refactoring duplicated installation code

Question

I have the following classes:

public record LocalCcmProgram : LocalCcmSoftwareBase, IInstallableProgram<LocalCcmProgram>, ICacheInfoProvider
{
 // Unimportant properties and methods
 public LocalCcmProgram Install()
 {
 _logger.Debug("Request to install {type} {name}", GetType(), Name);
 ValidateInstall();
 EventController.Reset();
 StartInstall();
 this.WaitProgramStop(_logger);
 EventController.Stop();
 return ReloadInstance();
 }
 protected virtual void ValidateInstall()
 {
 if (!CanInstall())
 throw new ActionNotAllowedException();
 if (ClientUtilities.IsRebootPending())
 throw new PendingRestartException();
 }
 protected virtual void StartInstall()
 {
 if (EventController.IsStarted)
 throw new InvalidOperationException(
 $"Attempted to use an already started CcmEventController to install 
 {GetType()} {Name}");
 EventController.Start();
 var result = ProgramsManagerController.ExecuteProgram(ProgramId, PackageId);
 if (result.ReturnValue != 0)
 throw new StartActionException($"Failed to start {GetType()} {Name}");
 this.WaitProgramStart(_logger);
 }
}

public record LocalCcmSoftwareDistribution : LocalCcmPolicy, IInstallableProgram<LocalCcmSoftwareDistribution>, ICacheInfoProvider
{
 // Unimportant properties and methods
 public LocalCcmSoftwareDistribution Install()
 {
 _logger.Debug("Request to install {type} {name}", GetType(), PkgName);
 ValidateInstall();
 EventController.Reset();
 StartInstall();
 this.WaitProgramStop(_logger);
 EventController.Stop();
 return ReloadInstance();
 }
 protected virtual void ValidateInstall()
 {
 if (ClientUtilities.IsRebootPending())
 throw new PendingRestartException();
 }
 protected virtual void StartInstall()
 {
 if (EventController.IsStarted)
 throw new InvalidOperationException(
 $"Attempted to use an already started CcmEventController to install {GetType()} {PkgName}");
 EventController.Start();
 var originalAdvRepeatRunBehavior = AdvRepeatRunBehavior;
 var originalAdvMandatoryAssignments = AdvMandatoryAssignments;
 if (originalAdvMandatoryAssignments == null || originalAdvRepeatRunBehavior == null)
 throw new ArgumentNullException(nameof(originalAdvMandatoryAssignments));
 _logger.Debug("Setting Assignment");
 _ = SoftwareDistributionController.SetAssignment(SystemProperties.Path!,
 Core.AdvRepeatRunBehavior.RerunAlways, true);
 
 _logger.Debug("Retrieving Schedule");
 var schedule =
 SchedulerHistoryLoader.GetInstances($"ScheduleID like '{AdvAdvertisementId}-{PkgPackageId}-%'")
 .SingleOrDefault() ?? throw new ScheduleNotFoundException();
 _logger.Debug("Triggering schedule: {id}", schedule.ScheduleId);
 var result = SmsClient.TriggerSchedule(schedule);
 if (result.ReturnValue != null && result.ReturnValue != 0)
 throw new StartActionException($"Failed to start {GetType()} {PkgName}");
 
 this.WaitProgramStart(_logger);
 _logger.Debug("Successfully started {type} {program}", GetType(), PkgName);
 _logger.Debug("Reverting Assignment");
 _ = SoftwareDistributionController.SetAssignment(SystemProperties.Path!,
 originalAdvRepeatRunBehavior.Value, originalAdvMandatoryAssignments.Value);
 }
}

As you can tell, some of the code is duplicated or very similar. Both classes represent the same object (program in this case) and they both start an installation of said program, but the backend API that is used to install them is different based on the class used.

I've already put quite a few things (WaitProgramStart/WaitProgramStop) into extension methods, but still feel like there's more that can be done, just don't quite know how to do it.

I can't really use class inheritance because both classes already inherit a (different) class on their own, I can't really use interfaces for all the internal stuff because then it would have to be public, and I don't really feel like using extension methods that accept an IInstallableProgram<T> and internally check what the concrete type is to determine what to do is the best way either.

The best idea I've come up with so far is passing an Action for the validation and another for the StartInstall but that would still leave me with the same code on these classes, just not directly invoked from the class itself.

Any other ideas on how to improve this?

• CharlesNRice: Do you use a DI container? There is where composition over inheritance comes in. Instead of an Action I would make another class that has that logic.
• cogumel0: @CharlesNRice I do have a DI container, yes. Can you give an example of how you'd put that logic in another class (which is what I'm trying to do ofc)
• Peter Csala: I assume that the Install method is defined inside the IInstallableProgram. Can't you define a default implementation there?

Answer 1

I don't think there is a lot of code duplication remaining that is worth eliminating. However a few additional comments:

1. This exception is potentially misleading:

   if (originalAdvMandatoryAssignments == null || originalAdvRepeatRunBehavior == null)
    throw new ArgumentNullException(nameof(originalAdvMandatoryAssignments));
   

   In case originalAdvRepeatRunBehavior is null then the incorrect argument name would be supplied. It should be broken up into two checks.

2. If there is an exception raised after setting the assignment then it will never be reverted - is this intended?

3. Biggest of all: Why did you use record instead of class? record was designed to represent an immutable data object (https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/record). What you have here isn't a record. All code should reflect a certain intention and design thinking so that the next developer that comes along and who has to maintain it, has an easier time to understand what it's for. Using language features opens up the door to introduction of subtle bugs and misunderstandings.

• cogumel0: Thx for the first two catches, you're right about those and they should be fixed. About number 3: the data on the class is actually immutable. This is why ReloadInstance() returns a new instance altogether. These instances represent WMI objects that were mapped to C# objects - hence records being used. If you disagree with the usage would love to hear more.

Answer 2

After reading the code several times over, what these classes have in common is the Install() method. There was more duplication but you eliminated that using extension methods. You didn't mention having more than two of these.

Based on all of that I'd say your work is done. The remaining duplication is minor and coincidental. Trying to remove that duplication might make the code harder to read and maintain. The two are very different. What if they diverge even more? Having them tied together by putting the remaining fragments of code into a shared base class might just make future changes harder.