3
\$\begingroup\$

Simplified from https://github.com/132nd-vWing/tacview-splitter.

External dependencies: zip crate

The code would do the following:

  1. read a file from disk (determined at runtime)
  2. analyze the content, split it into a header and a body
  3. divide the body into two categories (here blue and red)
  4. for each category, write the header and the corresponding body to a separate file
  5. the output file type depends on the input file type.
    1. if the input was a txt file, write the output to a txt file
    2. otherwise the input was a txt file contained in a zip file. Write the data as a txt in a zip.

Code is this:

use std::fs;
use std::io::Write;

struct Descriptors<T: Write> {
    blue: T,
    red: T,
}

struct OutputFilenames {
    pub txt: FilenamesVariant,
    pub zip: FilenamesVariant,
}

struct FilenamesVariant {
    pub blue: String,
    pub red: String,
}

impl Descriptors<zip::ZipWriter<fs::File>> {
    pub fn new(filenames: OutputFilenames) -> Descriptors<zip::ZipWriter<fs::File>> {
        let options =
            zip::write::FileOptions::default().compression_method(zip::CompressionMethod::Deflated);

        let file = fs::File::create(&filenames.zip.blue).unwrap();
        let mut blue = zip::ZipWriter::new(file);
        blue.start_file(&filenames.txt.blue, options).unwrap();

        let file = fs::File::create(&filenames.zip.red).unwrap();
        let mut red = zip::ZipWriter::new(file);
        red.start_file(&filenames.txt.red, options).unwrap();

        Descriptors { blue, red }
    }
}

impl Descriptors<fs::File> {
    pub fn new(filenames: OutputFilenames) -> Descriptors<fs::File> {
        let blue = fs::File::create(&filenames.txt.blue).unwrap();
        let red = fs::File::create(&filenames.txt.red).unwrap();
        Descriptors { blue, red }
    }
}

impl<T: Write> Descriptors<T> {
    fn write(&mut self, header: Vec<String>, blue: Vec<String>, red: Vec<String>) {
        for line in &header {
            writeln!(self.blue, "{}", line).unwrap();
            writeln!(self.red, "{}", line).unwrap();
        }
        for line in &blue {
            writeln!(self.blue, "{}", line).unwrap();
        }
        for line in &red {
            writeln!(self.red, "{}", line).unwrap();
        }
    }
}

impl OutputFilenames {
    fn default() -> Self {
        let txt = FilenamesVariant {
            blue: "blue.txt".to_string(),
            red: "red.txt".to_string(),
        };
        let zip = FilenamesVariant {
            blue: "blue.zip".to_string(),
            red: "red.zip".to_string(),
        };
        Self { txt, zip }
    }
}

fn main() {
    let is_zip = true; // in practice this would be read from disk
    let output_filenames = OutputFilenames::default();

    let header = vec!["some header".to_string(), "headers".to_string()];

    let blue_content = vec!["blue content".to_string(), "blue".to_string()];
    let red_content = vec!["red content".to_string(), "red".to_string()];

    if is_zip {
        let mut descriptors = Descriptors::<zip::ZipWriter<fs::File>>::new(output_filenames);
        descriptors.write(header, blue_content, red_content);
    } else {
        let mut descriptors = Descriptors::<fs::File>::new(output_filenames);
        descriptors.write(header, blue_content, red_content);
    }
}

I tried to write the code in manner that would avoid code repetition, by implementing it in a Descriptor<T: Write> struct. This struct holds the descriptors independent of the output file type. Then in the main code, I can simple write to the descriptors without having to care for the exact type.

Is it possible to write the code in a more generic / concise / idiomatic manner?

What else would you change about this code and why?

\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

I think it's quite good, and I only have a single nitpick:

  • use &[T] where T: AsRef<str> instead of Vec<String> for your function arguments (that's a general rule, use the least specific possible type in your arguments for maximum compatibility)

Everything else is honestly opinion based, like:

  • I would probably not handle blue and red as two separate things, because all of their code is duplicated. They can be handled as the same thing.
  • I'd employ Box to become even more type-agnostic and store things only by their trait
  • I don't see a big advantage of having all those structs, I think for this case simple functions are more suited

Something like this:

use std::fs::File;
use std::io::{self, Write};

use zip::{write::FileOptions, CompressionMethod, ZipWriter};

fn create_filewriter(filename: &str) -> File {
    File::create(filename).unwrap()
}

fn create_zipwriter(filename: &str, nested_filename: &str) -> ZipWriter<File> {
    let mut writer = ZipWriter::new(create_filewriter(filename));
    writer
        .start_file(
            nested_filename,
            FileOptions::default().compression_method(CompressionMethod::Deflated),
        )
        .unwrap();
    writer
}

fn create_writer(zip: bool, name: &str) -> Box<dyn Write> {
    if zip {
        Box::new(create_zipwriter(
            &format!("{name}.zip"),
            &format!("{name}.txt"),
        ))
    } else {
        Box::new(create_filewriter(&format!("{name}.txt")))
    }
}

trait StringsWriter {
    fn write_strings<S: AsRef<str>>(&mut self, lines: &[S]) -> Result<(), io::Error>;
}

impl<T> StringsWriter for T
where
    T: Write,
{
    fn write_strings<S: AsRef<str>>(&mut self, lines: &[S]) -> Result<(), io::Error> {
        for line in lines {
            writeln!(self, "{}", line.as_ref())?;
        }
        Ok(())
    }
}

fn main() {
    let is_zip = true; // in practice this would be read from disk

    let header = vec!["some header".to_string(), "headers".to_string()];

    let blue_content = vec!["blue content".to_string(), "blue".to_string()];
    let red_content = vec!["red content".to_string(), "red".to_string()];

    let mut blue_writer = create_writer(is_zip, "blue");
    let mut red_writer = create_writer(is_zip, "red");

    blue_writer.write_strings(&header).unwrap();
    blue_writer.write_strings(&blue_content).unwrap();

    red_writer.write_strings(&header).unwrap();
    red_writer.write_strings(&red_content).unwrap();
}

Again, this is mostly personal preference and I think your solution was fine as-is. I just thought I'd give you another perspective on how else this could be implemented.

The next step for me would be error handling. Get rid of all the unwrap() and expect(), write a proper error enum with thiserror and propagate it through. Maybe add miette for bling.

I'm unsure, though, how well those tips translate to the tacview-splitter repository.

\$\endgroup\$
6
  • \$\begingroup\$ That is the type of input I was looking for. Thank you very much! Especially with regards to Box and AsRef. \$\endgroup\$
    – sarema
    Aug 11 at 11:13
  • \$\begingroup\$ I'll let this sit a couple more days if someone else feels inclined to write something, otherwise I will accept you answer as correct. \$\endgroup\$
    – sarema
    Aug 11 at 11:14
  • \$\begingroup\$ Just trying to understand all you've written. I found that fn write_strings<S: Display>(...) also works. Are there any tradeoffs to consider when choosing between the different Traits? \$\endgroup\$
    – sarema
    Aug 11 at 12:37
  • 1
    \$\begingroup\$ It depends what you intend. Your intention should reflect in the purpose of the chosen trait. AsRef<str> is everything that can be referenced to a &str, meaning, &String and &str. Display is intended for something else. Yes, the outcome is also a string, but Display means all types that can be presented to a human in a human readable format. Yes, you could argue that writeln! means you want the content of your file to be explicitely human readable, and then yes, Display would probably be the correct choice. But that means that you can, for example also pass a &f32 into it. \$\endgroup\$
    – Finomnis
    Aug 11 at 19:04
  • 1
    \$\begingroup\$ If that is your intention, that this function shall accept everything that is displayable, then yes, use that trait :) I just thought you explicitely want to take string-like objects. And string-like objects are usually passed via &str. But &[&str] does not work with Vec<String>, because the content would have to be converted to &str first. That's why in this case, I'd use the AsRef<str> instead. If it wasn't wrapped in a container, I'd definitely do &str instead. \$\endgroup\$
    – Finomnis
    Aug 11 at 19:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.