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

ledger #316

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

ledger #316

wants to merge 28 commits into from

Conversation

Falilah
Copy link
Contributor

@Falilah Falilah commented Dec 12, 2024

Closes #196

Copy link

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)

Copy link
Contributor

@0xNeshi 0xNeshi left a comment

Choose a reason for hiding this comment

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

Did a first iteration, good job!

fn one_entry() {
let currency = Currency::USD;
let locale = Locale::en_US;
let entries = array![("2015-01-01", "Buy present", "-1000")];
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update entry tuples into structs

fn one_entry() {
let currency = Currency::USD;
let locale = Locale::en_US;
let entries = array![("2015-01-01", "Buy present", "-1000")];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let entries = array![("2015-01-01", "Buy present", "-1000")];
let entries = array![("2015-01-01", "Buy present", -1000)];

You should use signed integers for amountInCents, see canonical data.
Any particular reason you opted for treating them as ByteArray?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only used byteArray to make the entries consistent and mostly it will be easier to deal with positive and negative value, I will try and change it to signed integers and work around it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0xNeshi, I need help trying to convert i32 data type to ByteArray, Do you have any suggestion that can help implement such?

Copy link
Contributor

Choose a reason for hiding this comment

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

it should be possible to simply append the i32 value:

       let amount_in_cents = -1000_i32;
       // ...
       let row = format!("{} | {} | {}", formatted_date, format_transaction(transaction), amount_in_cents);

Doesn't it work?

Copy link
Contributor Author

@Falilah Falilah Dec 16, 2024

Choose a reason for hiding this comment

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

According to canonical.json, the amount_in_cents is expected to be seperated with , or . based on the locale for value that are up to 5 character for example -123456 is expected to be ($1,234.56) which means I have to be able to get into individual character to determine where the , or . is to be placed and using the format! macro won't give me the opprtunity to do that.

Copy link
Contributor

@0xNeshi 0xNeshi Dec 16, 2024

Choose a reason for hiding this comment

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

    let y = -123456_i32;
    let cents = (y % 100) * -1; // In Cairo `%` includes negative remainders, so we make it a positive number!
    let main = y / 100;
    let str = format!("{main}.{cents}");
    println!("{str}"); // prints "-1234.56"
    let str = format!("{main},{cents}");
    println!("{str}"); // prints "-1234,56"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The expected data is supposed to be -1, 234.56 or -1.234,56 , the , and . that seperate the main value for readability are expected too

Copy link
Contributor

@0xNeshi 0xNeshi Dec 17, 2024

Choose a reason for hiding this comment

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

You can use a similar approach to take 3-digit chunks out of a number by dividing it with 1000, and concatenate with the appropriate delimiter.

We should follow canonical data and use signed integers. It will change your existing formatting logic, but it'll probably be even more straightforward than it currently is.
Even if it's less straightforward, the idea is to solve the canonical exercise as it is using Cairo.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can take inspiration from Python's solution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I have been able to make it work with the format! macro

let currency = Currency::USD;
let locale = Locale::en_US;
let entries = array![
("2015-01-01", "Buy present", "-1000"), ("2015-01-02", "Get present", "1000")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
("2015-01-01", "Buy present", "-1000"), ("2015-01-02", "Get present", "1000")
("2015-01-02", "Get present", "1000"), ("2015-01-01", "Buy present", "-1000"),

These entries should be switched

let entries = array![
("2015-01-01", "Something", "-1"),
("2015-01-01", "Something", "0"),
("2015-01-01", "Something", "1")
Copy link
Contributor

Choose a reason for hiding this comment

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

Place entries in the correct order

exercises/practice/ledger/tests/ledger.cairo Outdated Show resolved Hide resolved
exercises/practice/ledger/.meta/example.cairo Outdated Show resolved Hide resolved
// Format the date based on the locale
let formatted_date = format_date(@date, @locale);

// Format the change based on the currency and locale
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this comment above the function definition


// Step 2: Process transactions
for (
date, transaction, change
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
date, transaction, change
date, description, amount_in_cents

as per canonical data, the names are much clearer now

(year, month, day)
}

fn format_change(change: @ByteArray, currency: @Currency, locale: @Locale) -> ByteArray {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn format_change(change: @ByteArray, currency: @Currency, locale: @Locale) -> ByteArray {
fn format_amount(amount_in_cents: @ByteArray, currency: @Currency, locale: @Locale) -> ByteArray {

@@ -0,0 +1,225 @@
// Refactored code
use core::to_byte_array::{AppendFormattedToByteArray};
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to add a comment on which changes you made when you refactored the solution

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't have to go into a lot of detail, just a short list summarizing the changes you made

let mut sep = 0;
let mut i = 0;

while i < date.len() {
Copy link
Contributor

@0xNeshi 0xNeshi Dec 16, 2024

Choose a reason for hiding this comment

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

You could probably separate this single while loop into 3 loops, each building it's own part of the date.
Then you wouldn't need so many if checks within

Comment on lines +135 to +141
if locale == @Locale::nl_NL {
result.append_byte(' ');
}

if negative && locale != @Locale::en_US {
result.append_byte('-');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if locale == @Locale::nl_NL {
result.append_byte(' ');
}
if negative && locale != @Locale::en_US {
result.append_byte('-');
}
if locale == @Locale::nl_NL {
result.append_byte(' ');
if negative {
result.append_byte('-');
}
}

Because: locale != @Locale::en_US is the same as locale == @Locale::nl_NL

fn format_transaction(transaction: ByteArray) -> ByteArray {
let mut formatted = "";

if transaction.len() > 22 {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does 22 mean? Use a const variable with a meaningful name

formatted += "...";
} else {
formatted += transaction;
while formatted.len() < 25 {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does 25 mean? Use a const variable with a meaningful name

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.

Add Practice Exercise: ledger
2 participants