Execercise in chapter 17.3 can be confusing

[videbar]

• I have checked the latest main branch to see if this has already been fixed
• I have searched existing issues and pull requests for duplicates

URL to the section(s) of the book with this problem:

https://doc.rust-lang.org/book/ch17-03-oo-design-patterns.html#implementing-transitions-as-transformations-into-different-types

Description of the problem:

During chapter 17.3 it is explained how the same system could be implemented first using the state design pattern and then a variation of this pattern that takes advantage of some of rust features. The system being explained is a blogpost that can be in different states: draft, pending review and post.

It is explained how to implement the blogpost using the state design pattern and a simple exercise is left for the reader:

• Add a reject method that changes the post’s state from PendingReview back to Draft.
• Require two calls to approve before the state can be changed to Published.
• Allow users to add text content only when a post is in the Draft state. Hint: have the state object responsible for what might change about the content but not responsible for modifying the Post.

Which is great.

Then it is explained how this pattern can be modified to use rust features and at the end of the chapter it says:

Try the tasks suggested for additional requirements that we mentioned at the start of this section on the blog crate as it is after Listing 17-20 to see what you think about the design of this version of the code.

I think this is a problem because, at least to the best of my knowledge, there's no easy way no implement the second requirement in the new design:

Require two calls to approve before the state can be changed to Published.

I found this post that explains the problems with this exercise and the best solution states:

I think the idea that they are getting at is that you add another type, so that you have PendingTwoReviewsPost and PendingOneReviewPost or the like, and approving the former returns the latter, and approving the latter returns a Post. I.e., encoding the state into the types.

which seems to be the only way to solve the problem without modifying how the API is called (which I think is the intention). If this is indeed the intended solution, it's awful, if I wanted to change the number of required approvals from 2 to let's say 5, instead of changing a parameter in the script, I would need to define 3 new structs!

Suggested fix:

Replace the line:

Try the tasks suggested for additional requirements that we mentioned at the start of this section on the blog crate as it is after Listing 17-20 to see what you think about the design of this version of the code.

with something like:

This design has other problems however, if you try the tasks suggested for additional requirements, you will realize that there is no easy way to change the number of approvals required for a post to change from PendingReview to Post.

[eddieantonio]

I think "not useful" is a bit harsh. I think this patterns merits a discussion about the tradeoffs of the OO state pattern versus this pattern of consuming the current struct to produce a new struct (is there a good name for this pattern in Rust?!!??)

I am also currently learning Rust and going through ch17 and I'd like to say that the pattern that is being described at the end of ch17 is absolutely brilliant in certain circumstances! For example, imagine you can allocate memory that can only be in one of three states: read-only, read/write, and read/execute (so that you can run code from that memory 😉). As the names imply, you can read in all states, but you can only write in the read/write state, and you can only run code in the read/execute state.

The pattern in the Rust book at the end of ch17 makes it impossible to do something silly like, attempt to write to a read-only area; however, Rust will still allow you to move into the desired state by changing types (note: .into_*() methods consume the input, returning a different type). This is enforced by the type system at compile-time. For example, I want to write a program that writes some code into the allocation, but I need to convert it to read-only, and then convert it into an executable and run the program I wrote, Rust allows me to write this:

let alloc = Allocation::new();

// allocation is initially read-only; let's make it writable so we can write a program into it!
let alloc = alloc.into_writable();
alloc.write_data("xor eax, eax");

// my system won't allow me write AND execute at the same time, so I need to go back to read-only...
let alloc = alloc.into_read_only();

// Okay, we can execute it now
let alloc = alloc.into_executable();
alloc.run_program();

Using the type system this way, writing to read-only memory is impossible and executing writable memory is also impossible. (You can even do nifty things like implement mut traits only on the read-write state type!) Using the OO state pattern, you could make it so that, when you're not in the ReadAndWrite state, .write_data() ignores the read, or perhaps returns some kind of Err(...), or it could even panic. However using types in this way makes these undesirable scenarios impossible, which I think is really darn neat!

[videbar]

I think "not useful" is a bit harsh. I think this patterns merits a discussion about the tradeoffs of the OO state pattern versus this pattern of consuming the current struct to produce a new struct

Agreed, I just edited the title.

I am also currently learning Rust and going through ch17 and I'd like to say that the pattern that is being described at the end of ch17 is absolutely brilliant in certain circumstances!

I don't intend to say that it can't be useful in some scenarios, like the one you're describing. My problem is that it doesn't work if you want to implement a system such that, when you call a certain method, may or may not change states based on some condition.

In your example, you may want that, when you call into_writable(), the memory object only changes from ReadOnly to ReadAndWrite if no other application is reading it. Now you need the method into_writable() to return something which can either be ReadOnly or ReadAndWrite and there's no easy way of writing the signature of such function.

In the book, that's exactly what the exercise is asking you to do by telling you to require two calls to the method approve() before moving from PendingReviewPost to Published. You would like approve() to return either a PendingReviewPost or a Published depending on the number of approvals, but that's not possible.

[jpenna]

I'd like to know the answer for approving only on the second request as well.

Everything I found online is awkward, like using a third-party crate, adding another struct between RequestApproval and Post (like ActuallyReadyForApproval) or extending a trait and using enum...

This is how I solved it using approve() -> Result<Post, PendingApprovalPost>:

#[derive(Debug)] // This was required here and in `Post` to use `Result.unwrap` in `main.rs`
pub struct PendingReviewPost {
    content: String,
    approval_count: i8,
}

impl PendingReviewPost {
    // -- snip

    pub fn approve(mut self) -> Result<Post, Self> {
        self.approval_count += 1;

        if self.approval_count < 2 {
            return Err(self);
        }

        Ok(Post {
            content: self.content,
        })
    }
}

Then in main.rs:

fn main() {
    // -- snip --

    let post = post.request_review();

    let approve_result = post.approve();

    let approve_result = approve_result.unwrap_err().approve();

    let post = approve_result.unwrap();

    assert_eq!(post.content(), "I am a text");
}

[digitalclover]

This thread seemed to be the most appropriate place for me to add my perspective. As the book suggested, I was thinking in the back of my head why an enum wasn't being used. The text suggested "try it and compare the end result", so I did.

enum PostState {
    Draft,
    PendingReview,
    Published,
}

pub struct Post {
    state: PostState,
    content: String,
}

impl Post {
    pub fn new() -> Post {
        Post {
            state: PostState::Draft,
            content: String::new(),
        }
    }

    pub fn add_text(&mut self, text: &str) {
        self.content.push_str(text);
    }

    pub fn content(&self) -> &str {
        match self.state {
            PostState::Published => self.content.as_str(),
            _ => "",
        }
    }

    pub fn request_review(&mut self) {
        if let PostState::Draft = self.state {
            self.state = PostState::PendingReview;
        }
    }

    pub fn approve(&mut self) {
        if let PostState::PendingReview = self.state {
            self.state = PostState::Published;
        }
    }
}

I'm still relatively new to Rust, however I personally prefer this approach compared to the struct strategy. The "Why not an enum?" snippet suggested that you must account for all branches of an enum, but I didn't find that to be case. This is because the majority of these methods have a default behavior (return an empty string or a unit) unless we're in a particular state.

I also found this strategy allowed for easy extension of the added requirements:

• Add a reject method
• Require two approvals
• Only allow mutable content during the Draft state

The following is my interpretation of these requirements again using enums.

enum PostState {
    Draft,
    PendingReview { approvals: u8 },
    Published,
}

pub struct Post {
    state: PostState,
    content: String,
}

impl Post {
    pub fn new() -> Post {
        Post {
            state: PostState::Draft,
            content: String::new(),
        }
    }

    pub fn add_text(&mut self, text: &str) {
        if let PostState::Draft = self.state {
            self.content.push_str(text);
        }
    }

    pub fn content(&self) -> &str {
        match self.state {
            PostState::Published => self.content.as_str(),
            _ => "",
        }
    }

    pub fn request_review(&mut self) {
        if let PostState::Draft = self.state {
            self.state = PostState::PendingReview { approvals: 0 };
        }
    }

    pub fn approve(&mut self) {
        if let PostState::PendingReview { mut approvals } = self.state {
            approvals += 1;
            if approvals >= 2 {
                self.state = PostState::Published;
            } else {
                self.state = PostState::PendingReview { approvals }
            }
        }
    }

    pub fn reject(&mut self) {
        if let PostState::PendingReview { .. } = self.state {
            self.state = PostState::Draft;
        }
    }
}

I extracted these two versions into their own module, and had my main() function run assertions on both modules which passed.

pub mod post_v1;
pub mod post_v2;

fn test_v1() {
    let mut post = post_v1::Post::new();

    post.add_text("I ate one salad for lunch today. ");
    assert_eq!("", post.content());

    post.request_review();
    assert_eq!("", post.content());

    post.approve();
    assert_eq!("I ate one salad for lunch today. ", post.content());
}

fn test_v2() {
    let mut post = post_v2::Post::new();

    post.add_text("I ate two salads for lunch today. ");
    assert_eq!("", post.content());

    post.request_review();
    assert_eq!("", post.content());

    post.add_text("Then I ate a hamburger. ");
    assert_eq!("", post.content());

    post.approve();
    assert_eq!("", post.content());

    post.approve();
    assert_eq!("I ate two salads for lunch today. ", post.content());
}

fn main() {
    test_v1();
    test_v2();
}

[digitalclover]

I decided to revisit this to make the interface a bit more aligned with the original state pattern. In this example I've essentially extracted the state definition and the match expression into a separate module. However, it still retains the benefits of using enums for better protection since the compiler will fail if the match expression is missing any variant combination.

post.rs

use crate::state::{ArticleState, ArticleTransition};

#[derive(Default)]
pub struct Post {
    state: ArticleState,
    content: String,
}

impl Post {
    pub fn add_text(&mut self, text: &str) {
        self.content.push_str(text);
    }

    pub fn content(&self) -> &str {
        self.state.content(&self.content)
    }

    pub fn request_review(&mut self) {
        self.state.update_state(ArticleTransition::RequestReview);
    }

    pub fn approve(&mut self) {
        self.state.update_state(ArticleTransition::Approve);
    }
}

state.rs

enum State {
    Draft,
    PendingReview,
    Published,
}

pub enum ArticleTransition {
    RequestReview,
    Approve,
}

pub struct ArticleState {
    state: State,
}

impl Default for ArticleState {
    fn default() -> Self {
        Self {
            state: State::Draft,
        }
    }
}

use ArticleTransition as T;
use State as S;
impl ArticleState {
    pub fn update_state(&mut self, transition: ArticleTransition) {
        match (&self.state, transition) {
            // Handle RequestReview
            (S::Draft, T::RequestReview) => self.state = S::PendingReview,
            (_, T::RequestReview) => (),

            // Handle Approve
            (S::PendingReview, T::Approve) => self.state = S::Published,
            (_, T::Approve) => (),
        }
    }

    pub fn content<'a>(&self, article_post: &'a str) -> &'a str {
        match self.state {
            S::Published => article_post,
            _ => "",
        }
    }
}

I ended up writing an article about this, which shows the same strategy being used in a different scenario.