WaveBox
WaveBox copied to clipboard
einsteinx2
Clean up ApiHandlers and HttpProcessor classes
While making modifications today, I noticed some places where we have duplicate code, etc, that should be refactored to common places to prevent bugs in the future where we change it in one place but not another.
Also, the method signatures in HttpProcessor are getting pretty long, so that should probably be refactored now that it has all of the range, etc support that it needs and we won't likely be changing it further.
It looks like you took care of a lot of this in your previous commits. Any specific areas you'd like cleaned up now?
I'm working on simplifying some of the API handlers and moving things into the Utility class as well. Extension methods are so handy.
What prompted me to write the ticket was working on the streaming stuff and realizing that the Art, Web, Stream, and Transcode API handlers had some duplicate logic, especially the stream and transcode handlers. So I just wanted to clean that up, and potentially have some subclass others (like a FileApiHandler superclass with common methods or something). Just in general I never want to have the same code in 2 places as it's a recipe for bugs.
Then I figured while I was at it, I should just review all of them and see what other common logic, if any, I could pull out.
And regarding extension methods, I think as that Utility class grows, we should separate that out so that the extension methods are logically grouped, but that can be later.
These are still pretty messy. I'm trying to figure out a few good ways to refactor them, but I haven't come up with any ideas that I'm really happy with.
I think we should probably investigate the HTTP processor first, perhaps using the new System.Net.Http namespace classes? There are quite a few functions which return void and modify instance parameters, rather than returning a value to fill them. My personal preference would be to change this behavior, because it is hard to follow.
As for the API handlers, I would like to move to a reflection approach, like the services, but this would also require us to send all parameters in to the Process() method, as you had previously mentioned, Ben. I also wonder if it would be a better idea for us to return JSON strings from the handlers, and send them via the HttpProcessor class directly, rather than passing around the processor object.
Just a few things to think about. I think this subsystem is definitely in need of some cleanup and improvement.
Ya it's all kind of spagetti code right now. It works but it's not maintainable. I'm not sure we can use the higher level HTTP constructs, but we can take a look. Either way we definitely need to refactor the processor class, potentially breaking it into multiple classes, and we definitely need to stop passing processors around if possible. Also we'll need to refactor the API handlers. Right now crunched for time, so since this is working, we might have to hold off for a bit.
Yeah, certainly. Once other obligations are out of the way, I'd be happy to work with you to come up with a plan to further decouple the HttpProcessor and ApiHandlers. It won't be a trivial task, but in the end, I think we will be better for it.