2
\$\begingroup\$

I have never created a web service before. I followed most of this Pluralsight tutorial to give me an idea of how to create one using ASP.NET MVC along with .NET Core 2.0 and Entity Framework Core.

The goal of this web service is to provide users with data from a database. It doesn't really do anything other than filter data down to what was requested and then return that data.

Here is an example request body:

{
    "buildIds": [
        "BuildId.1",
        "BuildId.2"
    ],
    "cRs": [
        100,
        400
    ]
}

The buildIds property is what is used to get "CRs". The cRs property is used to filter these CRs down to a specific set. The cRs property can be omitted if the user doesn't want to filter by anything.

This question is somewhat two-fold: I would like to know if there are any other cases for which I should handle certain things coming in as requests and also what you think of my code overall.

Controller:

[Route("api/metabuildCRs")]
public class MetabuildCRsController : Controller
{
    private IQSARepository _repository;

    public MetabuildCRsController(IQSARepository repository)
    {
        _repository = repository;
    }

    [HttpPost] // POST is used here because you can't send a body with GET
    public IActionResult GetMetabuildCrs([FromBody] MetabuildCRsRequest model)
    {
        if (model == null || model.BuildIds == null) {
            return BadRequest();
        }

        var metabuildCRs = new List<MetabuildCR>();
        foreach (var productBuildId in model.BuildIds) {
            var imageBuildIds = _repository.GetImageBuildsInProductBuild(productBuildId);
            foreach (var imageBuildId in imageBuildIds) {
                var crNumbers = _repository.GetJobDetailsForSoftwareImageBuild(imageBuildId)?
                    .Select(jd => jd.ChangeRequestNumber)
                    .Distinct();

                if (model.CRs != null && model.CRs.Count() > 0) {
                    // filter down to only crs we care about
                    crNumbers = crNumbers.Where(cr => model.CRs.Contains(cr));
                }

                foreach (var crNumber in crNumbers) {
                    var imageBuild = _repository.GetSoftwareImageBuild(imageBuildId);
                    var bulletinInfo = _repository.GetBulletinInformationForCR(crNumber);
                    var exception = _repository.GetCRException(crNumber, imageBuildId);
                    var dependentCRs = _repository.GetCRsThatDependOnCR(crNumber);

                    metabuildCRs.Add(new MetabuildCR {
                        ChangeRequestNumber = crNumber,
                        // Build Info
                        SoftwareImageBuildId = imageBuildId,
                        BuildDate = imageBuild.CrmbuildDate,
                        // Exception Info
                        RequestText = exception?.RequestText,
                        RequestedBy = exception?.RequestedBy,
                        RequestedOn = exception?.RequestedOn,
                        ExpiresOn = exception?.ExpiresOn,
                        JiraIssueKey = exception?.JiraIssueKey,
                        ReasonCode = exception?.ReasonCode,
                        ResponseBy = exception?.ResponseBy,
                        ResponseText = exception?.ResponseText,
                        ResponseOn = exception?.ResponseOn,
                        ExemptionNotes = exception?.Notes,
                        //Bulletin Info
                        SecurityBulletinDcn = bulletinInfo?.SecurityBulletinDcn,
                        DocumentType = bulletinInfo?.DocumentType,
                        DocumentReleaseDate = bulletinInfo?.DocumentReleaseDate,

                        DependentCRs = dependentCRs
                    });
                }
            }
        }

        return Ok(metabuildCRs);
    }
}

Request object:

public class MetabuildCRsRequest
{
    public IEnumerable<string> BuildIds { get; set; }
    public IEnumerable<int> CRs { get; set; }
}

Repository (service layer):

public class QSARepository : IQSARepository
{
    private QSAContext _context;

    public QSARepository(QSAContext context)
    {
        _context = context;
    }

    public IEnumerable<string> GetImageBuildsInProductBuild(string buildId)
    {
        return _context.SoftwareProductBuildCompositions.Where(x => x.SoftwareProductBuildId == buildId)?.Select(y => y.SoftwareImageBuildId);
    }

    public SoftwareImageBuild GetSoftwareImageBuild(string buildId)
    {
        return _context.SoftwareImageBuilds.FirstOrDefault(sib => sib.SoftwareImageBuildId == buildId);
    }

    public IEnumerable<VerifySourceJobDetail> GetJobDetailsForSoftwareImageBuild(string buildId)
    {
        var crNumbers = (from job in _context.VerifySourceJobs
                join details in _context.VerifySourceJobDetails on job.Id equals details.VerifySourceJobId
                where job.SoftwareImageBuildId == buildId
                select details).Distinct();

        return crNumbers;
    }

    public CRException GetCRException(int crNumber, string softwareImage)
    {
        return _context.CRExceptions.FirstOrDefault(e => e.ChangeRequestNumber == crNumber && e.SoftwareImage == softwareImage);
    }

    public PrismCRDocument GetBulletinInformationForCR(int crNumber)
    {
        return _context.PrismCRDocuments.FirstOrDefault(b => b.ChangeRequestNumber == crNumber);
    }

    public IEnumerable<int> GetCRsThatDependOnCR(int crNumber)
    {
        return from r in _context.PrismCRRelationships
               where r.ChangeRequestNumber2 == crNumber && r.Relationship == "DependsOn"
               select r.ChangeRequestNumber1;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! As per the How to Ask guidelines, please tell us more about what this web service is for, and also retitle the question to summarize what the code accomplishes. \$\endgroup\$ – 200_success Apr 3 '18 at 20:00
  • 1
    \$\begingroup\$ @200_success Thank you for that, I didn't realize how vague it was. Edited to clarify. \$\endgroup\$ – Window Apr 3 '18 at 20:15
1
\$\begingroup\$

Controllers should be kept as lean as possible. Consider adding another layer of abstraction specific the controller in order to separate concerns.

public interface IMetabuildCRsService  {
    List<MetabuildCR> GetMetabuildCrs(IEnumerable<string> BuildIds, IEnumerable<int> CRs = null);
}

Its implementation will encapsulate the core functionality currently being done in the controller.

public class DefaultMetabuildCRsService : IMetabuildCRsService {
    private readonly IQSARepository repository;

    public DefaultMetabuildCRsService(IQSARepository repository) {
        this.repository = repository;
    }

    public List<MetabuildCR> GetMetabuildCrs(IEnumerable<string> BuildIds, IEnumerable<int> CRs = null){
        var metabuildCRs = new List<MetabuildCR>();
        foreach (var productBuildId in BuildIds) {
            var imageBuildIds = repository.GetImageBuildsInProductBuild(productBuildId);
            foreach (var imageBuildId in imageBuildIds) {
                var crNumbers = repository.GetJobDetailsForSoftwareImageBuild(imageBuildId)?
                    .Select(jd => jd.ChangeRequestNumber)
                    .Distinct();

                if (CRs != null && CRs.Count() > 0) {
                    // filter down to only crs we care about
                    crNumbers = crNumbers.Where(cr => CRs.Contains(cr));
                }

                var imageBuild = repository.GetSoftwareImageBuild(imageBuildId);
                foreach (var crNumber in crNumbers) {
                    var bulletinInfo = repository.GetBulletinInformationForCR(crNumber);
                    var exception = repository.GetCRException(crNumber, imageBuildId);
                    var dependentCRs = repository.GetCRsThatDependOnCR(crNumber);

                    metabuildCRs.Add(new MetabuildCR {
                        ChangeRequestNumber = crNumber,
                        // Build Info
                        SoftwareImageBuildId = imageBuildId,
                        BuildDate = imageBuild.CrmbuildDate,
                        // Exception Info
                        RequestText = exception?.RequestText,
                        RequestedBy = exception?.RequestedBy,
                        RequestedOn = exception?.RequestedOn,
                        ExpiresOn = exception?.ExpiresOn,
                        JiraIssueKey = exception?.JiraIssueKey,
                        ReasonCode = exception?.ReasonCode,
                        ResponseBy = exception?.ResponseBy,
                        ResponseText = exception?.ResponseText,
                        ResponseOn = exception?.ResponseOn,
                        ExemptionNotes = exception?.Notes,
                        //Bulletin Info
                        SecurityBulletinDcn = bulletinInfo?.SecurityBulletinDcn,
                        DocumentType = bulletinInfo?.DocumentType,
                        DocumentReleaseDate = bulletinInfo?.DocumentReleaseDate,

                        DependentCRs = dependentCRs
                    });
                }
            }
        }
        return metabuildCRs;
    }
}

This simplifies the controller to

[Route("api/metabuildCRs")]
public class MetabuildCRsController : Controller {
    private readonly IMetabuildCRsService service;

    public MetabuildCRsController(IMetabuildCRsService service) {
        this.service = service;
    }

    [HttpPost]
    public IActionResult GetMetabuildCrs([FromBody] MetabuildCRsRequest model) {
        if (model == null || model.BuildIds == null) {
            return BadRequest();
        }

        List<MetabuildCR> metabuildCRs = service.GetMetabuildCrs(model.BuildIds, model.CRs);

        return Ok(metabuildCRs);
    }
}

If anything changes in the core functionality then there is no need to touch the controller as it is performing its Single Responsibility of handling requests.

The service can be modified independently of the controller. It can also be reused elsewhere if needed.

I am personally not a big fan of using underscore prefixes on variable names, so will notice that I removed them all.

As for your concern about additional functionality, they can be isolated to their own service abstraction and added to this controller or its own controller depending on your choice. Splitting functionality into small easy to maintain modules helps separate concerns within the application and allows the code to grow softly

\$\endgroup\$
  • \$\begingroup\$ Do you think its necessary to create another service layer class? Couldn't I just put the extracted functionality into the Repository class? Also, you mention my concern for additional functionality, I think you are addressing my comment on "any other cases for which I should handle certain things coming in as requests"? If so, I really meant, "are there any other scenarios for which I should return something other than BadRequest() or Ok()?" \$\endgroup\$ – Window Apr 3 '18 at 23:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.