Feedback on my first Rust project

So I'm brand new to the language (just installed 6 days ago) and I finally have a working project: a simple CLI that allows you to get the weather for the city/state of your choosing. I'd like feedback on the code if anyone would be so gracious as to provide some. I don't really feel great about the error handling but I am really struggling with that aspect at the moment.

The project is set up like this:

weather_cli/
-src/
--api_call.rs
--main.rs
--user_input.rs
-Cargo.toml

Cargo.toml:

[package]
name = "weather_cli"
version = "0.1.0"
authors = ["Matt Helm <myemail>"]
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
chrono = "0.4.19"
reqwest = { version = "0.11.0", features = ["blocking", "json"] }
serde = { version = "1.0.123", features = ["derive"] }
serde_json = "1.0.61"
structopt = "0.3.21"

api_call.rs

use std::env;
use reqwest::blocking;
use serde_json::Value;

pub fn get_weather(cty: &Vec<String>, st: &Vec<String>) -> Result<serde_json::Value, Box<dyn std::error::Error>> {
    let url = "https://api.openweathermap.org/data/2.5/weather";
    let api_key = env::var("OPEN_WEATHER_API_KEY")?;
    let city:String = cty.join(" ");
    let state: String = st.join("");
    let params = [
        ("q", city + "," + &state),
        ("appid", api_key),
        ("units", String::from("imperial"))
    ];
    let url = reqwest::Url::parse_with_params(url, &params)?;
    let res = blocking::get(url)?.text()?;
    let v: Value = serde_json::from_str(&res)?;
    Ok(v)
}

user_input.rs

use structopt::StructOpt;

#[derive(StructOpt, Debug)]
#[structopt(name = "weather")]
pub struct Cli {

    #[structopt(long, required=true, help="The name of the city for which you want to get the weather")]
    pub city: Vec<String>,

    #[structopt(long, required=true, help="The state in which the city is located")]
    pub state: Vec<String>,

    #[structopt(short, long, help="Adds sunrise/sunset times to results")]
    pub daylight: bool

}

pub fn get_user_input() -> Cli {
    let cli = Cli::from_args();
    return cli
}

main.rs

mod api_call;
use chrono::offset::Utc;
use chrono::TimeZone;
use chrono::Local;
mod user_input;

fn main() {
    let input = user_input::get_user_input();
    let jsonval = api_call::get_weather(&input.city, &input.state);
    match jsonval {
        Ok(v) => {
            if v["cod"] == 200 {
                println!("Current Conditions: {}", v["weather"][0]["description"]);
                println!("Temperature: {}°", v["main"]["temp"]);
                println!("Feels Like: {}°", v["main"]["feels_like"]);
                println!("Humidity: {}%", v["main"]["humidity"]);
                println!("Wind: {} mph", v["wind"]["speed"]);
                println!("Gust: {} mph", v["wind"]["gust"]);
                if input.daylight {
                    let sunrise = match v["sys"]["sunrise"].as_i64() {
                        Some(num) => Utc.timestamp(num, 0).with_timezone(&Local).time().format("%I:%M:%p"),
                        None => Utc.timestamp(0,0).with_timezone(&Local).time().format("%I:%M:%p")
                    };
                    let sunset = match v["sys"]["sunset"].as_i64() {
                        Some(num) => Utc.timestamp(num, 0).with_timezone(&Local).time().format("%I:%M:%p"),
                        None => Utc.timestamp(0,0).with_timezone(&Local).time().format("%I:%M:%p")
                    };
                    println!("Sunrise: {}", sunrise);
                    println!("Sunset: {}", sunset);
                }
            } else {
                println!("Error: {}", v["message"])
            }
        }
        Err(err) => println!("{:#?}", err)
    }
}

Hey,
This seems to be a good start!

I have some ideas to continue your Rust practice by improving your program:

  • Use an enum as an error type, impl std::convert::From<XXXX> for Error for all 3rd party error types and replace Box<dyn std::error::Error> by it. Here is one of the most up to date resource about error handling: Rust: Structuring and handling errors in 2020 - nick.groenen.me
  • use a struct to deserialize the api's reponse to instead of accessing JSON fields like in dynamic languages.
  • move to async (instead of using reqwest::blocking) by using tokio as a runtime.
  • move your code to github or gitlab, it's easier to share progress :slight_smile:

Welcome to the community :slight_smile:

1 Like

I don't think there's any reason to do this (except as an educational exercise). The program makes one API request per invocation and can't proceed until it receives a response, there's not even any potential gain from concurrency there. reqwest::blocking is simpler to use and entirely appropriate for this situation.

5 Likes

Thanks for the feedback!

I agree totally, I don't like the Box<Error> approach at all.

  • use a struct to deserialize the api's reponse to instead of accessing JSON fields like in dynamic languages.

:grinning: This is how I set out to do it but I gave up lol. I was able to deserialize the JSON responses into different variants (based on success/error of the API call) but then I couldn't figure out how to do what I wanted in main() with those results. Also, it seems very tedious as some APIs return JSON objects with a large number of fields so I had to create many different structs for this, some of which contain other structs within (due to the nested nature of JSON). To make matters worse, the API isn't consistent in what it returns - if it doesn't have wind gust information, for example, there is no gust key on the JSON object returned so all the fields on the structs would have to be optional. It would be nice if the keys on the object that the API returns were always the same but then missing value could just be empty strings (seems like this would be easier to deal with).

  • move to async (instead of using reqwest::blocking ) by using tokio as a runtime.

I'll definitely need to figure this out as I begin to build more complex applications.

  • move your code to github or gitlab, it's easier to share progress :slight_smile:

GitHub - mthelm85/weather_cli :slightly_smiling_face:

2 Likes

You should be able to achieve this by putting #[serde(default)] on the relevant fields instead of wrapping the types in Option. See Field attributes · Serde.

Sure, but as the program is relatively simple, it may be a good occasion to start using async and fight with the new rules possibly imposed by the compiler :slight_smile:

This function could use a couple of improvements. Firstly, avoid creating variables to hold return values, it just adds cognitive load, one more name to read and see how it's used. Also return is not idiomatic at the end of a function. It's for returning early.

pub fn get_user_input() -> Cli {
    Cli::from_args()
}

However, I'd really just avoid defining the function at all. The method is perfectly clear on its own. Defining this function just causes your reader to have to look it up to see what it is. Unless maybe you're planning on adding the ability to take input from stdin or something?

3 Likes

I don't know structopt well, but having a Vec<String> here and then joining together later seems very confusing. Better to just ask for a String. If structopt can't give you a nice CLI for that then I'd avoid it. If you want a simpler API with less flexibility (but at least you can ask for a String and get one) you could try auto-args, which is similar in spirit to structopt. But the point of either is good your struct to match with the types that you want.

Maybe your goal is to avoid the user having to put shell quotes around arguments with spaces in then? In which case of I argue that it's better to design command line apps for users who know how to use a shell.