I've been a .NET Developer for almost 4 years, but I'm stronly learning and trying using design patterns and following SOLID principles (I know - a little late, but better late than never).
I have complex business logic for saving some metadata in database (mainly jsonb object to postgresql database). My code is working right now, but I'm not so happy about my code. I think it is sometimes not clean as it should, and not always "adaptable". And also I think that I break Single Principle Responsibility even if I really try to stick to it. I mean that class should not have too much responsibility, but the last class which combines everything should have multiple injections and just delegate some steps by using interfaces. I think that is correct thinking, but when I see that I inject about 6 interfaces, I have a feeling that this code is not so good.
So I want to get feedback, what I am doing wrong in this code, and what can be written better. Also I think I am still not very well adapting desing patterns with Microsoft Dependency Injection. Hard for me is the factory pattern with generic types and how to properly inject it.
So here is a code for saving "Deliveryset". Note: I use "some" word in some names to hide real business names. This code below is saving data to postgresql database using ef core. Before it is saved it must be validated and also checked in a database that has an existing related entity ("Content") for it. After that I search for the current Deliveryset with the highest version, in order to compare it with the incoming DeliverySet. I compare big jsonb objects using "CompareNETObjects" nuget library. The comparison is needed to know if subtitles, videos, and other metadata changed. If any changes occurred it will create a new row in the ContentDeliverySet table with Version = currentVersion + 1. But it also checks related entities like SubtitleFiles or VideoFiles if they already exist. If it exists and doesn't have changes it should copy it but with a new entity Id. If for example video changed it will not copy and create ContentDeliveryset entity with empty VideoFilesEntity.
The code:
Interface:
public interface IDeliverysetSaverService
{
Task<bool> SaveDeliveryset(Deliveryset? deliverySet, bool isDeletedFlag = false, CancellationToken cancellationToken = default);
}
Implementation:
public class DeliverysetSaverService : IDeliverysetSaverService
{
private readonly IApplicationDbContext _context;
private readonly IDeliverysetVersionInfoByIdRetrieve _deliverysetVersionInfoByIdRetrieve;
private readonly IAssetStatusMapRetriever _assetStatusMapRetriever;
private readonly ILogger<DeliverysetSaverService> _logger;
private readonly IDeliverysetService _deliverySetService;
private readonly IValidator<Deliveryset> _deliverySetValidator;
private readonly IDeliverysetFileChangesCoordinator _deliverySetFileChangesCoordinator;
public DeliverysetSaverService(IApplicationDbContext context, IDeliverysetVersionInfoByIdRetrieve deliverysetVersionInfoByIdRetrieve, IAssetStatusMapRetriever assetStatusMapRetriever, ILogger<DeliverysetSaverService> logger, IDeliverysetService deliverySetService, IValidator<Deliveryset> deliverySetValidator, IDeliverysetFileChangesCoordinator deliverySetFileChangesCoordinator)
{
_context = context;
_deliverysetVersionInfoByIdRetrieve = deliverysetVersionInfoByIdRetrieve;
_assetStatusMapRetriever = assetStatusMapRetriever;
_logger = logger;
_deliverySetService = deliverySetService;
_deliverySetValidator = deliverySetValidator;
_deliverySetFileChangesCoordinator = deliverySetFileChangesCoordinator;
}
public async Task<bool> SaveDeliveryset(Deliveryset? deliverySet, bool isDeletedFlag = false,
CancellationToken cancellationToken = default)
{
if (deliverySet == null) return false;
var validationResult = await _deliverySetValidator.ValidateAsync(deliverySet, cancellationToken);
if (!validationResult.IsValid)
{
_logger.LogWarning("Deliveryset validation failed, reason: {reason}", validationResult.ToString());
return false;
}
var id = new SomeIdValueObject(deliverySet.Tags.MhtId);
var contentId = (await _context.Contents.SingleOrDefaultAsync(content =>
content.ProductionNr == id.ProductionNr && content.ProgramNr == id.ProgramNr
&& content.Version == id.Version && content.Format == id.Format, cancellationToken: cancellationToken))?.Id;
var (someTrafficId, _) = id;
if (!contentId.HasValue)
{
_logger.LogWarning(
"Content for id:{id} was not found when saving delivery set", someTrafficId);
return false;
}
var deliverySetVersion =
await _deliverysetVersionInfoByIdRetrieve.GetVersionInfo(contentId.Value, cancellationToken);
var deliverySetChangesInfo = await _deliverySetFileChangesCoordinator.GetDeliverysetChangesInfo(deliverySetVersion, deliverySet, cancellationToken);
if (!deliverySetChangesInfo.HasChanges)
{
return false;
}
var contentDeliveryset = new ContentDeliveryset
{
ContentDeliverysetMetadata = new ContentDeliverysetMetadata()
{
AssetStatusId = _assetStatusMapRetriever.GetAssetStatusMap()["some_code"].Id
},
Deliveryset = deliverySet,
ContentId = contentId,
Version = deliverySetVersion.NextVersion,
DeliverysetId = deliverySet.Id,
SubtitleFiles = deliverySetChangesInfo.SubtitleFiles.ToList(),
VideoFiles = deliverySetChangesInfo.VideoFiles.ToList(),
IsDeleted = isDeletedFlag
};
await _deliverySetService.Save(contentDeliveryset, cancellationToken);
return true;
}
}
IDeliverysetVersionInfoByIdRetrieve
interface is used for searching the current delivery set with it's version.
IAssetStatusMapRetriever
interface is used to get whole AssetStatus entity and transform to dictionary: <Code, Entity>.
IDeliverysetService
interface contains only context and saves passed object to database.
'IDeliverysetFileChangesCoordinator' interface is the most complicated one.
Interface:
public interface IDeliverysetFileChangesCoordinator
{
Task<DeliverysetChangesInfo> GetDeliverysetChangesInfo(DeliverysetVersionInfo deliverySetVersionInfo,
Deliveryset newDeliveryset,
CancellationToken cancellationToken = default); GetFilesToCopy();
}
Implementation:
public class DeliverysetFileChangesCoordinator : IDeliverysetFileChangesCoordinator
{
private readonly IApplicationDbContext _context;
private readonly IDeliverysetFileCopyHandlerFactory _deliverySetFileCopyHandlerFactory;
private readonly IDeliverysetFileDetectChangesHandlerFactory _detectChangesHandlerFactory;
private IEnumerable<VideoFiles> _videoFiles = Array.Empty<VideoFiles>();
private IEnumerable<SubtitleFiles> _subtitleFiles = Array.Empty<SubtitleFiles>();
private bool _anyChanges;
public DeliverysetFileChangesCoordinator(IApplicationDbContext context, IDeliverysetFileCopyHandlerFactory deliverySetFileCopyHandlerFactory, IDeliverysetFileDetectChangesHandlerFactory detectChangesHandlerFactory)
{
_context = context;
_deliverySetFileCopyHandlerFactory = deliverySetFileCopyHandlerFactory;
_detectChangesHandlerFactory = detectChangesHandlerFactory;
}
public async Task<DeliverysetChangesInfo> GetDeliverysetChangesInfo(DeliverysetVersionInfo deliverySetVersionInfo, Deliveryset newDeliveryset, CancellationToken cancellationToken = default)
{
if (deliverySetVersionInfo.CurrentVersion == null)
{
return new DeliverysetChangesInfo(true, Array.Empty<VideoFiles>(), Array.Empty<SubtitleFiles>());
}
var currentNewestDeliveryset =
await _context.ContentDeliveryset.AsNoTracking().Where(d => d.Id == deliverySetVersionInfo.CurrentVersion.Id)
.Select(t => new
{
t.Deliveryset,
t.VideoFiles,
t.SubtitleFiles,
t.Version
}).SingleAsync(cancellationToken: cancellationToken);
foreach (var deliverySetChange in Enum.GetValues<DeliverysetChange>())
{
var changesHandler = _detectChangesHandlerFactory.GetDeliverysetFileCopyHandler(deliverySetChange);
var changesResult = changesHandler.GetDeliverysetChangesResult(currentNewestDeliveryset.Deliveryset, newDeliveryset);
if (!changesResult.IsAllAreEqual)
{
_anyChanges = true;
}
if (changesResult.IsAllAreEqual && deliverySetChange == DeliverysetChange.Video)
{
var copyHandler = _deliverySetFileCopyHandlerFactory.GetDeliverysetFileCopyHandler<VideoFiles>(DeliverysetFileCopyHandler.Video);
_videoFiles = copyHandler.GetFilesToCopy(currentNewestDeliveryset.VideoFiles);
}
if (deliverySetChange == DeliverysetChange.Subtitle)
_subtitleFiles = GetSubtitlesToCopy(changesResult, currentNewestDeliveryset.SubtitleFiles);
}
return new DeliverysetChangesInfo(_anyChanges, _videoFiles, _subtitleFiles);
}
private IEnumerable<SubtitleFiles> GetSubtitlesToCopy(DeliverysetChangesResult changesResult, List<SubtitleFiles> currentDeliverysetSubFiles)
{
var copy = _deliverySetFileCopyHandlerFactory.GetDeliverysetFileCopyHandler<SubtitleFiles>(DeliverysetFileCopyHandler.Sub);
bool FilterNotChangedSubtitleTypes(SubtitleFiles t) => changesResult.NotChangedByKeyList.Contains(t.SubtitleType);
var subtitleFilesList = changesResult.IsAllAreEqual
? currentDeliverysetSubFiles
: currentDeliverysetSubFiles.Where(FilterNotChangedSubtitleTypes).ToList();
var subtitleFilesEnumerable = copy.GetFilesToCopy(subtitleFilesList);
return subtitleFilesEnumerable;
}
}
Implementation of DeliverysetFileDetectChangesHandlerFactory
public class DeliverysetFileDetectChangesHandlerFactory : IDeliverysetFileDetectChangesHandlerFactory
{
private readonly IDictionary<DeliverysetChange, IDeliverysetFileDetectChangesHandler> _map;
public DeliverysetFileDetectChangesHandlerFactory(IDictionary<DeliverysetChange, IDeliverysetFileDetectChangesHandler> map)
{
_map = map;
}
public IDeliverysetFileDetectChangesHandler GetDeliverysetFileCopyHandler(DeliverysetChange deliverySetChange)
{
return _map[deliverySetChange];
}
}
Interface IDeliverysetFileDetectChangesHandler:
public interface IDeliverysetFileDetectChangesHandler
{
DeliverysetChangesResult GetDeliverysetChangesResult(Deliveryset currentDeliveryset, Deliveryset newDeliveryset);
}
One of implementations:
public class DeliverysetAssetsDetectChangesHandler : IDeliverysetFileDetectChangesHandler
{
public DeliverysetChangesResult GetDeliverysetChangesResult(Deliveryset currentDeliveryset, Deliveryset newDeliveryset)
{
return DeliverysetComparer.Init(newDeliveryset, currentDeliveryset).AssetsChanges();
}
}
Implementation of DeliverysetFileCopyHandlerFactory
public class DeliverysetFileCopyHandlerFactory : IDeliverysetFileCopyHandlerFactory
{
private readonly IDictionary<DeliverysetFileCopyHandler, IDeliverysetFileCopyHandler> _map;
public DeliverysetFileCopyHandlerFactory(IDictionary<DeliverysetFileCopyHandler, IDeliverysetFileCopyHandler> map)
{
_map = map;
}
public IDeliverysetFileCopyHandler<T> GetDeliverysetFileCopyHandler<T>(DeliverysetFileCopyHandler copyMethod) where T : IFileEntity
{
var deliverySetFileCopyHandler = _map[copyMethod];
return (IDeliverysetFileCopyHandler<T>)deliverySetFileCopyHandler;
}
}
Interfaces IDeliverysetFileCopyHandler:
public interface IDeliverysetFileCopyHandler
{
}
public interface IDeliverysetFileCopyHandler<T> : IDeliverysetFileCopyHandler where T : IFileEntity
{
IEnumerable<T> GetFilesToCopy(List<T> entities);
}
One of implementations:
public class DeliverysetSubtitleFilesCopyHandler : IDeliverysetFileCopyHandler<SubtitleFiles>
{
public IEnumerable<SubtitleFiles> GetFilesToCopy(List<SubtitleFiles>? entities)
{
if (entities == null) yield break;
foreach (var entity in entities)
{
yield return new SubtitleFiles()
{
FileId = entity.FileId,
Status = entity.Status,
SubtitleType = entity.SubtitleType
};
}
}
}
And last is dependency injection:
public static class ConfigureServices
{
public static IServiceCollection AddLibraryServices(this IServiceCollection services)
{
var dummyConnString = "Server=127.0.0.1;Port=5432;Database=myDataBase;User Id=myUsername;Password=myPassword";
services.AddDbContext<ApplicationDbContext>(options =>
options.UseNpgsql(dummyConnString));
services.AddValidatorsFromAssembly(Assembly.GetExecutingAssembly());
services.AddScoped<IApplicationDbContext>(provider => provider.GetRequiredService<ApplicationDbContext>());
services.AddScoped<AuditableEntitySaveChangesInterceptor>();
services.AddDeliverysetServices();
return services;
}
public static IServiceCollection AddDeliverysetServices(this IServiceCollection services)
{
services.AddScoped<IValidator<Deliveryset>, DeliverysetValidator>();
services.AddScoped<IDeliverysetSaverService, DeliverysetSaverService>();
services.AddScoped<IAssetStatusMapRetriever, AssetStatusMapRetriever>();
services.AddScoped<IDeliverysetVersionInfoByIdRetrieve, DeliverysetVersionInfoByIdRetrieve>();
services.AddScoped<IDeliverysetService, Services.DataServices.DeliverysetService>();
services.AddScoped<IDeliverysetFileChangesCoordinator, DeliverysetFileChangesCoordinator>();
services.AddSingleton<IDeliverysetFileCopyHandlerFactory, DeliverysetFileCopyHandlerFactory>(_ =>
{
var deliverySetFileCopyHandlers =
new Dictionary<DeliverysetFileCopyHandler, IDeliverysetFileCopyHandler>
{
{ DeliverysetFileCopyHandler.Video, new DeliverysetVideoFilesCopyHandler() },
{ DeliverysetFileCopyHandler.Sub, new DeliverysetSubtitleFilesCopyHandler() }
};
return new DeliverysetFileCopyHandlerFactory(deliverySetFileCopyHandlers);
});
services.AddSingleton<IDeliverysetFileDetectChangesHandlerFactory, DeliverysetFileDetectChangesHandlerFactory>(_ =>
{
var deliverySetFileDetectChangesHandlers =
new Dictionary<DeliverysetChange, IDeliverysetFileDetectChangesHandler>
{
{ DeliverysetChange.Video, new DeliverysetVideoFilesDetectChangesHandler() },
{ DeliverysetChange.Asset, new DeliverysetAssetsDetectChangesHandler() },
{ DeliverysetChange.Subtitle, new DeliverysetSubtitleFilesDetectChangesHandler() }
};
return new DeliverysetFileDetectChangesHandlerFactory(deliverySetFileDetectChangesHandlers);
});
return services;
}
}
}
I put the whole fully compiling code in github. Don't watch repo structured it was copied from bigger solution for code review purposes.
I know that this is a lot of classes and interfaces. But if it was putted in single method in single class it won't be so big. This also my concern if I am not split to much the code.