5
\$\begingroup\$

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
}
toolic
14.3k5 gold badges29 silver badges200 bronze badges
asked Aug 16 at 11:03
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

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?

answered Aug 16 at 19:11
\$\endgroup\$
3
\$\begingroup\$

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);
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=63e36c4049adf44ccf1adf6f807bf451

answered Aug 19 at 7:41
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.