Skip to content
Snippets Groups Projects
Commit 37f6f11c authored by Jonas Johan Solsvik's avatar Jonas Johan Solsvik :video_game:
Browse files

discuss good and bad code

parent 926bfebc
Branches
No related tags found
No related merge requests found
......@@ -81,13 +81,109 @@ NOTE: We're interested in the 'HOW' and the 'WHY/WHY NOT' in the report.
### 2.1 A link to, and discussion of, code you consider good
```rust
pub async fn run(config: Config) -> Result<String, std::io::Error> {
match config.cmd {
CMD::Help => help(),
// DID
CMD::Init => init(),
CMD::Doc => doc(),
CMD::Connect{ didname, did } => connect(&didname, &did),
CMD::Dids => dids(),
CMD::Did{ didname } => did(&didname),
// DIDComm v2
CMD::Write{ didname, message } => write(&didname, &message),
CMD::Read{ dcem } => read(&dcem),
CMD::Hold{ dcem } => hold(&dcem),
CMD::Messages => messages(),
CMD::Message{ message_id } => message(&message_id),
// Verifiable Credentials
CMD::IssuePassport{ didname } => issue("Passport", &didname).await,
CMD::IssueLawEnforcer{ didname } => issue("LawEnforcer", &didname).await,
CMD::IssueTrafficAuthority{ didname } => issue("TrafficAuthority", &didname).await,
CMD::IssueDriversLicense{ didname } => issue("DriversLicense", &didname).await,
CMD::Present{ didname, dcem } => present(&didname, &dcem).await,
CMD::Verify{ issuer_didname, subject_didname, dcem } => verify(&issuer_didname, &subject_didname, &dcem).await,
}
}
```
*Link: https://github.com/DIN-Foundation/bcs-ntnu-2021/blob/main/did-cli/src/lib.rs#L2-L28*
Since our implementation is a Command-Line interface, there is a need to run different functionality depending on which command the user selects. The `run()`-function above is responsible mapping commands and arguments to a runnable function. It is basically a giant switch. So why do I like this piece of code?:
- **Decouples input-logic from application-logic** - The `run()`-function decouples application-logic from the `Config`-struct generated by the input-parsing code. In the `run()`-function one would be tempted to just pass the `Config` directly as an argument to all the underlying application functions. like `connect(config)`, `did(config)`, etc. This would then introduce the `Config`-struct as a dependency to the large parts of the codebase, making changes to the `Config`-struct potentially lead to very broad changes. By not falling into this temptation, and instead being very specific about which arguments are passed furteher on to the underlying functions, we avoid this trap, and the coupling between input-code and application-code remains very low. In pratice this means that if we change the layout of the `Config`-struct, most of the time we only have to change how the `run()`-function passes it's arguments further down the pipeline, leaving most of the codebase untouched. Decoupling is important in a collaborative environment. Decoupling makes the distinction between different parts of the codebase clear, and makes it easier for multiple people working on the codebase at the same time, without stepping on each others toes (merge conflicts).
- **Overview** - The `run()`-function is mostly self-explanatory, and gives a great overview of all functionality of the system. It is almost like a help-page.
- **Decouples data from functionality** - I already touched upon decoupling, but there is another point to be made about this. Instead of having free-standing functions for application logic amd the need to map commands to which free-standing function they are supposed to run in an imperative fashion, one could instead use an object-oriented approach and bundle the mapping between between commands and functionality inside the `Config`-struct. This introduces an inverse coupling of the one discussed in my first point about decoupling. Keeping data decoupled from functions allows us to only select if we only want to depend on the data layout, only depend on functions or both. Bundling data and logic together forces us to always depend on both.
### 2.2 A link to, and discussion of, code you consider bad
```rust
fn init() -> Result<String, std::io::Error> {
use std::io::Write;
// 1. Create empty folders, if not exists
if !std::fs::metadata(root_path()).is_ok() {
std::fs::create_dir_all(root_path()).unwrap();
}
if !std::fs::metadata(dids_path()).is_ok() {
std::fs::create_dir_all(dids_path()).unwrap();
}
if !std::fs::metadata(did_names_path()).is_ok() {
std::fs::create_dir_all(did_names_path()).unwrap();
}
if !std::fs::metadata(messages_path()).is_ok() {
std::fs::create_dir_all(messages_path()).unwrap();
}
```
*Link: https://github.com/DIN-Foundation/bcs-ntnu-2021/blob/main/did-cli/src/lib.rs#L60-L75*
DID-CLI's init-function starts with creating a bunch of directories, if they don't already exist. There are multiple reasons why this code is bad:
- **No else** - For every one of these ifs there is an else. In this context getting in the else-block means: "The directory you tried to create already exists.". In this case, the init() function should just exit and do nothing. Instead it continues executing, potentially corrupting existing files. What should have happened here is that the init() function should exit, and then give an error message to the user. Something like this: `"A `.did/`-directory already exists. Do you want to overwrite it? (yes/no)"`.
- **.unwrap()** - In Rust, whenever you are using `.unwrap()`, it means: Succeed or panic on this line and stop execution. This may be ok in many cases. It is not ok in this case and probably it was just done because of lazyness. Failing to create a directory is a potential error that could happen, and it is not a programmers error. Therefore we should have a way to recover from the error. Maybe give an error to the user and ask if he wants to "try again?". Also if the creation of directory number 3 fails, after 1 and 2 succeeded, there should be a cleanup of directory 1 and 2, before exiting the program. Leaving 1 and 2 there without a directory 3 and 4 is not a valid state for the application to be in. Either no directories should be present or all 4. In database-terms: The directories should be created as a single transaction, and the transaction should be rolled back to the initial state, if anything fails during the transaction.
### 2.3 A link to two pieces of code, a before and after refactoring.
This will include a discussion of why the code was refactored
**Before reface**
```rust=
```
**After refac**
```rust
fn read(dcem: &str) -> Result<String, std::io::Error> {
// 1. Get did:keys
let to_key = get_self_didkey();
let from_key = get_from_key_from_didcomm_message(dcem);
// 2. Decrypt message, to get the contents of the message-body
let (body, _) = decrypt_didcomm(&from_key, &to_key, dcem);
Ok(format!("{}", body))
}
```
### 2.4 Personal reflection about professionalism in programming
- Remove dead code - Clean up code that is no longer use. Do not write code that is not in use from day 1
- Write requirements -> Then write tests -> Then write code
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment