2
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Please edit the title to tell us roughly (or hint at) what your code is doing. \$\endgroup\$ – Mathieu Guindon Oct 13 '15 at 16:14
4
\$\begingroup\$
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;

}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks Heslacher. I really like your feedback, and refactoring. It's a nicer implementation. \$\endgroup\$ – derek kenney Oct 13 '15 at 19:32

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.