2
\$\begingroup\$

I made a little program that allows users to download a local copy of all their GitHub repositories for backup purposes. I've tried to write idiomatic Rust code, but I'm sure there are ways I could improve my code as a novice:

extern crate github_rs;
extern crate serde_json;
extern crate hyper;
extern crate terminal_size;
use serde_json::Value;
use github_rs::client::Github;
use std::process::Command;
use terminal_size::{Width, terminal_size};

struct Config{
    token:String,
    output_location:String,
}

impl Config{
    pub fn new() -> Config{
        Config {
            token: String::new(),
            output_location: String::from("repositories")
        }
    }
    pub fn new_with_token(token:String) -> Config{
        let mut config:Config = Config::new();
        config.token = token;
        config
    }
    pub fn new_with_token_and_output_dir(token:String, output_location:String) -> Config{
        let mut config:Config = Config::new();
        config.token = token;
        config.output_location = output_location;
        config
    }

}


fn get_nth_argument(n:usize) -> Result<String, &'static str>{
    for arg in std::env::args().skip(n){
        return Ok(arg);
    }
    Err("No token was provided")
}

fn get_width_of_terminal() -> usize{
    let size = terminal_size();
    if let Some((Width(w),_)) = size {
        return w as usize;
    }
    0 as usize
}

fn process_repo_json(repo_json:Option<Value>, config:Config){
    if let Some(json) = repo_json{
        if json.is_array() {
            if let Some(repository_list) = json.as_array(){
                for repository in repository_list{
                    //println!("Repository: {:?}", repository);
                    if repository.is_object(){
                        let repository_html_url = repository.get("html_url").unwrap().as_str().unwrap();
                        let repository_name = repository.get("name").unwrap().as_str().unwrap();
                        println!("Downloading {}", repository_html_url);

                        let protocol:String = repository_html_url.chars().take(8).collect();
                        let url:String = repository_html_url.chars().skip(8).collect();
                        let clone_url = format!("{}{}@{}",protocol,config.token,url);
                        let result = Command::new("git")
                            .arg("clone")
                            .arg(clone_url)
                            .arg(format!("{}/{}", &config.output_location, repository_name))
                            .spawn()
                            .expect("failed to execute command")
                            .wait();
                        println!("{}", "-".repeat(get_width_of_terminal()));
                    }
                }
            }
        }
    }      
}

fn validate(repos:Result<(hyper::Headers, hyper::StatusCode, std::option::Option<Value>), github_rs::errors::Error>,
    config:Config){
    match repos {
        Ok((_, status, json)) => {
            match status{
                hyper::StatusCode::Unauthorized => {
                    println!("Invalid token entered!");
                }
                _ => {
                    process_repo_json(json,config);                      
                }
            }

        },
        Err(e) => println!("{}", e)
    }  
}

fn main() {
    let arg_tuple=(get_nth_argument(1), get_nth_argument(2));
    let config:Config;
    //println!("{:?}", arg_tuple);
    match arg_tuple{
        (Ok(tok), Ok(dir)) => config=Config::new_with_token_and_output_dir(tok, dir),
        (Ok(tok), _) => config=Config::new_with_token(tok),
        _ => {
            println!("Token was not supplied!");
            std::process::exit(1);
        }
    }
    let client = Github::new(&config.token).unwrap();
    let repos = client.get()
                .user()
                .repos()
                .execute();
    validate(repos,config);
}

The way it works is that the user runs the program with their API token as a mandatory first argument and the desired output directory as an optional second argument. The program will then get iterate over a list of all the user's repositories and essentially run git clone https://<token>@github.com/user/repo to download the repositories.

\$\endgroup\$
2
\$\begingroup\$
  1. Always pay attention to compiler warnings!

    warning: unused variable: `result`
      --> src/main.rs:66:29
       |
    66 |                         let result = Command::new("git")
       |                             ^^^^^^
       |
    
  2. Run rustfmt on your code to tell you things like:

    • Spaces after : when declaring a value: fn new_with_token_and_output_dir(token: String, output_location: String)
    • Spaces around curly braces: -> Config {
    • Spaces after ,: Some((Width(w), _))
  3. Run clippy to tell you things like:

    warning: this loop never actually loops
      --> src/main.rs:39:5
       |
    39 | /     for arg in std::env::args().skip(n) {
    40 | |         return Ok(arg);
    41 | |     }
       | |_____^
       |
       = note: #[warn(never_loop)] on by default
       = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#never_loop
    
    warning: this argument is passed by value, but not consumed in the function body
      --> src/main.rs:53:56
       |
    53 | fn process_repo_json(repo_json: Option<Value>, config: Config) {
       |                                                        ^^^^^^ help: consider taking a reference instead `&Config`
       |
       = note: #[warn(needless_pass_by_value)] on by default
       = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_pass_by_value
    
  4. I recommend newlines between sequential function definitions.

  5. The repeated setup makes me worried from a maintainability perspective as well as an efficiency perspective. Does it ever make sense to have an empty token? I'd chain the constructor methods and use the field initialization shorthand.

  6. Iterator::nth exists.

  7. Can convert an Option to a Result by using Option::ok_or

  8. No need to store a temporary variable; you can call a function in a if let or match head.

  9. Avoid using if let if you actually care about multiple possibilities; use match instead.

  10. Can use methods on Option and Result to unwrap a value with a default, even mapping the value if there.

  11. There's no need to cast 0 to a usize, integers can infer the type.

  12. Don't pair if_array with as_array. as_array will return a None if it's not an array.

  13. Can chain Options that return more options using Option::and_then.

  14. take(8) is super suspicious. Where is that value coming from? I'd guess https://, but how would someone know that from the code?

  15. Taking 8 and then taking the rest would be more efficient using a single iterator.

  16. Even better would be splitting the string at N bytes, since you know the string you are looking for.

  17. Even better would be to use a library to parse the URL and build it so you don't have to hard code any of this logic.

  18. Unneeded & when formatting the output location.

  19. Could probably hoist the calculation of the width of the terminal out of the inner loop, but may want to respond to width changes during the run.

  20. Instead of traversing the entire argument list twice, just pull the next value. We fuse the iterator so we can call it multiple times without worrying about what happens after the first None. This also avoids the need to have an error message in get_nth_argument that is never used.

  21. There's no need to specify the type of config, it will be inferred.

  22. There's no need to set config in the match -- match evaluates to a value.

  23. Avoid using unwrap, prefer expect instead. This gives at least slightly useful error messages.

  24. The function validate does way more than "validation" -- it downloads everything. The function is misnamed at best.

  25. Why does an invalid token or a failed HTTP request cause the program to exit with exit code 0?

extern crate github_rs;
extern crate serde_json;
extern crate hyper;
extern crate terminal_size;

use serde_json::Value;
use github_rs::client::Github;
use std::process::Command;
use terminal_size::{Width, terminal_size};

struct Config {
    token: String,
    output_location: String,
}

impl Config {
    pub fn new_with_token(token: String) -> Config {
        Self::new_with_token_and_output_dir(token, String::from("repositories"))
    }

    pub fn new_with_token_and_output_dir(token: String, output_location: String) -> Config {
        Self { token, output_location }
    }
}

fn get_width_of_terminal() -> usize {
    terminal_size().map_or(0, |(Width(w), _)| w as usize)
}

fn process_repo_json(repo_json: Option<Value>, config: Config) {
    if let Some(repository_list) = repo_json.as_ref().and_then(|j| j.as_array()) {
        for repository in repository_list {
            if repository.is_object() {
                let repository_html_url =
                    repository.get("html_url").and_then(Value::as_str).unwrap();
                let repository_name =
                    repository.get("name").and_then(Value::as_str).unwrap();

                println!("Downloading {}", repository_html_url);

                let protocol = "https://";
                let (_, url) = repository_html_url.split_at(protocol.len());

                let clone_url = format!("{}{}@{}", protocol, config.token, url);
                let output_dir = format!("{}/{}", config.output_location, repository_name);

                Command::new("git")
                    .arg("clone")
                    .arg(clone_url)
                    .arg(output_dir)
                    .spawn()
                    .expect("failed to execute command")
                    .wait()
                    .unwrap();

                println!("{}", "-".repeat(get_width_of_terminal()));
            }
        }
    }
}

fn validate(
    repos: Result<
        (hyper::Headers, hyper::StatusCode, std::option::Option<Value>),
        github_rs::errors::Error,
    >,
    config: Config,
) {
    match repos {
        Ok((_, status, json)) => {
            match status {
                hyper::StatusCode::Unauthorized => println!("Invalid token entered!"),
                _ => process_repo_json(json, config),
            }
        }
        Err(e) => println!("{}", e),
    }
}

fn main() {
    let mut args = std::env::args().skip(1).fuse();

    let config = match (args.next(), args.next()) {
        (Some(tok), Some(dir)) => Config::new_with_token_and_output_dir(tok, dir),
        (Some(tok), _) => Config::new_with_token(tok),
        _ => {
            println!("Token was not supplied!");
            std::process::exit(1);
        }
    };

    let client = Github::new(&config.token).unwrap();
    let repos = client.get().user().repos().execute();

    validate(repos, config);
}
\$\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.