For context, I'm very new to Rust, and I'm trying to learn the best practices and nuances of it as I go.
I've written this small function that takes a HashMap<String, String>
and parses it into a http::HeaderMap
.
Any feedback on style, idiomatic inconsistencies and general best practice or code smells is appreciated.
fn build_headers(headers: &HashMap<String, String>) -> HeaderMap {
let mut header_map = HeaderMap::new();
for (key, value) in headers {
let key = match HeaderName::try_from(key) {
Ok(key) => key,
Err(_) => {
eprintln!("Invalid header name: {}", key);
continue
}
};
let value = match value.parse() {
Ok(v) => v,
Err(_) => {
eprintln!("Invalid header value: {}", value);
continue
}
};
header_map.insert(key, value);
}
header_map
}
2 Answers 2
temp vars
let key = match ...
Ok(key) => key,
Prefer a 2nd line of Ok(k) => k,
The following looks great:
let value = match ...
Ok(v) => v,
If you don't rename to k
, possibly you'd also like to use value
for v
.
diagnostics
eprintln!("Invalid header name: {}", key);
Consider using log::warn()
for that.
Then we can adjust the logging config to make
this code more or less chatty
in a testing or a production setting.
We can't see what other things get logged in the course of
satisfying a web request.
But if I were a maintenance engineer charged with chasing
down a bug, I doubt that just knowing key
would suffice.
Typically we'll want to know details like
the client's IP or session identifier, and
the full URL being requested.
In other words, enough information to use
a tool like the Postman app in order to reproduce the error.
Consider altering the Public API you export, if "good diagnostics" is a requirement. Maybe you'd like to also return a set of "bad" headers, which we typically expect to be empty? Then we can let the caller worry about what to do. Notice that the caller hangs onto more context, which can be logged, than the OP code has available to it. Or maybe you'd like to pass in some context, so that this routine can offer better diagnostic messages?
There already is a generic fallible conversion implemented from &HashMap<_, _>
to HeaderMap
:
https://github.com/hyperium/http/blob/b53194720352ef923d5fa662bc52592520e8b3ce/src/header/map.rs#L2068
Hence, your entire function is superfluous:
use http::HeaderMap;
use std::collections::HashMap;
fn main() {
let mut hash_map = HashMap::new();
hash_map.insert("Content-Type".to_string(), "application/json".to_string());
let header_map: HeaderMap = (&hash_map)
.try_into()
.expect("Hash map should contain valid headers.");
dbg!(header_map);
}