3
\$\begingroup\$

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;
            } 

        }
\$\endgroup\$
2
  • \$\begingroup\$ This is code review at least format left \$\endgroup\$
    – paparazzo
    Oct 14, 2016 at 21:58
  • \$\begingroup\$ Unless ERG_EDITION_ID == versionId && a.GUIDE_ID is a PK FirstOrDefault is nondeterministic; \$\endgroup\$
    – paparazzo
    Oct 14, 2016 at 22:31

2 Answers 2

4
\$\begingroup\$

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:

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

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<ED010_GUIDE> 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<ED010_GUIDE> 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:

  1. Don't do either
  2. Use a "unit-of-work" as an argument

I would go for something like this:

public virtual IQueryable<ED010_GUIDE> 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 Linq-to-SQL syntax because I like that format when querying a database and expression syntax when working in memory with results.

For more information on S.O.L.I.D. and a definition of letters and theory, check out these links:

\$\endgroup\$
5
  • 2
    \$\begingroup\$ Your code examples have some problems. In the very first example you are calling var rec = this.GetGuideRecords(guideId, versionId); which returns either null or a ED010_GUIDE object. If includeDeleted you try to call Where() extension on a single object not an enumerable. In addition you have used the condition wrong (rec.DATE_DELETED_DTE == null means the record isn't deleted). \$\endgroup\$
    – Heslacher
    Oct 12, 2016 at 5:11
  • 2
    \$\begingroup\$ In the last example using the GuideQueryRequest, which by the way is a good idea, you use FirstOrDefault() assuming that there first returned item will have the desired "deleted flag" which hasn't to be the case. Switch FirstOrDefault() to Where() and based on the condition call either FirstOrDefault() or FirstOrDefault(rec.DATE_DELETED_DTE != null) an all will be good. \$\endgroup\$
    – Heslacher
    Oct 12, 2016 at 5:11
  • \$\begingroup\$ @Heslacher Updated accordingly. Thanks for catching those things. I got too excited about the method overloading portion and missed those points. \$\endgroup\$
    – xDaevax
    Oct 12, 2016 at 13:32
  • \$\begingroup\$ Thanks for the response, I also like the unit of work idea, definitely will keep that in mind for the future, but wont be able to implement since I would be deviating from current setup, I would chain together the methods, but the original method filters out deleted records, to make it work I would have to violate the "O" principle and modify the original method to include deleted records, then chain like your example, feels like I'm stuck between a rock and hard place \$\endgroup\$
    – Sobbles
    Oct 12, 2016 at 13:41
  • \$\begingroup\$ You left out the FirstOrDefault \$\endgroup\$
    – paparazzo
    Oct 14, 2016 at 22:22
0
\$\begingroup\$
  1. First of all: your function naming is confusing in the way you use singularis/pluralis (-Record/-Records). They all returns a single (or null) object so stick to -Record.
  2. I would not filter the records by DATE_DELETED_DTE, but leave it to the client to check the returned object (for null and deleted-flag). That's because if you provide a function with the indclude-deleted-flag the client will have to check for null and/or rec.DATE_DELETED_DTE == null anyway, and he/she is not helped at all. So unless some clients are not allowed to access deleted records I'll suggest the following:

`

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

In that way the client also know from the return value if the record actual exists (but maybe deleted) or not (null is returned).

\$\endgroup\$
6
  • \$\begingroup\$ The filtering of the records by the caller won't work either with the provided code. See my comment: codereview.stackexchange.com/questions/143929/… \$\endgroup\$
    – Heslacher
    Oct 12, 2016 at 6:40
  • \$\begingroup\$ @Heslacher: I can't stand up for the meat of my suggestion (I've copied that from the question) but I think my conclusion is justifiable from my arguments :-). My point is that the caller does not gain anything from a deleted-flag. \$\endgroup\$
    – user73941
    Oct 12, 2016 at 6:56
  • \$\begingroup\$ What if the first record was not deleted but the second was? \$\endgroup\$
    – paparazzo
    Oct 14, 2016 at 22:29
  • \$\begingroup\$ @Paparazzi: Good question, but not for me to answer. If the guidId and versionId identify more than one record, then the use of FirstOrDefault() is wrong in the OP's code in the first place. I anticipate from his code they identify one or none record. Else his (and mine) functions should return a vector of elements. Or am I missing something? \$\endgroup\$
    – user73941
    Oct 15, 2016 at 5:22
  • \$\begingroup\$ Pretty big unstated assumption \$\endgroup\$
    – paparazzo
    Oct 15, 2016 at 5:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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