3
\$\begingroup\$

I am a newbie to Rust and low-level coding. I come from data science and I have always used scripting languages so I may have done weird or useless things (with lifetimes and references).

This is what input look like:

331268137       331268137       8.77e-89        100.000 247     1       123     1       123     123     123
331268137       11467352        1.25e-82        91.057  231     1       123     1       123     123     124
331268137       428214485       3.29e-81        87.805  228     1       123     1       123     123     124
....

Columns 1 and 2 are identifiers; other columns are values describing the relations of identifier. This main objective of this script is to sort col 1 and col 2 and print the result in a temp_dir. But also, if asked, it filters lines that do not match some criteria. You may see that I have an argument of output dir, which is for future development.

extern crate argparse;

use std::io::{self, BufReader, Write, BufWriter};
use std::io::prelude::*;
use std::fs::File;
use std::path::{Path, PathBuf};
use argparse::{ArgumentParser, StoreTrue, Store};
use std::error::Error;
use std::cmp::Ordering;

struct Options{ // options struct for argparse

    infile: String,
    outfile: String,
    cover: f32,
    eval: f64,
    id: f32,
    only_cover: bool,
    numerical_id: bool,
}

fn my_round<'a>(value: f32) -> f32{ // round with 2 trailing car ex 12.456 -> 12.46
    let r = (value * 100.0).round().trunc() / 100.0;
    return r;
}

fn compute_cov<'a>(start: &'a str, end: &'a str, len: &'a str)->  f32{// will compute a coverage based on three &str value
    let start_f = start.parse::<f32>().unwrap();
    let end_f = end.parse::<f32>().unwrap();
    let length_f = len.parse::<f32>().unwrap();
    let result = my_round(((end_f - start_f + 1.0) / length_f) * 100.0);

    result
    }

fn compute_qcov_tcov<'a>(vec:Vec<&'a str>)->( f32, f32){ // sthis function will return qcov and tcov
    let qcov = compute_cov(vec[3], vec[4], vec[7]);
    let tcov = compute_cov(vec[5], vec[6], vec[8]);
    let result = (qcov, tcov);
    result
    }


fn first_pass(options: &Options, temporary_file: &PathBuf, path_in_file: &Path){
        // will read a file sort column one and two 
        // skip line if some option say to (value of a column under a threshold
        // if option cover_only will contract last 6 column into 2. 

        let bool_only_and_filter = options.cover.is_sign_positive() & options.only_cover;
        let filter_pid = options.id.is_sign_positive();
        let filter_evalue = options.eval.is_sign_positive();
        let filter_cov = options.cover.is_sign_positive();
        // /*
        let mut q_s = String::new();
        let mut t_s = String::new();
        let mut s1 = String::new();
        let mut s2 = String::new();
        let mut pid_value:f32;
        let mut eval_value:f64;


        let out_file = match File::create(temporary_file){ // opening in write only 
            Err(why) => panic!("couldn't create {}: {}",
                           temporary_file.display(),
                           why.description()),
            Ok(file) => file,
            };

        let in_file = match File::open(path_in_file){   // opening in read only
            Err(why) => panic!("couldn't open {}: {}",
                           path_in_file.display(),
                           why.description()),
            Ok(file) => file
            };

        let in_file_buffer = BufReader::with_capacity(60000, in_file); //bufering
        let mut out_file_buffer = BufWriter::with_capacity(20000, out_file); //buffering

        'outer: for line in in_file_buffer.lines(){ // lifetime for explicity

            let current_line = line.unwrap();

            let mut v_line:Vec<&str> = current_line.trim() // split line into vector
                                                .split_whitespace()
                                                .collect::<Vec<&str>>(); 

            if v_line[0] == (v_line[1]){ // if id col 1 == id col 2 skip line
                continue 'outer;
            }

            let (mut v_id, mut v_rest) = v_line.split_at_mut(2); // split vector into 2 
            // one contain id
            // other contain numerical info
            // may be better to keep it and use swap instead of sort?

            let mut v_rest: Vec<&str> =  v_rest.iter().map(std::ops::Deref::deref).collect();

            if filter_pid{
                pid_value = v_rest[1].parse::<f32>().unwrap();
                if pid_value < options.id{
                    continue 'outer;                
                }
            }
            if filter_evalue{
                eval_value = v_rest[0].parse::<f64>().unwrap();
                if eval_value > options.eval{
                    continue 'outer;
                }

            }


            if bool_only_and_filter { // skip line that do not meet a threshold and
                                      // rewrite last 6 column into only 2
                {
                    let (cover_q, cover_t) = compute_qcov_tcov(v_rest.clone());
                    if cover_q < options.cover || cover_t < options.cover {
                        continue 'outer;    
                        }


                    q_s =  cover_q.clone().to_string();
                    t_s =  cover_t.clone().to_string();
                }               
                v_rest.truncate(5);
                v_rest[3] =  &q_s;
                v_rest[4] =  &t_s;

            }else if filter_cov{ // skip line that do not meet a threshold

                let (cover_q, cover_t) = compute_qcov_tcov(v_rest.clone());
                if cover_q < options.cover || cover_t < options.cover {
                    continue 'outer;    
                    }

            }

            else if options.only_cover{ // rewrite last 6 column into only 2
                let (cover_q, cover_t) = compute_qcov_tcov(v_rest.clone());
                q_s =  cover_q.clone().to_string();
                t_s =  cover_t.clone().to_string();
                v_rest.truncate(5);
                v_rest[3] =  &q_s;
                v_rest[4] =  &t_s;
            }

            if options.numerical_id{ // col 1 and 2 should be threat as integer
                let id1 = v_id[0].clone().parse::<u32>().unwrap();
                let id2 = v_id[1].clone().parse::<u32>().unwrap();
                if id1.gt(&id2){
                    s1 = id1.clone().to_string();
                    s2 = id2.clone().to_string();
                    v_id[0] = &s2;
                    v_id[1] = &s1;
                }

            }
            else{   // col 1 and two shoul be threat as string
                v_id.sort();
            }

            // join results
            let mut j1 = v_id.join("\t");
            let j2 = v_rest.join("\t");
            j1.push_str("\t");
            j1.push_str(&j2);
            j1.push_str("\n");
            // write results
            out_file_buffer.write_all(j1.as_bytes());
        }

}


fn main() {
    let mut options = Options{infile:"".to_string(), outfile:"".to_string(),
                             cover:-1.0, eval:-1.0, id:-1.0,
                              only_cover:false, numerical_id:false};

    { //scope for argument parsing
    let mut parser = ArgumentParser::new();
    parser.refer(&mut options.infile) // input file
        .add_option(&["-i", "--in"], Store, "input file mandatory")
        .required();
    parser.refer(&mut options.outfile) // output file
        .add_option(&["-o", "--out"], Store, "output file mandatory")
        .required();
    parser.refer(&mut options.cover) //cover option
        .add_option(&["--cover"], Store, "cover option take positive float if filled will
                                         only consider line min(qcov, tcov) > cover(option)
                                         should be done in percentage eg 80.00");
    parser.refer(&mut options.eval) // eval
        .add_option(&["--eval"], Store, "eval option take positive float if filled will
                                         only consider line eval < eval(option)");
    parser.refer(&mut options.id) // pident
        .add_option(&["--pident"], Store, "pident option take positive float if filled will
                                         only consider line pident > pident(option)");
    parser.refer(&mut options.only_cover) // bolean does need to reduce information of aln position?
        .add_option(&["--only_cov"], StoreTrue, "If set the output will only contain
                                                min(qcov, tcov) in place of the aln 
                                                position");
    parser.refer(&mut options.numerical_id)
        .add_option(&["--num_id"],StoreTrue, "are the id numerical");
    parser.parse_args_or_exit();

    }


    let path_in_file = Path::new(&options.infile);
    let path_out_file = Path::new(&options.outfile);
    println!("input file: {}", &path_in_file.display());
    println!("output file: {}", &path_out_file.display());

    // assert the input file exist
    assert_eq!(path_in_file.is_file(), true,
             "the input file {} do not exist or is not a file", &options.infile);

    // assert out_file do not exist:
    assert_eq!(path_out_file.is_file(), false,
             "the output file {} do exist, I won't dare erase it", &options.outfile);

    // get a temporary filename
    let mut cpt = 0;
    let path_to_out_dir = path_out_file.parent().unwrap();
    let mut temporary_file = path_to_out_dir.join(format!("temporary_{value}",value=cpt));
    while temporary_file.exists(){
    cpt += 1;
    temporary_file = path_to_out_dir.join(format!("temporary_{value}",value=cpt));
        }

    println!("{}", &options.infile);
    println!("{}", &options.outfile);
    println!("{}", &temporary_file.display());
    first_pass(&options, &temporary_file, &path_in_file)


}

EDIT: I want to thanks you for those helpful answer and the time you pass to make them there are all clear and constructive.

\$\endgroup\$
1
\$\begingroup\$

Rust is a compiled language. This means that there is a compiler, one that generates warnings and errors. It does not get any simpler than this: the compiler tells you what is wrong; listen to it:

warning: unused import: `self`
 --> src/main.rs:3:15
  |
3 | use std::io::{self, BufReader, Write, BufWriter};
  |               ^^^^
  |
  = note: #[warn(unused_imports)] on by default

warning: unused import: `std::cmp::Ordering`
 --> src/main.rs:9:5
  |
9 | use std::cmp::Ordering;
  |     ^^^^^^^^^^^^^^^^^^

warning: value assigned to `q_s` is never read
  --> src/main.rs:54:13
   |
54 |         let mut q_s = String::new();
   |             ^^^^^^^
   |
   = note: #[warn(unused_assignments)] on by default

warning: value assigned to `t_s` is never read
  --> src/main.rs:55:13
   |
55 |         let mut t_s = String::new();
   |             ^^^^^^^

warning: value assigned to `s1` is never read
  --> src/main.rs:56:13
   |
56 |         let mut s1 = String::new();
   |             ^^^^^^

warning: value assigned to `s2` is never read
  --> src/main.rs:57:13
   |
57 |         let mut s2 = String::new();
   |             ^^^^^^

warning: variable does not need to be mutable
  --> src/main.rs:91:28
   |
91 |             let (mut v_id, mut v_rest) = v_line.split_at_mut(2); // split vector into 2
   |                            ^^^^^^^^^^
   |
   = note: #[warn(unused_mut)] on by default

Especially the last one:

warning: unused `std::result::Result` which must be used
   --> src/main.rs:169:13
    |
169 |             out_file_buffer.write_all(j1.as_bytes());
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: #[warn(unused_must_use)] on by default

Things in programs can fail and failure is bad. Do not neglect to handle an error.

As discussed on the Rust tag info page, use Clippy and Rustfmt. Among other issues, rustfmt highlights these points to me:

  • If you are going to document a struct, use the documentation comment syntax (///). That way rustdoc will pick it up.

  • Spaces around curly braces; -> f32{ should be -> f32 {

  • Rust indentation is 4 spaces. Sometimes you have 8.

Clippy adds a lot more warnings; understand them all:

warning: unneeded return statement
  --> src/main.rs:31:5
   |
31 |     return r;
   |     ^^^^^^^^^ help: remove `return` as shown: `r`
   |
   = note: #[warn(needless_return)] on by default
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#needless_return

warning: returning the result of a let binding from a block. Consider returning the expression directly.
  --> src/main.rs:40:5
   |
40 |     result
   |     ^^^^^^
   |
   = note: #[warn(let_and_return)] on by default
note: this expression can be directly returned
  --> src/main.rs:38:18
   |
38 |     let result = my_round(((end_f - start_f + 1.0) / length_f) * 100.0);
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#let_and_return

warning: returning the result of a let binding from a block. Consider returning the expression directly.
  --> src/main.rs:47:5
   |
47 |     result
   |     ^^^^^^
   |
note: this expression can be directly returned
  --> src/main.rs:46:18
   |
46 |     let result = (qcov, tcov);
   |                  ^^^^^^^^^^^^
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#let_and_return

warning: long literal lacking separators
  --> src/main.rs:88:51
   |
88 |     let in_file_buffer = BufReader::with_capacity(60000, in_file); //bufering
   |                                                   ^^^^^
   |
   = note: #[warn(unreadable_literal)] on by default
   = help: consider: 60_000
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#unreadable_literal

warning: long literal lacking separators
  --> src/main.rs:89:56
   |
89 |     let mut out_file_buffer = BufWriter::with_capacity(20000, out_file); //buffering
   |                                                        ^^^^^
   |
   = help: consider: 20_000
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#unreadable_literal

warning: this lifetime isn't used in the function definition
  --> src/main.rs:29:13
   |
29 | fn my_round<'a>(value: f32) -> f32 {
   |             ^^
   |
   = note: #[warn(unused_lifetimes)] on by default
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#unused_lifetimes

warning: explicit lifetimes given in parameter types where they could be elided
  --> src/main.rs:43:1
   |
43 | / fn compute_qcov_tcov<'a>(vec: Vec<&'a str>) -> (f32, f32) {
44 | |     let qcov = compute_cov(vec[3], vec[4], vec[7]);
45 | |     let tcov = compute_cov(vec[5], vec[6], vec[8]);
46 | |     let result = (qcov, tcov);
47 | |     result
48 | | }
   | |_^
   |
   = note: #[warn(needless_lifetimes)] on by default
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#needless_lifetimes

warning: this argument is passed by value, but not consumed in the function body
  --> src/main.rs:43:31
   |
43 | fn compute_qcov_tcov<'a>(vec: Vec<&'a str>) -> (f32, f32) {
   |                               ^^^^^^^^^^^^ help: consider changing the type to: `&[&'a str]`
   |
   = note: #[warn(needless_pass_by_value)] on by default
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#needless_pass_by_value

warning: variable does not need to be mutable
   --> src/main.rs:105:14
    |
105 |         let (mut v_id, v_rest) = v_line.split_at_mut(2); // split vector into 2
    |              ^^^^^^^^
    |
    = note: #[warn(unused_mut)] on by default

warning: using `clone` on a `Copy` type
   --> src/main.rs:136:23
    |
136 |                 q_s = cover_q.clone().to_string();
    |                       ^^^^^^^^^^^^^^^ help: try removing the `clone` call: `cover_q`
    |
    = note: #[warn(clone_on_copy)] on by default
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#clone_on_copy

warning: using `clone` on a `Copy` type
   --> src/main.rs:137:23
    |
137 |                 t_s = cover_t.clone().to_string();
    |                       ^^^^^^^^^^^^^^^ help: try removing the `clone` call: `cover_t`
    |
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#clone_on_copy

warning: using `clone` on a `Copy` type
   --> src/main.rs:152:19
    |
152 |             q_s = cover_q.clone().to_string();
    |                   ^^^^^^^^^^^^^^^ help: try removing the `clone` call: `cover_q`
    |
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#clone_on_copy

warning: using `clone` on a `Copy` type
   --> src/main.rs:153:19
    |
153 |             t_s = cover_t.clone().to_string();
    |                   ^^^^^^^^^^^^^^^ help: try removing the `clone` call: `cover_t`
    |
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#clone_on_copy

warning: using `clone` on a double-reference; this will copy the reference instead of cloning the inner type
   --> src/main.rs:161:23
    |
161 |             let id1 = v_id[0].clone().parse::<u32>().unwrap();
    |                       ^^^^^^^^^^^^^^^ help: try dereferencing it: `(*v_id[0]).clone()`
    |
    = note: #[warn(clone_double_ref)] on by default
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#clone_double_ref

warning: using `clone` on a double-reference; this will copy the reference instead of cloning the inner type
   --> src/main.rs:162:23
    |
162 |             let id2 = v_id[1].clone().parse::<u32>().unwrap();
    |                       ^^^^^^^^^^^^^^^ help: try dereferencing it: `(*v_id[1]).clone()`
    |
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#clone_double_ref

warning: using `clone` on a `Copy` type
   --> src/main.rs:164:22
    |
164 |                 s1 = id1.clone().to_string();
    |                      ^^^^^^^^^^^ help: try removing the `clone` call: `id1`
    |
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#clone_on_copy

warning: using `clone` on a `Copy` type
   --> src/main.rs:165:22
    |
165 |                 s2 = id2.clone().to_string();
    |                      ^^^^^^^^^^^ help: try removing the `clone` call: `id2`
    |
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#clone_on_copy

warning: this expression borrows a reference that is immediately dereferenced by the compiler
   --> src/main.rs:264:43
    |
264 |     first_pass(&options, &temporary_file, &path_in_file)
    |                                           ^^^^^^^^^^^^^ help: change this to: `path_in_file`
    |
    = note: #[warn(needless_borrow)] on by default
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#needless_borrow

Of note, it saves three actual clone calls (see Why is it discouraged to accept a reference to a String (&String) or Vec (&Vec) as a function argument?, again from the tag info page) and a handful of needless ones.

A noticeable amount of your documentation is basically useless. Foe example, fn compute_qcov_tcov<'a>(vec:Vec<&'a str>)->( f32, f32){ // sthis function will return qcov and tcov — yes, that's what the function name is. The comment adds nothing and just takes up time to read.


Here are my thoughts from reading through it

  1. If you are going to put examples in your documentation, make them executable so it's always correct.

  2. Do not put the type of a variable in the name of the variable (bool_only_and_filter). The compiler will track the type for you just fine.

  3. Do not use & to compute the boolean AND of two values, use &&.

  4. Your matches with panic can be written with Result::unwrap_or_else

  5. Don't specify the type of the collected item if you don't have to; let it be inferred (Vec<_>).

  6. Don't specify the type of the collected item in both the variable declaration (foo: Bar) and in collect (collect::<Bar>).

  7. Don't needlessly wrap variables in parenthesis.

  8. You have some unneeded turbofish operators on your parse calls for the same reason as collect above. You only need it once.

  9. Declare your variables as close as possible to where they are used. Don't define them at the top of the function, this isn't C from 1989. This also allows them to not be mutable.

  10. Don't use the gt method; just use the > symbol

  11. Do not use assert_eq! to test if something is true or false, just use assert!

  12. Your assertions about the if the file exists or not are useless at best. The user could create / delete them between when you tried to check if they are there and when you actually try to open them. This is one of the reasons that File::open / File::create return a Result!.

  13. Likewise, your "temporary filename" function has the same problem. Something can create / delete the file between when you look for it and when you open it. You need to just open the file.

  14. It's better to formulate the temporary file as a loop to avoid writing the joining logic twice. Might as well extract it as a function as well; removing the need for a comment.

  15. If you have a default value for a type, implement Default.

  16. You use negative numbers to denote presence of a value. You should be using Option instead.

  17. While loop labels look a lot like lifetimes, they really should not be thought of as such. You also should avoid using them at all, unless absolutely needed. Having a giant loop that you continue at strange places in the middle isn't made easier to understand with loop labels.

  18. If you have to introduce a scope, try to give it a name and make it a function

extern crate argparse;

use std::io::{BufReader, BufWriter, Write};
use std::io::prelude::*;
use std::fs::File;
use std::path::{Path, PathBuf};
use argparse::{ArgumentParser, Store, StoreTrue};
use std::error::Error;

struct Options {
    infile: String,
    outfile: String,
    cover: f32,
    eval: f64,
    id: f32,
    only_cover: bool,
    numerical_id: bool,
}

impl Default for Options {
    fn default() -> Self {
        Options {
            infile: "".to_string(),
            outfile: "".to_string(),
            cover: -1.0,
            eval: -1.0,
            id: -1.0,
            only_cover: false,
            numerical_id: false,
        }
    }
}

/// Round to 2 decimal points
///
/// ```rust
/// assert_eq!(my_round(12.456), 12.46);
/// ```
fn my_round(value: f32) -> f32 {
    (value * 100.0).round().trunc() / 100.0
}

fn compute_cov(start: &str, end: &str, len: &str) -> f32 {
    let start_f = start.parse::<f32>().unwrap();
    let end_f = end.parse::<f32>().unwrap();
    let length_f = len.parse::<f32>().unwrap();
    my_round(((end_f - start_f + 1.0) / length_f) * 100.0)
}

fn compute_qcov_tcov(vec: &[&str]) -> (f32, f32) {
    let qcov = compute_cov(vec[3], vec[4], vec[7]);
    let tcov = compute_cov(vec[5], vec[6], vec[8]);
    (qcov, tcov)
}


/// Reads a file and sorts by column one and two
///
/// Skip a line if some option says to (value of a column under a threshold)
/// if option `cover_only`, will contract last 6 column into 2.
fn first_pass(options: &Options, temporary_file: &PathBuf, path_in_file: &Path) {
    let only_and_filter = options.cover.is_sign_positive() && options.only_cover;
    let filter_pid = options.id.is_sign_positive();
    let filter_evalue = options.eval.is_sign_positive();
    let filter_cov = options.cover.is_sign_positive();

    let mut q_s;
    let mut t_s;
    let mut s1;
    let mut s2;

    let out_file = File::create(temporary_file).unwrap_or_else(|why| {
        panic!(
            "couldn't create {}: {}",
            temporary_file.display(),
            why.description()
        )
    });

    let in_file = File::open(path_in_file).unwrap_or_else(|why| {
        panic!(
            "couldn't open {}: {}",
            path_in_file.display(),
            why.description()
        )
    });

    let in_file_buffer = BufReader::with_capacity(60_000, in_file);
    let mut out_file_buffer = BufWriter::with_capacity(20_000, out_file);

    for line in in_file_buffer.lines() {
        let current_line = line.unwrap();

        let mut v_line: Vec<_> = current_line.trim().split_whitespace().collect();

        if v_line[0] == v_line[1] {
            continue;
        }

        // may be better to keep it and use swap instead of sort?
        let (v_id, v_rest) = v_line.split_at_mut(2);

        let mut v_rest: Vec<_> = v_rest.iter().map(std::ops::Deref::deref).collect();

        if filter_pid {
            let pid_value: f32 = v_rest[1].parse().unwrap();
            if pid_value < options.id {
                continue;
            }
        }

        if filter_evalue {
            let eval_value: f64 = v_rest[0].parse().unwrap();
            if eval_value > options.eval {
                continue;
            }
        }

        if only_and_filter {
            // skip line that do not meet a threshold and
            // rewrite last 6 column into only 2
            {
                let (cover_q, cover_t) = compute_qcov_tcov(&v_rest);
                if cover_q < options.cover || cover_t < options.cover {
                    continue;
                }

                q_s = cover_q.to_string();
                t_s = cover_t.to_string();
            }
            v_rest.truncate(5);
            v_rest[3] = &q_s;
            v_rest[4] = &t_s;
        } else if filter_cov {
            // skip line that do not meet a threshold

            let (cover_q, cover_t) = compute_qcov_tcov(&v_rest);
            if cover_q < options.cover || cover_t < options.cover {
                continue;
            }
        } else if options.only_cover {
            // rewrite last 6 column into only 2
            let (cover_q, cover_t) = compute_qcov_tcov(&v_rest);
            q_s = cover_q.to_string();
            t_s = cover_t.to_string();
            v_rest.truncate(5);
            v_rest[3] = &q_s;
            v_rest[4] = &t_s;
        }

        if options.numerical_id {
            // col 1 and 2 should be threat as integer
            let id1: u32 = v_id[0].parse().unwrap();
            let id2: u32 = v_id[1].parse().unwrap();

            if id1 > id2 {
                s1 = id1.to_string();
                s2 = id2.to_string();
                v_id[0] = &s2;
                v_id[1] = &s1;
            }
        } else {
            // col 1 and 2 should be treated as a string
            v_id.sort();
        }

        // join results
        let mut j1 = v_id.join("\t");
        let j2 = v_rest.join("\t");
        j1.push_str("\t");
        j1.push_str(&j2);
        j1.push_str("\n");

        // write results
        out_file_buffer
            .write_all(j1.as_bytes())
            .expect("Unable to write");
    }
}

fn main() {
    let options = argument_parsing();

    let path_in_file = Path::new(&options.infile);
    let path_out_file = Path::new(&options.outfile);
    println!("input file: {}", &path_in_file.display());
    println!("output file: {}", &path_out_file.display());

    let temporary_file = get_a_temporary_filename(path_out_file);

    println!("{}", &options.infile);
    println!("{}", &options.outfile);
    println!("{}", &temporary_file.display());
    first_pass(&options, &temporary_file, path_in_file)
}


fn argument_parsing() -> Options {
    let mut options = Options::default();

    {
        let mut parser = ArgumentParser::new();

        parser
            .refer(&mut options.infile)
            .add_option(&["-i", "--in"], Store, "input file mandatory")
            .required();
        parser
            .refer(&mut options.outfile)
            .add_option(&["-o", "--out"], Store, "output file mandatory")
            .required();
        parser.refer(&mut options.cover).add_option(
            &["--cover"],
            Store,
            "cover option take positive float if filled will
                                             only consider line min(qcov, tcov) > cover(option)
                                             should be done in percentage eg 80.00",
        );
        parser.refer(&mut options.eval).add_option(
            &["--eval"],
            Store,
            "eval option take positive float if filled will
                                             only consider line eval < eval(option)",
        );
        parser.refer(&mut options.id).add_option(
            &["--pident"],
            Store,
            "pident option take positive float if filled will
                                             only consider line pident > pident(option)",
        );
        parser.refer(&mut options.only_cover).add_option(
            &["--only_cov"],
            StoreTrue,
            "If set the output will only contain
                                                    min(qcov, tcov) in place of the aln
                                                    position",
        );
        parser.refer(&mut options.numerical_id).add_option(
            &["--num_id"],
            StoreTrue,
            "are the id numerical",
        );
        parser.parse_args_or_exit();
    }

    options
}

fn get_a_temporary_filename(path_out_file: &Path) -> PathBuf {
    let mut cpt = 0;
    let path_to_out_dir = path_out_file.parent().unwrap();
    loop {
        let temporary_file = path_to_out_dir.join(format!("temporary_{value}", value = cpt));
        if !temporary_file.exists() {
            return temporary_file;
        }
        cpt += 1;
    }
}

There's more to be said about the code at this point, but frankly I ran out of steam.

However, I'd be remiss if I didn't point out that your variable names are quite bad. Quick — what does q_s store? How about t_s? What do I store in s1 as opposed to s2? Those variable names are free; you can make them longer.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

pid_value can be declared inside the loop and does not need to be mutable.

    if filter_pid{
        pid_value = v_rest[1].parse::<f32>().unwrap();
        if pid_value < options.id{
            continue 'outer;                
        }
    }

can be

    if filter_pid{
        let pid_value = v_rest[1].parse::<f32>().unwrap();
        if pid_value < options.id{
            continue 'outer;                
        }
    }

and the line

let mut pid_value:f32;

can be deleted. The same goes for eval_value.

q_s =  cover_q.clone().to_string();
t_s =  cover_t.clone().to_string();

I have a couple things to say bout these lines. First, f32 implements Copy so you do not need to explicitly clone it, and to_string doesn't take ownership of the value anyways. Second, if you were reusing the memory of q_s and t_s it would make sense to declare them outside the loop, but since you're creating new strings there's no reason to.

So altogether, the following lines

{
    let (cover_q, cover_t) = compute_qcov_tcov(v_rest.clone());
    if cover_q < options.cover || cover_t < options.cover {
        continue 'outer;    
        }


    q_s =  cover_q.clone().to_string();
    t_s =  cover_t.clone().to_string();
}               
v_rest.truncate(5);
v_rest[3] =  &q_s;
v_rest[4] =  &t_s;

can be replaced with

    let (cover_q, cover_t) = compute_qcov_tcov(v_rest.clone());
    if cover_q < options.cover || cover_t < options.cover {
        continue 'outer;    
    }
    v_rest.truncate(5);
    v_rest[3] =  &cover_q.to_string();
    v_rest[4] =  &cover_t.to_string();

The parenthesis on the following line are unnecessary

if v_line[0] == (v_line[1]){ // if id col 1 == id col 2 skip line

that is,

if v_line[0] == v_line[1] { // if id col 1 == id col 2 skip line

works just as well.

Your code has several usages of unwrap where if the input data is bad then the program will crash. You might want to handle those cases and print out an error message indicating which line of the file the bad data is on.

This is just about making the code easier for others to read, but the indentation of some of your braces is off:

while temporary_file.exists(){
cpt += 1;
temporary_file = path_to_out_dir.join(format!("temporary_{value}",value=cpt));
    }

should instead be like this:

while temporary_file.exists() {
    cpt += 1;
    temporary_file = path_to_out_dir.join(format!("temporary_{value}",value=cpt));
}

Since compute_qcov_tcov does not mutate the input vector, you don't need to copy the vector, you can just let the function borrow the vector by passing a reference

fn compute_qcov_tcov<'a>(vec:Vec<&'a str>)->( f32, f32){

can become:

fn compute_qcov_tcov(vec: &Vec<&str>) -> ( f32, f32) {

This means you will have to call it differently:

compute_qcov_tcov(v_rest.clone())

should be

compute_qcov_tcov(&v_rest)

instead.

Since this way makes the explicit lifetime 'a unnecessary for compute_qcov_tcov, it is also unneeded on my_round and compute_cov

Also, since Rust implicitly returns the last expression in a function you don't need an intermediate variable, so

let result = (qcov, tcov);
result

can just be

(qcov, tcov)
| improve this answer | |
\$\endgroup\$

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.