Consider changing documentation to avoid parameterless constructor pitfall

[Zero3]

The documentation suggests scheduling jobs using the .Schedule<T>() method. This method has a common pitfall though: If T does not have a parameterless constructor, a System.MissingMethodException will be thrown at runtime.

In my case, I just broke a job during refactoring because I added a constructor that took a parameter to it and forgot/was unaware that FluentScheduler was responsible for initializing it through reflection. Since no compile-time error was raised, I did not notice my mistake before the exception was thrown at runtime.

To avoid this pitfall, I suggest these changes:

1. Change the documentation to suggest the compile-time safe .Schedule(Action) and/or Schedule(IJob) methods instead of the .Schedule<T>() method.

2. Document this pitfall for the .Schedule<T>() method.

[tallesl]

Or better yet, I can replace where T : IJob for where T : IJob, new() on the library, this way you would get a compilation error (1 2 3 4 5).

Thank you for reporting it.

[jplindgren]

@tallesl the first thing I thought was the new() constraint in the generic too. But thinking twice i think this is not a good solution.
Correct if i am wrong, but:
If you put it you´ll force every implementation of IJob used by the method "Schedule" to have a parameterless constructor, while this can be good in the scenario painted by @Zero3 it can be bad in others scenarios. One of scenarios that it would be bad and come on top of my mind is when you use an IJobFactory to instantiate your jobs. We we´ll be forced to have a parameterless constructor in every implementation, even if it is not needed, because it is being constructed by my dependency injection container.

[Zero3]

I don't have too strong opinions about this, but my thoughts are that the construction of the job object should not be done by FluentScheduler. Since the construction logic is job-specific, FluentScheduler cannot reasonably know how to construct the object. Requiring a parameterless constructor for this seems kind of hacky to me.

This is why I think using the other two scheduling methods are a better approach. With those, the caller handles the construction logic either by passing an existing object or a lambda/action/factory that FluentScheduler can use to construct one without knowing how it will be done.

[Zero3]

On a related note, I just messed up using .Schedule(Action) as well. I mistakenly thought that I was supposed to give it an IJob factory, like .Schedule(() => new SomethingJob()), which fails in practice since neither my lambda nor FluentScheduler calls .Execute() on the job. Instead, I should have used .Schedule(() => new SomethingJob().Execute()).

Perhaps you could consider adding a .Schedule(Func<IJob>) variant of the method as well, so that you can pass in an IJob factory as I intended to.

[tallesl]

Sorry for the long delay!

You guys are absolutely right, it totally slipped my mind all the use cases that new() would break. It would break not only IJobFactory, but also a way for the user to hand an already instantiated IJob that I implemented some time ago (#65).

Thanks for being so attentive and correcting me.

It's not much, but I added a remark on IJob XML documentation about this issue (159d729). I'm afraid that's all I can do. I thought of checking for a parameterless constructor and throwing a more specific exception, but I'm afraid of the overhead of the reflection required to do it.

Perhaps you could consider adding a .Schedule(Func<IJob>) variant of the method as well, so that you can pass in an IJob factory as I intended to.

Someone made a pull request of exactly that (#115).

[tallesl]

Just published on nuget.org, version 5.2.0.