Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update lib.rs #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Muhindo-Galien
Copy link

Hey @setbap

This pull request addresses various improvements and fixes in two canisters, namely user and paste. The changes aim to enhance code clarity, error handling, and overall maintainability.

Changes in user Canister

  1. Refactored User Profile Logic: Refactored the logic related to user profiles, improving code organization and readability.

  2. Simplified Profile Existence Check: Simplified the check for the existence of a user profile during profile creation, making the code more concise.

  3. Enhanced Error Handling: Improved error handling during user profile updates and creations, providing more informative error messages.

  4. Optimized Paste Index Handling: Optimized the handling of paste indices in user profiles for better performance.

Changes in paste Canister

  1. Refactored PasteData Logic: Restructured the logic for managing PasteData, improving code structure and modularity.

  2. Improved Error Messages: Enhanced error messages for paste-related queries and updates, providing clearer information about the encountered issues.

  3. Simplified Code for Finding Pastes: Simplified the code for finding pastes by tags, extension, name, and short URL, making it more readable and maintainable.

  4. Optimized Paste Retrieval: Optimized the retrieval of pastes by user, last N pastes, and other queries for better performance.

General Improvements

  1. Consistent Naming Conventions: Ensured consistent naming conventions throughout the codebase for better code readability.

  2. Enhanced Code Comments: Improved code comments to provide better documentation and understanding of the code.

  3. Fixed Minor Typos: Corrected minor typos in comments and variable names.

Copy link
Owner

@setbap setbap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, I want to thank you for your attention and time.
I think there are some problems with this pull request that I tried to specify them. In some parts, it seems that the performance has decreased, also in some parts, the nested codes have reduced the code readability

@@ -55,14 +54,14 @@ fn _get_profile(id: String) -> Option<UserProfile> {
#[ic_cdk::query]
fn get_self_info() -> Result<UserProfile, IcpUserError> {
let caller: String = ic_cdk::api::caller().to_text();
let user = _get_profile(caller);
let user = _get_profile(caller.clone());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Comment on lines +98 to +100
if let Some(paste) = _get_paste_by_id(idx) {
pastes.push(paste);
} else {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this way has more indent and i thinks original way has much cleaner code

Comment on lines +124 to +128
if let Some(paste) = _get_paste_by_id(index) {
Ok(paste)
} else {
Err(IcpPasteError::PasteNotFound)
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

original way is cleaner and more Rusty

let id = if None == caller {
ic_cdk::caller()
let id = if let Some(caller_id) = caller {
caller_id
} else {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still i think original way is cleaner and more readable.it has less indent and if works like guard.

}

// if count is None just return 10 pastes otherwise return count pastes from last paste
#[ic_cdk::query]
fn get_last_n_paste(count: Option<u8>) -> Result<Vec<PasteData>, IcpPasteError> {
let mut pastes = vec![];
let mut ids = vec![];
let mut count = if None == count { 10 } else { count.unwrap() } as u64;
if count > 10 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

max count is 10 and seems is was remove in your code

Comment on lines +215 to +220
if let Some(paste_id) = PASTES_SHORT_URL.with(|p| p.borrow().get(&short_url)) {
if let Some(paste) = _get_paste_by_id(paste_id.to_string()) {
return Ok(paste);
}
}
let paste = _get_paste_by_id(paste_id.unwrap());
paste.ok_or(IcpPasteError::PasteNotFound)
Err(IcpPasteError::PasteNotFound)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

original is cleaner

@@ -304,7 +298,7 @@ fn update_paste(paste_id: String, value: PasteDataUpdater) -> Result<PasteData,
if is_user_none {
return Err(IcpPasteError::PasteIsNotAccessable);
}
let paste = _get_paste_by_id(paste_id);
let paste = _get_paste_by_id(paste_id.clone());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why it has extra clone?

return Err(IcpPasteError::ShortUrlShouldBeBetween4And10);
}

// short url should be unique
if short_url.is_some() && _is_short_url_exist(&short) {
if _is_short_url_exist(&short) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this way has performance issue. if url doesn't exist it check for empty string in short.
why we add extra search in blockchain data while we know it is empty

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants