public class Query:QueryBase,IQuery
{
public Query(IConnectionFactory connectionFactory): base(connectionFactory, DatabaseName.Search){}
public async Task Search(ISearchCriteria criteria)
{
SearchResultDto SearchResult = null;
await Fetch(conn => conn.QueryMultipleAsync(@"dbo.SearchGet",
new
{
criteria.Keywords,
criteria.LocationId,
criteria.GridEast,
criteria.GridNorth,
criteria.GridRadius,
criteria.RateFrom,
criteria.RateTo,
criteria.IsContract,
criteria.HoursUpdatedOffset,
SortById = criteria.OrderBy,
RowTake = criteria.DbTakeAmount,
RowSkip = criteria.Page <= 1 ? 0 : (criteria.Page - 1)*criteria.PageSize
},
commandType: CommandType.StoredProcedure)).ContinueWith(t =>
{
var result = t.Result;
if (result != null)
{
SearchResult = new SearchResultDto();
var last30Days = DateTime.Today.AddDays(-30);
SearchResult.s = t.Result.Read().Select(x =>
{
SearchResult.MaxRowsCount = x.MaxRows;
return new Dto()
{
CandidateId = x.Candidate_Id,
Name = x.Forename + " " + x.Surname,
Surname = x.Surname,
Forename = x.Forename,
Location = x.Location_Name,
DistanceFromSearchedLocation = x.Distance > 0 ? Convert.ToDecimal(x.Distance) : null,
JobTitle = x.Job_Title,
Employer = x.Previous_Employer,
LastSignIn = x.Last_Login,
PayRate = x.Minimum__Rate > 0 ? Convert.ToDecimal(x.Minimum__Rate) : null,
CalculatedSalary = x.CalcSal > 0 ? Convert.ToDecimal(x.CalcSal) : null,
MinimumSalary = x.Minimum_Salary > 0 ? Convert.ToDecimal(x.Minimum_Salary) : null,
AdditionalInfo = x.AdditionalInfo,
IsAvailable = x.ShowLabel,
AvailableMornings = (x.DoesMornings ?? false) && (x.AvailabilityUpdatedOn > last30Days),
AvailableAfternoons = (x.DoesAfternoons ?? false) && (x.AvailabilityUpdatedOn > last30Days),
AvailableEvenings = (x.DoesEvenings ?? false) && (x.AvailabilityUpdatedOn > last30Days),
AvailableWeekends = (x.DoesWeekends ?? false) && (x.AvailabilityUpdatedOn > last30Days),
AvailableShiftWork = (x.DoesShiftWork ?? false) && (x.AvailabilityUpdatedOn > last30Days),
AvailableNights = (x.DoesNights ?? false) && (x.AvailabilityUpdatedOn > last30Days),
AvailabilityUpdatedOn = x.AvailabilityUpdatedOn
};
}).ToList();
}
});
return SearchResult;
}
}
there are a few concerning issues with the above code.SRP states that "Classes should have only one reason to change."
first It breaks the single responsibility principle. lets determine the responsibilities in the class.
the class is responsible for querying the database and getting a result set of data. the class is also responsible for mapping that result set to the return type. in a small class like the above this might not seem like a big issue, but as a project grows you can be sure that so does the above class and it will be harder to maintain this code.
it breaks the open closed principle.
OCP states that "Classes should be open for extension but closed for modification."
if we need to map a new field from sql to the poco class then we will have to open this class and modify it. even though this change is the side effect of the mapping responsibility in this class.
The best way to avoid introducing new bugs into a unit of code that already works is to just avoid making changes to that unit. but we know that is not possible as new features are requested and we always have to make changes to pieces of code in order to deliver new features to our customers or fix new bugs that we introduce along the way. one of the benefits of following solid principles is that the code that is produced is composed of smaller modules which are easier to understand as they are smaller and have explicit boundaries due to following SRP, we can work faster because we wont have to read through unnecessary code which is not related to the unit that we are working on and another side effect of having smaller units of code is that there is less risk of introducing new bugs. I broke this class into two distinct classes with explicit responsibilites. first I created the following interface for my mappers:
public interface IMapper<out T>
{
T Map(Dapper.SqlMapper.GridReader reader);
}
I want to follow a convention based naming scheme for my mappers. any mapper that maps a certain query result to a DTO should be named as [DTO name + Mapper], ie. the mapper for SearchResultDto will be called SearchResultDtoMapper.
if I follow this convention I will be able to use a dependency injection framework and tell it to use this convention to wire all my mappers for me. this is called convention over configuration.
I created the following mapper for the SearchResultDto:
public class SearchResultDtoMapper:IMapper<searchResultDto>
{
public SearchResultDto Map(SqlMapper.GridReader reader)
{
var SearchResult = new SearchResultDto();
var last30Days = DateTime.Today.AddDays(-30);
SearchResult.s = reader.Read<dynamicT>().Select(x =>
{
SearchResult.MaxRowsCount = x.MaxRows;
return new Dto()
{
CandidateId = x.Candidate_Id,
Name = x.Forename + " " + x.Surname,
Surname = x.Surname,
Forename = x.Forename,
Location = x.Location_Name,
DistanceFromSearchedLocation = x.Distance > 0 ? Convert.ToDecimal(x.Distance) : null,
JobTitle = x.Job_Title,
Employer = x.Previous_Employer,
LastSignIn = x.Last_Login,
PayRate = x.Minimum__Rate > 0 ? Convert.ToDecimal(x.Minimum__Rate) : null,
CalculatedSalary = x.CalcSal > 0 ? Convert.ToDecimal(x.CalcSal) : null,
MinimumSalary = x.Minimum_Salary > 0 ? Convert.ToDecimal(x.Minimum_Salary) : null,
AdditionalInfo = x.AdditionalInfo,
IsAvailable = x.ShowLabel,
AvailableMornings = (x.DoesMornings ?? false) && (x.AvailabilityUpdatedOn > last30Days),
AvailableAfternoons = (x.DoesAfternoons ?? false) && (x.AvailabilityUpdatedOn > last30Days),
AvailableEvenings = (x.DoesEvenings ?? false) && (x.AvailabilityUpdatedOn > last30Days),
AvailableWeekends = (x.DoesWeekends ?? false) && (x.AvailabilityUpdatedOn > last30Days),
AvailableShiftWork = (x.DoesShiftWork ?? false) && (x.AvailabilityUpdatedOn > last30Days),
AvailableNights = (x.DoesNights ?? false) && (x.AvailabilityUpdatedOn > last30Days),
AvailabilityUpdatedOn = x.AvailabilityUpdatedOn
};
}).ToList();
return SearchResult;
}
}
Now I can use my mapper to mapp the objects that I need but I will first have to wire up my DI so that it can Inject my implementation. as you can see I am telling Castle Windsor to wire all the classes that end with "Mapper" to their appropriate implementations.
container.Register(
Classes
.FromAssemblyContaining<searchResultDtoMapper>()
.Where(t => t.Name.EndsWith("Mapper"))
.WithService
.AllInterfaces());
Now I can inject my IMapper
public class Query:QueryBase,IQuery
{
private readonly IMapper<searchResultDto> mapper;
public Query(IMapper<searchResultDto> mapper)
: base(Connection.DatabaseName.Search)
{
this.mapper = mapper;
}
public Task<searchResultDto> Search(ISearchCriteria criteria)
{
return Fetch(async conn =>
{
var result = await conn.QueryMultipleAsync(@"dbo.SearchGet", new
{
criteria.Keywords,
criteria.LocationId,
criteria.GridEast,
criteria.GridNorth,
criteria.GridRadius,
criteria.RateFrom,
criteria.RateTo,
criteria.IsContract,
criteria.HoursUpdatedOffset,
SortById = criteria.OrderBy,
RowTake = criteria.DbTakeAmount,
RowSkip = criteria.Page <= 1 ? 0 : (criteria.Page - 1)*criteria.PageSize
},
commandType: CommandType.StoredProcedure);
return mapper.Map(result);
});
}
}
This is great, now that we have dependency injection set-up, we can start injecting as many mappers as we would need, but that would quickly lead to another problem called "bloated constructor", where we end up injecting too many dependencies into a constructor. this is an indicator of bad object oriented design and could indicate that we have a class that has too many dependencies.
public class TempQuery:QueryBase,ITempQuery
{
private readonly IMapper<SearchResultDto> searchResultDtoMapper;
private readonly IMapper<PreviewDto> previewDtoMapper;
public TempQuery(IMapper<SearchResultDto> searchResultDtoMapper, IMapper<PreviewDto> previewDtoMapper)
: base(Connection.DatabaseName.CandidateSearch)
{
this.searchResultDtoMapper = searchResultDtoMapper;
this.previewDtoMapper = previewDtoMapper;
}
...........
}
one solution for dealing with bloated constructors is to separate the responsibilities and put them behind a facade and instead inject those facades into the constructor, but in this scenario since we are injecting multiple instances of the same types i.e. implementations of IMapper
public interface IMapperFactory
{
IMapper<T> Create<T>();
}
And the implementation of our mapper factory interface:
public class MapperFactory:IMapperFactory
{
private readonly IWindsorContainer container;
public MapperFactory(IWindsorContainer container)
{
this.container = container;
}
public IMapper<T> Create<T>()
{
return this.container.Resolve<IMapper<T>>();
}
}
Here we face another anti-pattern. since this class is referencing the "Inversion of control Container" directly, it would cause the assembly where it lives to have a hard dependency on the IOC, and would definitely make this implementation a "service locator".
service locator is considered to be an anti pattern because it hides a class' dependencies, causing run-time errors instead of compile-time errors, as well as making the code more difficult to maintain because it becomes unclear when you would be introducing a breaking change.
We can fix this problem and avoid the service locator anti pattern by moving the factory implementation into the IOC composition root:
public class QueriesInstaller:IWindsorInstaller
{
public void Install(Castle.Windsor.IWindsorContainer container, Castle.MicroKernel.SubSystems.Configuration.IConfigurationStore store)
{
container.Register(
Classes
.FromAssemblyContaining<SearhcQuery>()
.Where(t => t.Name.EndsWith("Query"))
.WithService
.AllInterfaces().LifestylePerWebRequest());
container.Register(
Classes
.FromAssemblyContaining<SearchResultDtoMapper>()
.Where(t => t.Name.EndsWith("Mapper"))
.WithService
.AllInterfaces());
container.AddFacility<TypedFactoryFacility>();
container.Register(Component.For<IMapperFactory>().AsFactory());
}
}
We can then inject our mapper factory instance into the classes that are dependent on it and create any of the mappers that we need at run time:
public class Query:QueryBase,IQuery
{
private readonly IMapperFactory mapperFactory;
public Query(IMapperFactory mapperFactory) : base(Connection.DatabaseName.Search)
{
this.mapperFactory = mapperFactory;
}
public Task<SearchResultDto> Search(ISearchCriteria criteria, int? ecruiterId, int? ouId)
{
return Fetch(async conn =>
{
var result = await conn.QueryMultipleAsync(@"dbo.SearchGet", new
{
criteria.Keywords,
criteria.LocationId,
criteria.GridEast,
criteria.GridNorth,
criteria.GridRadius,
criteria.RateFrom,
criteria.RateTo,
criteria.IsContract,
criteria.HoursUpdatedOffset,
SortById = criteria.OrderBy,
EcruiterId = ecruiterId,
OuId = ouId,
RowTake = criteria.DbTakeAmount,
RowSkip = criteria.Page <= 1 ? 0 : (criteria.Page - 1)*criteria.PageSize
},
commandType: CommandType.StoredProcedure);
return mapperFactory.Create<SearchResultDto>().Map(result);
});
}
}
No comments :
Post a Comment