Struct, Traits, Enums - How did I do it?

code review
1

In order to learn Rust, I'm trying to convert an existing C# application to Rust.
The project is creating and writing a (very limited) Adobe PDF document.
Currently, I've just finished writing the PDF Header and signature, but I want to know the opinions of you guys and girls, to ensure that I'm on the right track on creating something stable and speedy.
Any remarks on how to improve the design or how to reduce the memory allocations are highly appreciated.

First, I have the pdf module itself:

//! High-level API for generating Adobe PDF documents.
mod document;
mod version;
mod writer;

use writer::Writer;

pub use document::Document;
pub use version::Version;
pub use writer::BufferedWriter;

Let's get started with the Writer trait.
I decided to use a trait here so that in tests I can provide a custom implementation to verify that failures are properly handled.

//! Implementation of a writer for writing Adobe PDF documents.
use std::io::{BufWriter, Error, Write};

/// The API for writing the data of an Adobe PDF document.
pub trait Writer<W: Write> {
    /// Write data to the underlying writer (prefixed with a `%` character).
    fn write_as_comment(&mut self, data: &[u8]) -> Result<(), Error>;

    /// Write data to the underlying writer (prefixed with a `%` character) and append a newline character.
    /// NOTE: This should always be a Windows newline character (Carriage Return followed by a Line Feed - `\r\n`).
    fn write_as_comment_with_newline(&mut self, data: &[u8]) -> Result<(), Error>;
}

/// A buffered PDF document writer.
pub struct BufferedWriter<W: Write> {
    inner: BufWriter<W>,
}

//# Implement the `Writer<W: Write>` trait for the `BufferedWriter<W: Write>` struct.
impl<W: Write> Writer<W> for BufferedWriter<W> {
    fn write_as_comment(&mut self, data: &[u8]) -> Result<(), Error> {
        if let Err(err) = self.inner.write_all(b"%") {
            return Err(err);
        };

        self.inner.write_all(data)
    }

    fn write_as_comment_with_newline(&mut self, data: &[u8]) -> Result<(), Error> {
        if let Err(err) = self.write_as_comment(data) {
            return Err(err);
        }

        self.inner.write_all(b"\r\n")
    }
}

//# The implementation of the `BufferedWriter<W: Write>` struct.
impl<W: Write> BufferedWriter<W> {
    pub(crate) fn new(writer: W) -> Self {
        Self {
            inner: BufWriter::new(writer),
        }
    }
}

// UT's: Ensure that the code in this file correctly implemented.
#[cfg(test)]
mod unit_tests {
    use super::*;

    //# Create UT's to ensure that `Writer<W: Write>::write_as_comment()` writes the  correct data to the given
    //# writer.
    macro_rules! write_as_comment_tests {
        ($($name:ident: $data:expr,)*) => {
        $(
            #[test]
            fn $name() {
                // ARRANGE.
                let mut writer_bytes: Vec<u8> = Vec::new();
                {
                    let mut writer = BufferedWriter::new(&mut writer_bytes);

                    // ACT.
                    writer.write_as_comment($data).unwrap();
                }

                // ASSERT.
                assert_eq!(writer_bytes[0], b'%');
                assert_eq!(writer_bytes[1..$data.len() + 1].as_ref(), $data)
            }
        )*
        }
    }

    //# Create UT's to ensure that `BufferedWriter<W: Write>::write_as_comment_with_newline()` writes the correct data
    //# to the given writer.
    macro_rules! write_as_comment_with_newline_tests {
        ($($name:ident: $data:expr,)*) => {
        $(
            #[test]
            fn $name() {
                // ARRANGE.
                let mut writer_bytes: Vec<u8> = Vec::new();
                {
                    let mut writer = BufferedWriter::new(&mut writer_bytes);

                    // ACT.
                    writer.write_as_comment_with_newline($data).unwrap();
                }

                // ASSERT.
                assert_eq!(writer_bytes[0], b'%');
                assert_eq!(writer_bytes[1..$data.len() + 1].as_ref(), $data);
                assert_eq!(writer_bytes[$data.len() + 1..$data.len() + 3].as_ref(), b"\r\n")
            }
        )*
        }
    }

    //# UT's: Ensure that `BufferedWriter<W: Write>::write_as_comment()` writes the  correct data to the given writer.
    write_as_comment_tests! {
        write_as_comment_tests_example_1: b"Example 1.",
        write_as_comment_tests_example_2: b"Example 2.",
    }

    //# UT's: Ensure that `Writer<W: Write>::write_as_comment_with_newline()` writes the correct data to the given
    //#       writer.
    write_as_comment_with_newline_tests! {
        write_as_comment_with_newline_tests_example_1: b"Example 1.",
        write_as_comment_with_newline_tests_example_2: b"Example 2.",
    }
}

Let's move to the enumeration which represents the different Adobe PDF versions.

//! Implementation of the different Adobe PDF versions.

/// The different versions of the Adobe PDF specification.
#[derive(PartialEq, Debug)]
pub enum Version {
    V14, // Represents Adobe PDF version 1.4
    V15, // Represents Adobe PDF version 1.5
    V16, // Represents Adobe PDF version 1.6
    V17, // Represents Adobe PDF version 1.7
    V20, // Represents Adobe PDF version 2.0
    V21, // Represents Adobe PDF version 2.1
}

//# Implement the `Default` trait for the `Version` struct.
impl Default for Version {
    fn default() -> Self {
        Self::V14
    }
}

//# The implementation of the `Version` enumeration.
impl<'a> Version {
    /// Return the version as a byte slice.
    pub fn as_bytes(&self) -> &'a [u8; 7] {
        match self {
            Version::V14 => b"PDF-1.4",
            Version::V15 => b"PDF-1.5",
            Version::V16 => b"PDF-1.6",
            Version::V17 => b"PDF-1.7",
            Version::V20 => b"PDF-2.0",
            Version::V21 => b"PDF-2.1",
        }
    }
}

// UT's: Ensure that the code in this file correctly implemented.
#[cfg(test)]
mod unit_tests {
    use super::*;

    //# Create UT's to ensure that `Version::as_bytes()` returns a slice of bytes representing the version.
    macro_rules! as_bytes_for_version_tests {
        ($($name:ident: ($input:expr, $output:expr),)*) => {
        $(
            #[test]
            fn $name() {
                // ACT.
                let bytes = $input.as_bytes();

                // ASSERT.
                assert_eq!(bytes, $output)
            }
        )*
        }
    }

    //# UT: Ensure that `Version::default()` returns the default version.
    #[test]
    fn test_default_trait_implementation_for_version() {
        // ACT.
        let version = Version::default();

        // ASSERT.
        assert_eq!(version, Version::V14)
    }

    //# UT's: Ensure that `Version::as_bytes()` returns a slice of bytes representing the version.
    as_bytes_for_version_tests! {
        version_v14: (Version::V14, b"PDF-1.4"),
        version_v15: (Version::V15, b"PDF-1.5"),
        version_v16: (Version::V16, b"PDF-1.6"),
        version_v17: (Version::V17, b"PDF-1.7"),
        version_v20: (Version::V20, b"PDF-2.0"),
        version_v21: (Version::V21, b"PDF-2.1"),
    }
}

And lastly, the Adobe PDF document itself.

//! Implementation of an Adobe PDF document.
use std::io::{Error, Write};

use super::Version;

// The signature of an Adobe PDF file.
const PDF_SIGNATURE_BYTES: [u8; 5] = [128, 129, 130, 131, 132];

/// An Adobe PDF document.
pub struct Document {
    version: Version,
}

//# Implement the `Default` trait for the `Document` struct.
impl Default for Document {
    fn default() -> Self {
        Self {
            version: super::Version::default(),
        }
    }
}

//# The implementation of the `Document` struct.
impl Document {
    /// Write the document to the given writer.
    pub fn write<W: Write>(&self, mut writer: impl super::Writer<W>) -> Result<(), Error> {
        self.write_header(&mut writer)
    }

    /// Write the PDF header to the given writer.
    fn write_header<W: Write>(&self, writer: &mut impl super::Writer<W>) -> Result<(), Error> {
        if let Err(err) = writer.write_as_comment_with_newline(self.version.as_bytes()) {
            return Err(err);
        }

        writer.write_as_comment_with_newline(&PDF_SIGNATURE_BYTES)
    }
}

// UT's: Ensure that the code in this file correctly implemented.
#[cfg(test)]
mod unit_tests {
    use super::*;

    //# Create UT's to ensure that `Document::write()` writes the correct data to the given writer.
    macro_rules! write_for_empty_document_tests {
        ($($name:ident: $document:expr,)*) => {
        $(
            #[test]
            fn $name() {
                // ARRANGE.
                let mut pdf_bytes = Vec::new();
                let writer = super::super::BufferedWriter::new(&mut pdf_bytes);

                // ACT.
                $document.write(writer).unwrap();

                // ASSERT.
                assert_eq!(pdf_bytes[0], b'%');
                assert_eq!(pdf_bytes[1..8].as_ref(), $document.version.as_bytes());
                assert_eq!(pdf_bytes[8..11].as_ref(), b"\r\n%");
                assert_eq!(pdf_bytes[11..16].as_ref(), PDF_SIGNATURE_BYTES);
                assert_eq!(pdf_bytes[16..18].as_ref(), b"\r\n")
            }
        )*
        }
    }

    //# UT: Ensure that `Document::default()` returns the default document.
    #[test]
    fn test_default_trait_implementation_for_document() {
        // ACT.
        let document = Document::default();

        // ASSERT.
        assert_eq!(document.version, Version::default())
    }

    //# UT's: Ensure that `Document::write()` writes the correct data to the given writer.
    write_for_empty_document_tests! {
        write_empty_v14_document: (Document {
            version: Version::V14,
        }),

        write_empty_v15_document: (Document {
            version: Version::V15,
        }),

        write_empty_v16_document: (Document {
            version: Version::V16,
        }),

        write_empty_v17_document: (Document {
            version: Version::V17,
        }),

        write_empty_v20_document: (Document {
            version: Version::V20,
        }),

        write_empty_v21_document: (Document {
            version: Version::V21,
        }),
    }
}
2

I would recommend making this enum #[non_exhaustive] so you can support new versions in the future without breaking changes.

It's a bit weird to have generic parameters of a specific function declared on an impl block instead of the function itself. Instead, do this:

impl Version {
    /// Return the version as a byte slice.
    pub fn as_bytes<'a>(&self) -> &'a [u8; 7] {

or this, it's equivalent:

impl Version {
    /// Return the version as a byte slice.
    pub fn as_bytes(&self) -> &'static [u8; 7] {

I'm not sure about the use of the Writer trait. Couldn't you just pass a custom implementation of std's Write trait from within tests? Is there any reason you need to define your own trait?

1 Like
3

Thanks for the suggestions.
I decided to use a custom Write trait for the following reasons:

If want to have the following functions defined on the writer:

  • Write comments
  • Write comments and append a newline at the end.

This could be achieved with a struct, but then there's NO way to provide a custom implementation in tests (to verify the behavior).
So for that reason I decided to pick a trait, because this allows me to define these functions and provide an own implementation in tests.

If I take your suggestion and use the standard Writer interface, I need to implement all these functions defines in this trait (which is more that I would use, and I believe that this makes the code more confusing for readers).

Anything I'm missing?

4

Can't you just write into a Vec and then later check whether the Vec contains the % and \n signs you expected?

Aren't they implemented already inside BufferedWriter? I'm not really sure what you mean here.

5

If I do understand you correctly, you expect the Document struct to be designed as following:

//# The implementation of the `Document` struct.
impl Document {
    /// Write the document to the given writer.
    pub fn write(&self, writer: impl Write) -> Result<(), Error> {
        self.write_header(&mut writer)
    }

    /// Write the PDF header to the given writer.
    fn write_header(&self, writer: impl Write) -> Result<(), Error> {
        if let Err(err) = writer.write_as_comment_with_newline(self.version.as_bytes()) {
            return Err(err);
        }

        writer.write_as_comment_with_newline(&PDF_SIGNATURE_BYTES)
    }
}

Please correct my if I'm wrong.

But that code won't compile since the functions write_as_comment_with_newline is NOT defined on the Write trait. Off course I can define the function write_as_comment_with_newline on the Document struct, accepting a Writer trait implementation, but I do believe that It would make my core more cluttered, since a Document shouldn't know how it's saved, it should only know that it's saved. See S.O.L.I.D. design principles.

6

Unless you have an actual use-case where a Document would write to a something that does not use % for comments and \r\n for newlines, making it generic is premature abstraction. If you are concerned about the responsibility of Document you can make write_as_comment and write_as_comment_with_newline free-standing functions. That's usually best practice anyway if you find that a method of a type does not actually use the self parameter.

1 Like
7

Thanks for the clarification and the code review.

8

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.