Explicit rules for readbility or abstract over the principle for expresivity

code review
1

Hi Guys, I'm struggling to make up my mind which is better:

A

Pros:

  • Easy to read.
    Cons:
  • Principle is inferred.
pub fn colour(&self, is_ai_generated: bool, has_review: bool, tag: Option<&TagBasic>) -> Colour {
    let tag_or = |default| tag.and_then(|t| t.colour).unwrap_or(default);

    if self.is_proposed_update() {
        return tag_or(Colour::PROPOSED_UPDATE);
    }

    if self.is_sensitive_content() {
        return tag_or(Colour::SENSITIVE_DEFAULT);
    }

    if self.is_regular_manual(is_ai_generated) {
        return tag_or(Colour::HIGHLIGHT_DEFAULT);
    }

    if is_ai_generated {
        return if has_review {
            tag.map(|t| t.colour.unwrap_or(Colour::HIGHLIGHT_DEFAULT))
                .expect("AI highlights are always applied to tags")
        } else {
            // An unvalidated AI-generated highlight.
            Colour::AI_RESULT
        };
    }

    unreachable!("encountered a highlight kind without a specified colour")
}

B

Pros:

  • Principle is declarative.
    Cons:
  • Hard to read.
pub fn colour(&self, is_ai_generated: bool, has_review: bool, tag: Option<&Tag>) -> Colour {
    enum Origin {
        Human(Colour),
        AI(Colour),
    }

    // Define default fallback colours for every highlight kind:
    let origin = if self.is_proposed_update() {
        Origin::Human(Colour::PROPOSED_UPDATE)
    } else if self.is_sensitive_content() {
        Origin::Human(Colour::SENSITIVE_DEFAULT)
    } else if self.is_regular_manual(is_ai_generated) {
        Origin::Human(Colour::HIGHLIGHT_DEFAULT)
    } else if is_ai_generated {
        Origin::AI(Colour::AI_RESULT)
    } else {
        unreachable!("encountered a highlight kind without a specified default colour")
    };

    match origin {
        // Human-generated highlights get their tag colour, if they have a
        // tag, or fallback to their associated default:
        Origin::Human(default) => tag.and_then(|t| t.colour).unwrap_or(default),

        // AI-generated highlights get their tag colour, if they have
        // been validated, or fallback to their associated default:
        Origin::AI(default) => match has_review {
            true => tag
                .map(|t|
                    // NOTE: the default of a tag without a specified colour
                    //       is the highlight default.
                    t.colour.unwrap_or(Colour::HIGHLIGHT_DEFAULT))
                .expect("validated AI highlight missing tag"),
            false => default,
        },
    }
}
2

I think B is much better. The only thing I would definitely change is use if instead of the boolean match.

Another thing you could try is match (origin, has_review) which puts all three branches on the same level.

I think that last map is a bit overloaded. It's always awkward to read such method chains when the middle closure has no interaction with the final expect. So I would break it into two lines, with the expect first.

1 Like
3

Great tips! I think I found a nicer way to express the has_validation branch as well.

Now B reads rather beautifully, and I’m somewhat confident that it’s an improvement over the approach taken in A. Thank you!

B2

pub fn colour(&self, is_ai_generated: bool, has_review: bool, tag: Option<&Tag>) -> Colour {
    enum Origin {
        Human(Colour),
        AI(Colour),
    }

    // Define default fallback colours for every highlight kind:
    let origin = if self.is_proposed_update() {
        Origin::Human(Colour::PROPOSED_UPDATE)
    } else if self.is_sensitive_content() {
        Origin::Human(Colour::SENSITIVE_DEFAULT)
    } else if self.is_regular_manual(is_ai_generated) {
        Origin::Human(Colour::HIGHLIGHT_DEFAULT)
    } else if is_ai_generated {
        Origin::AI(Colour::AI_RESULT)
    } else {
        unreachable!("encountered a highlight kind without a specified default colour")
    };

    match origin {
        // Human-generated highlights get their tag colour, if they have a
        // tag, or fallback to their associated default:
        Origin::Human(default) => tag.and_then(|t| t.colour).unwrap_or(default),

        // Validated AI-generated highlights get their tag colour:
        Origin::AI(_) if has_review => tag
            .expect("AI-generated highlight missing tag")
            .colour
            .unwrap_or(Colour::HIGHLIGHT_DEFAULT),

        // Unvalidated (open) AI-generated highlights get their kind's
        // default colour:
        Origin::AI(default) => default,
    }
}
1 Like