Fetching records, optionally including deleted records

I have an original method that almost does what I want (a method I did not create), but now I need to find records that were deleted so I added another method GetAllGuideRecords minus the delete condition in the where clause.

But now I kind have repeated code so I decided to pack everything in one method, with an optional parameter. I sometimes like using optional parameters because it gives me the flexibility to not break existing code.

Should I create overload method instead, stick with what I have, or is there a better way?

Original code:

public ED010_GUIDE GetNonDeletedGuideRecords(int guideId, int versionId)
        {
            var rec = ErgDb.ED010_GUIDE.FirstOrDefault(a => a.ERG_EDITION_ID == versionId && a.GUIDE_ID == guideId && a.DATE_DELETED_DTE == null);
            return rec;
        }

public ED010_GUIDE GetAllGuideRecords(int guideId, int versionId)
    {
        var rec = ErgDb.ED010_GUIDE.FirstOrDefault(a => a.ERG_EDITION_ID == versionId && a.GUIDE_ID == guideId);
        return rec;
    }

New code:

public ED010_GUIDE GetGuideRecord(int guideId, int versionId, bool getDeletedAndActiveRecords )
        {

            var rec = ErgDb.ED010_GUIDE.FirstOrDefault(a => a.ERG_EDITION_ID == versionId && a.GUIDE_ID == guideId);

            if (!getDeletedAndActiveRecords)
            {
                return  (rec != null && rec.DATE_DELETED_DTE == null)? rec:null;
            }
            else
            {
                return rec;
            } 

        }

Lots of ways to approach this one. Here is how I would approach this (and why).

When working through decisions like this, I tend to think about several concerns:

• Maintainability

• Readability

• Testability

• Scalability

Maintainability

A core principal in S.O.L.I.D. programming is the idea of the "O" or the Open closed principal which states:


Code should be open for extension and closed for modification.

In other words, I shouldn't have to change existing code to accomodate new functionality in an ideal world.

There are several reasons for this:

• In some schools of thought, EVERY change is a breaking change (change == bad).

• When changing existing code, all dependent code needs to be tested for regressions / compatibility and any derived code must be checked for consistency.

• Classes should only change as a result of fixing bugs.

Readability

This is where opinions tend to vary widely, but IMO overloads are much easier to read.

When you use an overload, it allows you to specify an additional set of documentation:

/// 
/// ...description stuff... This method differs from overloads because of x.
/// 

I find this DRAMATICALLY helps readability (and maintainability) for future devs. It also speaks to the initial intent of your authorship so the functions function is explicit and unlikely to be misunderstood.

Your duplication is also unnecessary since in your case the calls could be chained together:

public IQueryable GetGuideRecords(int guideId, int versionId, bool includeDeleted)
{
    var rec = this.GetGuideRecords(guideId, versionId);

    if (!includeDeleted) {
        rec = from x in rec
              where x.DATE_DELETED_DTE == null
              select x;
    }

    return rec;
}

public IQueryable GetGuideRecords(int guideId, int versionId)
{
    return from x in ErgDb.ED010_GUIDE
           where x.ERG_EDITION_ID == versionId && x.GUIDE_ID == guideId
           select x;
}

Testability

This one can really go either way in your case. I would opt for the separation into separate methods most likely because it means that the setup logic for any unit tests would be much simpler as I don't have to account for this or other future scenarios breaking my tests.

Which leads me to my last thought.

Scalability

Or perhaps Flexibility is a better term.

The optional parameters way gets REALLY messy if additional circumstances continue to evolve in the future as you are setting a precedent of non-explicit method signatures. Future DEVs or indeed yourself are likely to continue the pattern of fragile conditionals, broken unit tests and vague signature usage that can lead to bugs.

Recommendation

Ultimately, I would probably do two things:

• Don't do either

• Use a "unit-of-work" as an argument

I would go for something like this:

public virtual IQueryable GetGuideRecords(GuideQueryRequest request)
{
    var rec = from x in ErgDb.ED010_GUIDE
              where x.ERG_EDITION_ID == request.VersionId && x.GUIDE_ID == request.GuideId
              select x;

    if (!request.IncludeDeletedRecords)
    {
        rec = from x in rec
              where x.DATE_DELETED_DTE == null
              select x;
    }

    return rec;
}

public class GuideQueryRequest
{
    public GuideQueryRequest()
    {
    }

    public int VersionId { get; set; }
    public int GuideId { get; set; }
    public bool IncludeDeletedRecords { get; set; }
}

The inclusion of the GuideQueryRequest type means that you can add new properties to your signature without having to change your signature and break all of the code that depends on it.

This strategy leverages the best aspects of maintainability, flexibility, and readability because you don't need to create additional overloads to supply new arguments, and there are no optional arguments to specify and create ambiguity.

It also gives you a canvas to add robust documentation to the GuideQueryRequest type on how it should be used and what the properties are used for.

In theory, whichever type defines the GetGuideRecord would mark that method as VIRTUAL so that derived types can override the functionality (if new properties are added to the GuideQueryRequest type) and not break the existing functionality.

EDIT

Based on all of the great feedback from @Heslacher, I have updated the above code examples to be correct.

I have also changed the signature of the methods to return IQueryable instances. It depends on the needs of your application, but I like to have the option of performing additional sorting operations or filtering on a query before the query has been executed. It also makes more sense given that the method doesn't know what the criteria of the GuideQueryRequest properties will be, it is impossible for this function to know whether or not multiple records (or indeed any records) will be returned. This delegates the responsibility of how to use the query and the query results to the calling code.

I switched to

StackExchange Code Review Q#143929, answer score: 4