Help refactoring HTML string building function

[mihsamusev]

Hi everyone, i'm new to Rust. I was solving an exercise where you need to create a builder for HTML strings. Although the code presented below does the job, but i'm not sure whether to_indented_string function can be refactored to be more idiomatic Rust? Also, the compiler asked me to specify lifetimes, i pleased it, however, and i have no idea whether i have done it in a meaningful way. My best guess is that it has something to do with a promise that the members of HtmlElement wont live longer than a HtmlElement object itself, that's why the structs are promised to live for the time 'a same as name, text, elements. Can you please review this code, thank you.

const INDENT_SIZE: usize = 2;

struct HtmlElement<'a> {
    name: &'a str,
    text: &'a str,
    elements: Vec<HtmlElement<'a>>,
}

impl<'a> HtmlElement<'a> {
    fn new(name: &'a str, text: &'a str) -> HtmlElement<'a> {
        return HtmlElement{
            name: name,
            text: text,
            elements: Vec::<HtmlElement>::new(),
        }
    }

    fn to_indented_string(&self, indent_lvl: usize) -> String {
        let mut result = " ".repeat(INDENT_SIZE * indent_lvl);
        result.push_str(
            format!("<{}>\n", self.name).as_str());

        if self.text.len() > 0 {
            result.push_str(
                " ".repeat(INDENT_SIZE * (indent_lvl + 1)).as_str());
            result.push_str(
                format!("<{}>\n", self.text).as_str())
        }

        for e in self.elements.iter() {
            result.push_str(
                e.to_indented_string(indent_lvl + 1).as_str())
        }

        result.push_str(
            " ".repeat(INDENT_SIZE * indent_lvl).as_str());
        result.push_str(
            format!("</{}>\n", self.name).as_str());
        return result;
    }

    fn to_string(&self) -> String {
        return self.to_indented_string(0);
    }
}

struct HtmlBuilder<'a> {
    root: HtmlElement<'a>,
}

impl<'a> HtmlBuilder<'a> {
    fn new(root_name: &'a str) -> Self {
        let root = HtmlElement::new(root_name, "");
        return HtmlBuilder{root: root};
    }

    fn add_child(mut self, name: &'a str, text: &'a str) -> Self {
        let child = HtmlElement::new(name, text);
        self.root.elements.push(child);
        return self;
    }

    fn to_string(&self) -> String {
        return self.root.to_string();
    }
}

fn main() {
    let builder: HtmlBuilder = HtmlBuilder::new("root")
        .add_child("tag1", "text1")
        .add_child("tag2", "text2");

    println!("{}", builder.to_string());
}

Expected result:

<root>
  <tag1>
    <text1>
  </tag1>
  <tag2>
    <text2>
  </tag2>
</root>

[Hyeonu]

Current code allocates lots of intermediate buffers, copy it into another buffer and drop it. It's not necessarily bad, but you can optimize it for performance.

    fn to_indented_string(&self, buf: &mut String, indent_lvl: usize) -> std::fmt::Result {
        use std::fmt::Write;
        
        writeln!(buf, "{:indent$}<{}>", "", self.name, indent = INDENT_SIZE * indent_lvl)?;
        
        if self.text.len() > 0 {
            writeln!(buf, "{:indent$}<{}>", "", self.text, indent = INDENT_SIZE * (indent_lvl + 1))?;
        }

        for e in self.elements.iter() {
            e.to_indented_string(buf, indent_lvl + 1)?;
        }

        writeln!(buf, "{:indent$}</{}>", "", self.name, indent = INDENT_SIZE * indent_lvl)?;
        
        Ok(())
    }

    fn to_string(&self) -> String {
        let mut buf = String::new();
        self.to_indented_string(&mut buf, 0).unwrap();
        buf
    }

Rust Playground

[mihsamusev]

Yeah, that was my concern, allocating to many intermediate strings, thanks!

[H2CO3]

Comments:

• for e in self.elements.iter() is unnecessary since references to containers implement IntoIterator. You can just write for e in &self.elements.
• It's not elegant to push_str(format!(…)), because it allocates needlessly. Use write!()/writeln!() instead.
• For the same reason, it would be better if to_indented_string() didn't return an owned String, because then the recursion needs to allocate, too. You should append to a string through a mutable reference instead.
• Explicit return at the end of the function is not needed. Rust is an expression language, the last expression in the function will be its return value.
• You should implement to_string() using the ToString trait, not as an inherent method. Better yet, implement Display and let the blanket impl handle it.

Here's the updated code. I reproduce the most important part, the to_string_indented() method below:

fn to_indented_string<W: Write>(&self, indent_lvl: usize, result: &mut W) -> Result {
    Self::write_indent(indent_lvl, &mut *result)?;
    writeln!(result, "<{}>", self.name)?;

    if !self.text.is_empty() {
        Self::write_indent(indent_lvl + 1, &mut *result)?;
        writeln!(result, "<{}>", self.text)?;
    }

    for e in &self.elements {
        e.to_indented_string(indent_lvl + 1, &mut *result)?;
    }

    Self::write_indent(indent_lvl, &mut *result)?;
    writeln!(result, "</{}>", self.name)
}

[mihsamusev]

Thank, great points i never though about!

[kornel]

Don't think of 'a as "lives for 'a lifetime", because temporarily borrowed references are not in control of their lifetime. It's better to think of it as "it's borrowed from somewhere else, which can be found by looking where the 'a came from".

In your case use of temporary non-owning views into external strings (&str) in the struct is probably a mistake. This will be horrendously painful to use in any real-world case (that doesn't rely on the special case of &'static str hardcoded into the program), because the first element you create will freeze the source of the strings as the only one you can use, and also require it to be shared+immutable. Without an extra abstraction like arena, you won't even be able to create a second element. And even with an arena, you'll likely run into self-referential struct problem.

Until you think Rust is easy, don't use references in structs at all. Treat struct …<'a> as a red flag.

Use String in structs. Use &str in function arguments.

[mihsamusev]

Okay, i see, the compiler didnt need me to specify lifetimes anymore if i change struct fields to String. I will try to avoid struct …<'a> until im more experienced, thanks.