Conversation
|
Hello 👋 Thanks for your PR. This repo does not currently have dedicated maintainers. Our cross-track maintainers team will attempt to review and merge your PR, but it will likely take longer for your PR to be reviewed. If you enjoy contributing to Exercism and have a track-record of doing so successfully, you might like to become an Exercism maintainer for this track. Please feel free to ask any questions, or chat to us about anything to do with this PR or the reviewing process on the Exercism forum. (cc @exercism/cross-track-maintainers) |
0xNeshi
added
x:module/practice-exercise
| }; | ||
|
|
||
| let formatted_value = format_number(int_value, negative, currency, locale); | ||
| let mut extra = ""; |
There was a problem hiding this comment.
| let mut extra = ""; | |
| let mut padding = ""; |
more accurate
| i += 1; | ||
| } | ||
|
|
||
| const AMOUNT_COLUMN_WIDTH: usize = 13; |
There was a problem hiding this comment.
Move all constants outside to the top of the file, and outside any functions
| fn format_amount(amount_in_cents: @i32, currency: @Currency, locale: @Locale) -> ByteArray { | ||
| let amount_in_cents = format!("{amount_in_cents}"); | ||
| let mut int_value: u32 = 0; | ||
| let mut negative = false; | ||
| let mut i = 0; | ||
|
|
||
| if amount_in_cents[i] == '-' { | ||
| negative = true; | ||
| i += 1; | ||
| } | ||
|
|
||
| const AMOUNT_COLUMN_WIDTH: usize = 13; | ||
| while i < amount_in_cents.len() { | ||
| let zero_ascii = '0'; | ||
| if let Option::Some(digit) = Option::Some(amount_in_cents[i] - zero_ascii) { | ||
| int_value = int_value * 10 + digit.into(); | ||
| } | ||
| i += 1; | ||
| }; |
There was a problem hiding this comment.
This is unnecessary logic - the first line turns an integer amount_in_cents value into a bytearray, and then immediately after in this while loop, it parses that same bytearray back into an integer.
Refactor this to utilize the original integer value directly.
You can even move negative into the formatter, thus removing the extra function parameter, and doing away with the local variable
| if fraction < 10 { | ||
| result.append_byte('0'); | ||
| fraction.append_formatted_to_byte_array(ref result, 10); | ||
| } else { | ||
| fraction.append_formatted_to_byte_array(ref result, 10); | ||
| } |
There was a problem hiding this comment.
| if fraction < 10 { | |
| result.append_byte('0'); | |
| fraction.append_formatted_to_byte_array(ref result, 10); | |
| } else { | |
| fraction.append_formatted_to_byte_array(ref result, 10); | |
| } | |
| if fraction < 10 { | |
| result.append_byte('0'); | |
| } | |
| fraction.append_formatted_to_byte_array(ref result, 10); |
| let whole = value / 100; | ||
|
|
||
| result += add_sep(whole, locale); | ||
| let fraction = value % 100; |
There was a problem hiding this comment.
move fraction closer to where it's actually used
| Locale::en_US => { | ||
| day += "/"; | ||
| month += "/"; | ||
| ByteArrayTrait::concat(@month, @ByteArrayTrait::concat(@day, @year)) | ||
| }, | ||
| Locale::nl_NL => { | ||
| day += "-"; | ||
| month += "-"; | ||
| ByteArrayTrait::concat(@day, @ByteArrayTrait::concat(@month, @year)) | ||
| }, |
There was a problem hiding this comment.
Why not use the shorter format!?
| fn split_date(date: @ByteArray) -> (ByteArray, ByteArray, ByteArray) { | ||
| let mut year = ""; | ||
| let mut month = ""; | ||
| let mut day = ""; | ||
| let mut sep = 0; | ||
| let mut i = 0; | ||
|
|
||
| while i < date.len() { | ||
| if sep == 0 && i < 4 && date[i] != '-' { | ||
| year.append_byte(date[i]); | ||
| } else if date[i] == '-' { | ||
| sep += 1; | ||
| } else if sep == 1 && i < 7 && date[i] != '-' { | ||
| month.append_byte(date[i]); | ||
| } else { | ||
| day.append_byte(date[i]); | ||
| } | ||
| i += 1; | ||
| }; | ||
|
|
||
| (year, month, day) | ||
| } |
There was a problem hiding this comment.
Here's a more efficient solution:
| fn split_date(date: @ByteArray) -> (ByteArray, ByteArray, ByteArray) { | |
| let mut year = ""; | |
| let mut month = ""; | |
| let mut day = ""; | |
| let mut sep = 0; | |
| let mut i = 0; | |
| while i < date.len() { | |
| if sep == 0 && i < 4 && date[i] != '-' { | |
| year.append_byte(date[i]); | |
| } else if date[i] == '-' { | |
| sep += 1; | |
| } else if sep == 1 && i < 7 && date[i] != '-' { | |
| month.append_byte(date[i]); | |
| } else { | |
| day.append_byte(date[i]); | |
| } | |
| i += 1; | |
| }; | |
| (year, month, day) | |
| } | |
| fn split_date(date: ByteArray) -> (ByteArray, ByteArray, ByteArray) { | |
| // Helper to append bytes to a ByteArray | |
| #[cairofmt::skip] | |
| let append_range = |start: usize, end: usize| -> ByteArray { | |
| let mut part = ""; | |
| for i in start..end { | |
| part.append_byte(date[i]); | |
| }; | |
| part | |
| }; | |
| // Find separator positions | |
| let mut separators = array![]; | |
| for i in 0..date.len() { | |
| if date[i] == '-' { | |
| separators.append(i); | |
| } | |
| }; | |
| // Extract year, month, and day | |
| let year = append_range(0, *separators[0]); | |
| let month = append_range(*separators[0] + 1, *separators[1]); | |
| let day = append_range(*separators[1] + 1, date.len()); | |
| (year, month, day) | |
| } |
Benefits:
- Avoids multiple checks during iteration.
- The logic is broken into meaningful parts, making the code easier to test and reuse.
- The intent of each section is clear and self-contained
- More gas efficient
|
|
||
|
|
||
| fn add_sep(whole: u32, locale: @Locale) -> ByteArray { | ||
| let mut result = ""; |
There was a problem hiding this comment.
redundant result, you can remove it and return values directly
| let mut temp = ""; | ||
| @whole.append_formatted_to_byte_array(ref temp, 10); |
There was a problem hiding this comment.
| let mut temp = ""; | |
| @whole.append_formatted_to_byte_array(ref temp, 10); | |
| let temp = format!("{whole}"); |
no need to overcomplicate things
| Entry { date: "2015-01-01", description: "Buy present", amount_in_cents: -1000 }, | ||
| Entry { date: "2015-01-02", description: "Get present", amount_in_cents: 1000 }, |
There was a problem hiding this comment.
Double check that all test entries are in correct order
| let entries = array
|
Hey @Falilah , hope all is well! I wanted to commend on on your contributions to this track so far, you've really helped a lot! This PR is so close to being merged and the Cairo community is going to love having this new exercise available. I noticed we're getting close to the finish line with this one. How are you feeling about the remaining tasks? If you're running into any roadblocks or could use a second pair of eyes on anything, just let me know - happy to jump in and help however I can. |
2 participants
Closes #196
Opened instead of #316
I have not done the switches you recommended in the previous one and reason is that I am still thinking of how best to write the sorting logic of the
ByteArray.