high-assurance-rust icon indicating copy to clipboard operation
high-assurance-rust copied to clipboard

Example is missing a generic

Open tatupesonen opened this issue 2 years ago • 3 comments

Hello! I'm looking at https://highassurance.rs/chp7/traits.html#the-map-get-api. The example with the get is never introducing the second generics K and V in the code example.

/// Returns a reference to the value corresponding to the key.
pub fn get<Q>(&self, key: &Q) -> Option<&V>
where
    K: Borrow<Q> + Ord,
    Q: Ord + ?Sized,
{
    // ...function body here...
}

I think a better example would be:

impl<K, V> BTreeMap<K, V> {
    pub fn get<Q>(&self, key: &Q) -> Option<&V>
    where
        K: Borrow<Q> + Ord,
        Q: Ord + ?Sized,
    {
        // ...function body here...
    }
    // ... rest of the methods
}

to show where the generic for BTreeMap are introduced. There is a mention of BTreeMap<K, V> but in my opinion I think it's easily missed and this would illustrate how the generics from the impl are used.

tatupesonen avatar Jun 14 '22 10:06 tatupesonen

While I like including explicit information,

This information is already stated a line previously.

Based on the above, you might expect the get method's signature to look like this for BTreeMap<K, V>:

furthermore, it distracts from the objective of the article. I would like to mention that this article isn't a tutorial for building a BTreeMap (otherwise, direct copy and paste would be desirable), but rather an explanation of why BTreeMap uses more than just Ord as a constraint.

Skarlett avatar Jun 14 '22 21:06 Skarlett

Hey all! Thank you both for looking into this.

Indeed, the text mentions BTreeMap<K, V> and the focus is on why Ord alone doesn't quite get us everything we'd want. So these snippets omit the K and V for brevity.

However, I agree the suggested change would be an improvement! Having both the current text and impl<K, V> BTreeMap<K, V> { ... } code blocks is likely the most clear: Q stands out more as being introduced by fn get<Q> and missing from the impl<K, V> scope (as pointed out). And in this paragraph we're still talking about BTreeMap, haven't gotten to scapegoat trees yet!

I read this really great blog post once about writing technical books. Can't find the original link (will share if I find it later) but the advice was something like "always go for clear over clever, if it's not clear then it can't be clever". Here omitting K and V is "clever" but makes things less "clear". :wink:

How about making the suggested change to the first 2 code blocks containing fn get in this section? Would you like to PR this update?

IMO the last block containing fn get, which adds Default to the signature, is safe to leave as is because by that point the reader will be aware of the surrounding impl - having seen it twice before.

tnballo avatar Jun 15 '22 02:06 tnballo

Hey, sorry for the super late reply. I will look into creating a PR for this today!

tatupesonen avatar Sep 17 '22 11:09 tatupesonen