Refactoring HTTP #11
Refactoring HTTP #11
Conversation
|
Hello. Thank you for the PR, I'll drop some notes on it. |
src/server.rs
Outdated
| @@ -39,7 +40,7 @@ impl Server { | |||
|
|
|||
| pub fn build_http_request(mut stream: TcpStream) -> (TcpStream, Request) { | |||
| let (request, body) = parse_tcp_stream(&mut stream); | |||
| let request = parse_http_string((request, body)); | |||
| let request = Request::from_str(&request).unwrap().body(body).clone(); | |||
There was a problem hiding this comment.
Implementing from_str is a good idea.
However, this
(&request).unwrap().body(body).clone();is why I don't like chainable methods.
I feel like it's hurting readability here.
IMO the previous code is a bit better, and a bit more abstract, instead of passing a reference then unwrapping then calling another method to set the body then cloning.
Maybe a good middle ground is implementing a method on the struct like what parse_http_string is doing and having it construct the struct with the body.
I think an even better solution would be implementing parse_tcp_stream on the Request struct, after all, it is related to it, and it's also the only thing using it.
There was a problem hiding this comment.
Chainable methods is needed here, You can't passing any arguments other than string that you want parse, So It's important here and unavoidable (Unless there is a way that I don't know) if we want use from_str
src/http/methods.rs
Outdated
| PATCH, | ||
| DELETE, | ||
| HEAD, | ||
| OPTIONS, |
There was a problem hiding this comment.
Does it really need its own file? :D
There was a problem hiding this comment.
https.rs was crowded with many things. Frankly, I like organization and I do not see that there is a problem with a large number of files, but everything is organized.
Maybe in future you can add Http status codes enum here, But the difficulty will be in naming the file :D
src/http/mod.rs
Outdated
| true => { | ||
| self.headers.insert( | ||
| "Content-Type".to_string(), | ||
| "multipart/form-data".to_string(), |
There was a problem hiding this comment.
Again, not particularly fond of chainable methods.
|
Thanks for the PR! |
What's new?