Refactoring to reduce redudant code advise

[JasinCole]

Hi, I'm new here and recently found my way back to C# for a project I am working on.
While I can fudge my way through programming in lot's of languages I've never been proficient or effective in any language. Time to change that...

Given the below code, which works as designed, obvious issues of redundancy I am looking for some advise on how I can reduce the redudant code as this class grows.
I don't understand how I should handle the DbContext that both functions need. I read that the DbContext should be short lived so it would seem logicial to make that local to each function. But then if I created a third private function to return the employee record (that both functions need) and pass that back to the functions I'd still need a local DbContext for the Entry operations. Can the DbContext be a class field in this instance? What determines whether something is long or short lived?
This is where my lack of hard core fundamentals lets me down and I want to learn the right way and the thought process behind it.

using DatabaseAccess.Entities;
using DatabaseAccess.Context;
using Microsoft.EntityFrameworkCore;

namespace DatabaseAccess;

public class EmployeeRecords
{
    private readonly string? _connectionString;

    public EmployeeRecords(string? connectionString)
    {
        _connectionString = connectionString;
    }

    public Employ GetRecord(int employeeNum)
    {
        using var ctx = new SageDbContext(_connectionString);
        var employee = ctx.Employs
            .FirstOrDefault(employee => employee.Recnum == employeeNum) ?? new Employ();
        ctx.Entry(employee)
            .Collection(e => e.Payrecs!)
            .Load();
        return employee;
    }

    public Employ GetRecord(int employeeNum, List<DateTime> payPeriods)
    {
        using var ctx = new SageDbContext(_connectionString);
        var employee = ctx.Employs
            .FirstOrDefault(employee => employee.Recnum == employeeNum) ?? new Employ();
        ctx.Entry(employee)
            .Collection(e => e.Payrecs!)
            .Query()
            .Where(payrec => payPeriods.Contains((DateTime)payrec.Payprd!))
            .Load();
        return employee;
    }
}

 

[JohnH]

Try the Extract a method refactoring - Visual Studio (Windows)

 

[Skydiver]

In general, you put that stuff that varies into parameters, and the framework of the method stays the same. For clarity of code, you may decide to make the method that takes a ton of parameters less public.

Also using a less fluent style may help identify where would be the good points to insert functionality to take advantage of those parameters.

The following is untested code but may show you a path forward:

public Employ GetRecord(int employeeNum)
    => GetRecord(employeeNum, payrec => true);

public Employ GetRecord(int employeeNum, List<DateTime> payPeriods)
    => GetRecord(employeeNum, payrec => payPeriods.Contains((DateTime)payrec.Payprd!));

protected Employ GetRecord(int employeeNum, Predicate<Payrec> payRecFilter)
{
    using var ctx = new SageDbContext(_connectionString);
    var employee = ctx.Employs
        .FirstOrDefault(employee => employee.Recnum == employeeNum) ?? new Employ();
    ctx.Entry(employee)
        .Collection(e => e.Payrecs!)
        .Query(payRecFilter)
        .Load();
    return employee;
}

 

[cjard]

Is that EFCore? I ask because you seem to be implementing some sort of manual version of Include?

public class EmployeeRepository //do not name classes in the plural. Typically a class that abstracts away database specifics into business requirements "GetXByY" is named ..Repository. Not a hard rule
{
    private SageDbContext _sageDbContext; //context will live to the lifetime of the repo, which you will also inject with a short lifetime

    public EmployeeRepository(SageDbContext sageDbContext)  //dependency injector will provide a working instance of this
    {
        _sageDbContext = sageDbContext;
    }

    public Employ GetEmployeeOrNewByEmployeeNum(int employeeNum, List<DateTime> payPeriods = default) //no ration on chars for method names
    {
        var eq = _sageDbContext.Employs.Include(e => e.Payrecs).Where(employee => employee.Recnum == employeeNum); //making a queryable means you can add more clauses conditionally

        if(payPeriods != default) //and they are ANDed together
          eq = eq.Where(e => e.Payrecs.Any(pr => payPeriods.Contains(pr.Payprd))); //can be quirky getting these double nesters to work out; in other cases it may be simpler to start from payrecs and work back

       return eq.FirstOrDefault() ?? new Employ();  //personally I'd return the null
    }


    //or this, if you're query splitting manually and you only want certain persiods rather than trying to use the perdiods to filter for employees that have them
    public Employ GetEmployeeOrNewByEmployeeNumAndPayPeriods(int employeeNum, List<DateTime> payPeriods = default) 
    {
        var e = _sageDbContext.Employs.Find(employeeNum); 

        if(e != default && payPeriods != default) 
          _sageDbContext.Payrecs.Where(pr => pr.EmployeeRecnum == employeeNum && payPeriods.Contains(pr.Payprd)); //EFC relationship fixup will put these PRs into the e.,Payrecs collection after download

       return e ?? new Employ();  //personally I'd return the null
    }
}

Don't try too hard to parameterize the predicates you pass in; eventually you'll go so crazy with parameterization you'll end up reinventing all the stuff EFC was invented for and the compelxity of using your hyper-adaptable super-generic EFC wrapper is on par with just using the context directly. Mmm; spaghetti

To some extent I'd even say that this is could be too far; having that optional parameter. Two methods GetXByY and GetXByYAndZ would read well for me

 

[JasinCole]

Is that EFCore? I ask because you seem to be implementing some sort of manual version of Include?

public class EmployeeRepository //do not name classes in the plural. Typically a class that abstracts away database specifics into business requirements "GetXByY" is named ..Repository. Not a hard rule
{
    private SageDbContext _sageDbContext; //context will live to the lifetime of the repo, which you will also inject with a short lifetime

    public EmployeeRepository(SageDbContext sageDbContext)  //dependency injector will provide a working instance of this
    {
        _sageDbContext = sageDbContext;
    }

    public Employ GetEmployeeOrNewByEmployeeNum(int employeeNum, List<DateTime> payPeriods = default) //no ration on chars for method names
    {
        var eq = _sageDbContext.Employs.Include(e => e.Payrecs).Where(employee => employee.Recnum == employeeNum); //making a queryable means you can add more clauses conditionally

        if(payPeriods != default) //and they are ANDed together
          eq = eq.Where(e => e.Payrecs.Any(pr => payPeriods.Contains(pr.Payprd))); //can be quirky getting these double nesters to work out; in other cases it may be simpler to start from payrecs and work back

       return eq.FirstOrDefault() ?? new Employ();  //personally I'd return the null
    }


    //or this, if you're query splitting manually and you only want certain persiods rather than trying to use the perdiods to filter for employees that have them
    public Employ GetEmployeeOrNewByEmployeeNumAndPayPeriods(int employeeNum, List<DateTime> payPeriods = default)
    {
        var e = _sageDbContext.Employs.Find(employeeNum);

        if(e != default && payPeriods != default)
          _sageDbContext.Payrecs.Where(pr => pr.EmployeeRecnum == employeeNum && payPeriods.Contains(pr.Payprd)); //EFC relationship fixup will put these PRs into the e.,Payrecs collection after download

       return e ?? new Employ();  //personally I'd return the null
    }
}

Don't try too hard to parameterize the predicates you pass in; eventually you'll go so crazy with parameterization you'll end up reinventing all the stuff EFC was invented for and the compelxity of using your hyper-adaptable super-generic EFC wrapper is on par with just using the context directly. Mmm; spaghetti

To some extent I'd even say that this is could be too far; having that optional parameter. Two methods GetXByY and GetXByYAndZ would read well for me

it is in fact EFCore, now that you mentioned it, include does indeed look to be exactly what I need.

I'm having a hard time trying to figure out the dependency injection. If I understand it correctly shouldn't we be passing in an interface to the constructor? But how then do you access the properties of the SageDbContext class since all EF DbContext will have different properties while a interface should be generic? Theory always seems to make things much easier until you try and implement it...

 

[Skydiver]

Before there were more modern mocking tools, declaring the constructor parameters new needed to use interfaces to give you a chance to use mocks when unit testing. If you have no plans of unit testing, or you are using a mocking tool that lets you mock concrete classes, then use concrete classes.

 

[cjard]

I'm having a hard time trying to figure out the dependency injection.

It's nothing you haven't done before, it's just the perspective on it that I found confusing

A class has dependencies; things it needs to work. A person data is downloaded from an api; the class that wants the person data has a dependency on the class that does the downloading

class School{
  void GetStudentData(int studentId){
    var a = new ApiClient();
    return a.GetPersonDataById(studentId);
  }
}

This School class now depends on ApiClient; anywhere it is used, the library that implements ApiClient has to go also.

This, notionally is very simple dependency injection:

class School{
  ApiClient _a;
  School(ApiClient a){
    _a = a;
  }

  void GetStudentData(int studentId){
    return _a.GetPersonDataById(studentId);
  }
}

We can now inject (send in) the class that School depends on, by providing a constructor argument

var s = new School(new ApiClient());

The lib that implements it still has to go with it, but here we are injecting the thing depended on but we've removed the need for School to use the new keyword

Let's make it more flexible

class School{
  IApiClient _a;
  School(IApiClient a){
    _a = a;
  }

  void GetStudentData(int studentId){
    return _a.GetPersonDataById(studentId);
  }
}

By making it an interface we could either send in the real ApiClient that makes a http call and incurs a fee, or we could write a class that just returns the same dummy data for GetPersonDataById - this would make our testing life easier but it's not a mandated part of dependency injection

So we've talked about removing the use of "new" in classes throughout, by giving them their dependencies rather than having them make them themselves - if we hand the job of "new"ing off to something we used to call an "inversion of control container" then we don't have to "new" anything any more - tell c# how to "new" everything we use and then we don't make anything; c# makes them and passes them in. We can reconfigure entire hierarchies of dependent objects just be reconfiguring the IOC container options to use our test classes or real ones. C# needs to know some stuff like whether it should make new classes often or reuse the same ones, and options it should pass to the creates objects but largely all the "new"ing is done by something like reflection

None of that is DI in the strict sense but we've tended to stop using the term IOC and just call it an "dependency injector" now..

As you've seen you don't have to use interfaces, you can use concrete classes. Microsoft's DI mechanism has a dedicated route for registering dbContexts that sets appropriate lifecycle options; don't make them too long lived. Particularly in a Blazor server app, object a context factory instead and use it to manufacture contexts on demand per method

If I understand it correctly shouldn't we be passing in an interface to the constructor? But how then do you access the properties of the SageDbContext class since all EF DbContext will have different properties while a interface should be generic?

An interface "should be generic" - I think there's another confusion there

An interface should specify an external appearance of an object so it can be treated consistently even though the class that actually provides the implementation differs. It's nothing to do with c# Generics and it's not the case that interfaces should be some bland minimal thing that doesn't have necessary properties and methods to implement the program logic. Interfaces should be as complex as necessary to make the program work

Theory always seems to make things much easier until you try and implement it...

Do concrete classes for now. If you want to use interfaces then your interface needs to have literally everything you use from your concrete class. You create the interface by looking at every method and property of the concrete class that is called by the class that accepts it for inject; that's your interface spec

 

[cjard]

I forgot to mention a bit in the above post:

So if we stop our low level class (School) needing to use "new" to create things it depends on, and then we stop the class above it (the class that depends on school and does a new School(new ApiClient) from having to use "new" by injecting a school and an api client, we start to focus all the "new"ing up near the top of the dependency hierarchy. If the root classes create the dependencies and hand them down then you've concentrated a single location that does the new'ing which gives you an easy place to swap out the classes rather than having to look all over the code base (eg Find/Replace ApiClient with FakeApiClient). The classes down the dependency tree just use what they're given, and you have this one place where everything is configured. That's quite a common theme you've used before too, with stuff like logging or config

So that "the classes above create the things the lower classes need to work" isinversion of control" and, to get rid of using the new keyword in our code completely we can hand that job over to an IOC container. When you see a .net core web app (most prevalent use i'd say and asides to explain) doing things in its program or startup class like services.AddScoped<X,Y>(); - you're telling C# "when a request comes in, chase down the hierarchy of needs, starting with the controller that handles the request; look at what that controller takes as parameters to its constructor, and if it requires an X provide a new instance of Y... and look at what Y's constructor needs, and provide those. And look at what their constructors need, and provide those.. And.."

The transient/scoped/singleton specify how ofte C# should provide a new object or reuse one it just made; you can read up on the differences, but Scoped is the most common, and means "per handled request" whereas Singleton is like it being a static class; there is only one of them ever

--

DI doesn't remove all new'ing- it would perhaps be awkward and pointless to get an injector to create a data POCO for us. Things don't need uncoupling to that extent of abstracting classes that just hold data and don't embody any logic, but perhaps there is a scenario where a data class has one set of defaults in X many places and another set of defaults in Y many places; perhaps then we could create a factory that knows how to make each and inject the factory and have it manufacture our POCOs on demand; the factory uses new, but it represents another focusing down of not having new in hundreds of places around the codebase

 

[JasinCole]

@cjard thanks for that explanation. I assumed a lot of that theory, but putting it all together into a seemless understanding sometimes drives me crazy.

Per the interface discussion, you say that these should be as complex as needed to make the application work. Does it make sense to create an interface that only applies to one class? If it doesn't make sense then why, other than to fake a class for testing, do it?
I guess what I am asking is it just good design practice for future proofing an application? I do understand there is times that using an interface is helpful to make sure a concrete class implementation will function with other code by providing all the properties and methods needed for that interaction.

 

[cjard]

Does it make sense to create an interface that only applies to one class?

Probably not

If it doesn't make sense then why, ... do it?

I didn't realize that I'd advocated to!

it just good design practice for future proofing an application?

Usually it's nothing that can't be done in the future, when the future is known.

Go for the simple solution, not the perfect one