Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up ApiHandlers and HttpProcessor classes #88

Open
einsteinx2 opened this issue Jun 14, 2013 · 7 comments
Open

Clean up ApiHandlers and HttpProcessor classes #88

einsteinx2 opened this issue Jun 14, 2013 · 7 comments
Assignees

Comments

@einsteinx2
Copy link
Owner

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.

@ghost ghost assigned einsteinx2 Jun 14, 2013
@mdlayher
Copy link
Contributor

It looks like you took care of a lot of this in your previous commits. Any specific areas you'd like cleaned up now?

@mdlayher
Copy link
Contributor

I'm working on simplifying some of the API handlers and moving things into the Utility class as well. Extension methods are so handy.

@einsteinx2
Copy link
Owner Author

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.

@mdlayher
Copy link
Contributor

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.

@einsteinx2
Copy link
Owner Author

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.

@mdlayher
Copy link
Contributor

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.

@einsteinx2
Copy link
Owner Author

Sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants