Finished ch. 1 of Web Browser Engineering, please review my code

code review
1

Hi,

I'm following along the online book Web Browser Engineering, using Rust instead of Python. I hope someone can provide some feedback about what I could improve in my code, as I am relatively new to Rust.

Right now, the "browser" simply takes an url as a cli param and prints the response without tags. It supports https, redirects, gzip-encoding, chunked transfer encoding, and caching.

Project on Github

Thanks.

3 Likes
2

:star_struck:this is exactly what I've been looking for. I was trying to find a simple implementation of web browser. Subscribed!

3

Hi Daniel,

I took a quick look, your Rust code is looking quite good :slight_smile:
I think a valuable lesson when learning Rust is how to properly handle and forward errors (e.g. From impls between different error types). If unwrap() fails it panics with a quite user-hostile error message. And if you are intending to upgrade your command-line tool to a longer-lived program with rendering you probably also don't want to crash when e.g. a server sends you some non-ASCII headers or redirects to a malformed URL.

Your Url::new could be improved, you always set the default port even when it's not needed and I wouldn't check host.contains(':') before doing host.splitn(2, ':');. And you could also take advantage of an impl Scheme { fn default_port(&self) -> u16; }, e.g:

let mut host_port = "example.com:234".split(':');
let host = host_port.next().unwrap();
let port = host_port.next().map(|s| s.parse().unwrap()).unwrap_or_else(|| scheme.default_port());

Also note that real URLs have a bunch of features you don't cover (e.g. fragments or userinfo). For proper browser-grade URL parsing you could use the url crate. In RequestHandler::request you do:

                    Url::new(&format!(
                        "{}://{}:{}{}",
                        url.scheme, url.host, url.port, location
                    ))
                    .unwrap()

which seems to just needlessly allocate a string which you then parse again. You could instead directly create a new URL from the given fields.

You do if let Some(...) = sth { ... } else { ... } a couple of times, which I am personally not a fan of. I'd rather either return in the first block and omit the else or use a match block. But that's just my personal preference.

You might want to change your main method to exit with a non-zero status code in case of an error and you could add more info to Cargo.toml e.g. authors, license, description and repository.

1 Like
4

Hi Martin,

Thanks for your detailed feedback, it's much appreciated.

5

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.