[dependencies]
rand = "0.8"
rand_core = { version = "0.9", features = ["os_rng"] }
Why are you using two different versions of rand_core (one directly, 0.9, and one via rand, 0.6)? If there isn’t a reason, you should try to have only one, which improves build performance (either by removing the rand_core dependency, or upgrading rand 0.8 to 0.9). If there is a reason, add a comment documenting the reason.
From searching your main.rs, it seems that you are not using the rand_core, and also that you aren’t using validator, either.
// User Types
#[derive(Serialize, Deserialize, PartialEq, Debug, Clone)]
enum UserType {
Admin,
Guest,
}
// Activeness
#[derive(Serialize, PartialEq, Deserialize, Debug, Clone)]
enum ActiveStatus {
LoggedIn,
LoggedOut,
}
These comments are conveying less information than the types’ declarations, and so in their current form, it would be best to remove them. But if you want to expand them instead, then you should make documentation comments (three slashes):
/// Type of a user.
///
/// The type determines blah blah blah ...
#[derive(...)]
enum UserType {
impl User {
fn new(name: &str, address: &str, email: &str, password: &str) -> Self {
let hashed = hash_password(password);
Self {
name: String::from(name),
address: String::from(address),
email: String::from(email),
Functions, especially constructors, should typically take borrowed or owned data according to what they need. Since this function needs Strings, its parameters should have type String, not &str. This avoids the situation where new() copies a string that it could have simply taken ownership of instead.
struct Product {
name: String,
price: f64,
Money for accounting purposes (which this is) should NEVER be stored as a floating-point number. If you do this, you will end up with stray quadrillionths of your monetary units throwing off your results, and have difficulty making your software comply with accounting rules it may be subject to.
Instead, you should always use an integer, storing whatever the smallest unit of your currency is. It may be useful to store this in a newtype to avoid confusion and allow implementing useful operators (like multiplying by a sales tax or VAT rate, followed by the legally required form of rounding the result):
struct UsDollar {
cents: u64,
}
struct Product {
name: String,
price: UsDollar,
}
let acc_no = generate_account_number(phone_no);
This is bad design:
- it potentially leaks sensitive information (e.g. people working on fulfilling orders shouldn’t necessarily have access to the customer’s phone number)
- Phone numbers are not uniquely assigned to people — they may be reused or shared. You might have two employees or customers with the same phone number.
- Even if you have a solution for conflicts, it’s better to assign a meaningless account number so that nobody ever thinks it’s usable as a callable phone number when it might not be.
struct Cart {
user: Account,
You shouldn’t duplicate a user’s entire account information into their cart. That violates typical user expectations (if they change their info it should apply everywhere, including to their next order, whether or not they currently have a cart or not) and also makes the cart information more sensitive than if it’s only a list of products and an account number.
Also, reading further, it seems like your “cart” struct is more properly called a “line item”, since it contains only one product. Duplicating the account info for every single item creates even more potential for misbehavior.
struct Order {
user: Account,
carts: Vec<Cart>,
grand_total: f64,
Don’t store the total; calculate it when it is needed. The cost of the computation is a much smaller problem than having a discrepancy between item prices and the total.
println!(
"Options: \n1. Add Product\n2. View All Products\n3. View Single Product\n4. View All Cart\n5. View My Cart\n6. View All Orders\n7. View My Orders\n8. Create Account\n9. Change User Type\n10. Login\n11. Fund my Account\n12. Exit"
);
Something you should consider as a future exercise is making this menu generated based on the available options, rather than a hard-coded string that could contain a misnumbering. For example, you could define
#[derive(Clone, Copy, Debug)]
enum Command {
AddProduct,
ViewAllProducts,
...
}
impl Command {
fn menu_item_name(self) -> &str {
match self {
Command::AddProduct => "Add Product",
...
}
}
}
const MENU_CONTENTS: &[Command] = &[
Command::AddProduct,
Command::ViewAllProducts,
...
];
and then you can print a numbered menu, and accept input, based on the contents of MENU_CONTENTS.
In general, good code avoids spreading a single fact (in this case, how the menu items are numbered) across different places, unless it is necessary, and when it is necessary, takes measures to ensure that the different places continue to match (such as comments pointing from one place to the other, and tests that verify the behavior is consistent).
let _product_db: Vec<Product> = match load_database(product_path) {
Ok(product) => product,
Err(err) => {
println!("{}", err);
return;
}
};
You should avoid duplicating error reporting code, because it is a very large amount of clutter in the application logic. In order to do that in this case, you should move all of your command implementations into a function (similar to the single_product_commands() function you already have), which returns a Result and uses the ? operator, and then you can match the result of that function once inside the main loop.
I haven’t thoroughly gone through the rest of your code, but I hope these comments help.
