Refactored to use polymorphism in place of case statements

Question

The purpose of the below code is to return different types of job objects based on an argument named "jobType."

The original code for the below methods was implemented as a switch statement. I refactored the code removing the switch statement, and in place creating different classes representing different types of jobs.

I would like feedback on my below implementation. The code works, and is released in production, but perhaps there is a better approach.

public bool RunNextJob()
    {
        //  Return true if we found a job to run
        DeferredJob dJob = _jobRepository.GetNextJob();
        SessionProvider sessionProvider = ObjectFactory.GetInstance<SessionProvider>();

        if(dJob != null)
        {
            Logger.Info(string.Format("Running job type: {0} Id: {1}", dJob.JobType, dJob.Id));
            dJob.ProcessedDate = DateTime.Now;
            _jobRepository.Update(dJob);
            sessionProvider.Flush();

            var jobType = new JobType();
            var job = jobType.GetJobType(dJob.JobType);

            if (job != null)
                job.RunJob(dJob);
        }


public class JobType : IJobType
{
    public IJob GetJobType(string jobTypeCode)
    {
        if(jobTypeCode == JobTypeToProcess.Notify.ToString())
            return new NotifyJob();

        if (jobTypeCode == JobTypeToProcess.ConflictedAppointmentMessageJobType.ToString())
            return new ConflictedAppointmentMessageJob();

        if (jobTypeCode == JobTypeToProcess.UnifiedExport.ToString())
            return new UnifiedExportReportJob();

        return jobTypeCode == JobTypeToProcess.FTPJobType.ToString() ? new FTPJob() : null;
    }
}

Answer 1

public class JobType : IJobType
{
    public IJob GetJobType(string jobTypeCode)
    {
        if(jobTypeCode == JobTypeToProcess.Notify.ToString())
            return new NotifyJob();

        if (jobTypeCode == JobTypeToProcess.ConflictedAppointmentMessageJobType.ToString())
            return new ConflictedAppointmentMessageJob();

        if (jobTypeCode == JobTypeToProcess.UnifiedExport.ToString())
            return new UnifiedExportReportJob();

        return jobTypeCode == JobTypeToProcess.FTPJobType.ToString() ? new FTPJob() : null;
    }
}  

This seems to be some kind of a factory. So I would rename JobType to JobTypeFactory and IJobType to IJobTypeFactory having the GetJobType() method changed to Create().

• instead of using the string representation of the enum you should use Enum<T>.TryParse()

• I don't see any advantage on using multiple if statements over a switch..case.

• Although braces {} are optional you should use them always to make your code less error prone.

Putting the mentioned points together will lead to

public class JobTypeFactory : IJobTypeFactory
{
    public IJob Create(string jobTypeCode)
    {
        JobTypeToProcess jobType = JobTypeToProcess.None;
        if(!Enum.TryParse<JobTypeToProcess>(jobTypeCode, out jobType)) { return null; }

        switch(jobType)
        {
            case JobTypeToProcess.Notify: 
                return new NotifyJob();

            case JobTypeToProcess.ConflictedAppointmentMessageJobType: 
                return new ConflictedAppointmentMessageJob();

            case JobTypeToProcess.UnifiedExport: 
                return new UnifiedExportReportJob();

            case JobTypeToProcess.FTPJobType: 
                return new FTPJob();

            default: 
                return null;

        }
    }
}  

which is a little bit longer than the original, but it is more readable IMO.

---

public bool RunNextJob()
{
    //  Return true if we found a job to run
    DeferredJob dJob = _jobRepository.GetNextJob();
    SessionProvider sessionProvider = ObjectFactory.GetInstance<SessionProvider>();

    if(dJob != null)
    {
        Logger.Info(string.Format("Running job type: {0} Id: {1}", dJob.JobType, dJob.Id));
        dJob.ProcessedDate = DateTime.Now;
        _jobRepository.Update(dJob);
        sessionProvider.Flush();

        var jobType = new JobType();
        var job = jobType.GetJobType(dJob.JobType);

        if (job != null)
            job.RunJob(dJob);

        return true;
    }  

    return false;
}

• By adding a guard clause you could remove some horizontal spacing,

• Its a little bit strange setting the ProcessedDate before the job had been processed.

• What is the sense of getting the SessionProvider if maybe the job is null ?

• for the case you are running C# 6.0 you could use string interpolation instead of string.Format().

Leaving aside the setting of ProcessedDate because I don't know if that would cause sideeffects but including the changes from above, the mentioned points would lead to

public bool RunNextJob()
{
    //  Return true if we found a job to run
    DeferredJob dJob = _jobRepository.GetNextJob();

    if (dJob == null) { return false; }

    SessionProvider sessionProvider = ObjectFactory.GetInstance<SessionProvider>();

    Logger.Info($"Running job type: {dJob.JobType} Id: {dJob.Id}"); // C# 6.0 only

    dJob.ProcessedDate = DateTime.Now;
    _jobRepository.Update(dJob);
    sessionProvider.Flush();

    var jobTypeFactory = new JobTypeFactory();
    var job = jobTypeFactory.Create(dJob.JobType);

    if (job == null) { return false; }

    job.RunJob(dJob);

    return true;

}