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

Challenge 2/ronald ris #51

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

Conversation

ronaldris21
Copy link

This is my propose for solving the challenge problems.

Copy link

@kevin-vm kevin-vm left a comment

Choose a reason for hiding this comment

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

Very good solution overall! Minor suggestions to consider for next time

--
SELECT
TYPE ACCOUNT_TYPE,
ROUND(SUM(MOUNT)::NUMERIC, 2) MONEY_SUM
Copy link

Choose a reason for hiding this comment

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

Nice to see you are using built-in functions such as round

Copy link
Author

Choose a reason for hiding this comment

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

Thx this was to avoid problems with DOUBLE PRECISION and trailing number due to its precision scope.

Variable types matter!

END AS mount
FROM movements
)
UNION ALL
Copy link

Choose a reason for hiding this comment

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

Why using union all instead of union

Copy link
Author

@ronaldris21 ronaldris21 Dec 5, 2024

Choose a reason for hiding this comment

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

Asumme all movement incoming to an account is "1000", and starting mount on the same account is "1000".

UNION would take both rows as the same, while in reality the account should have 2000 as total balance!

This way I make sure to have all the mounts related to the account (account.from + account.to + account.startingMount) = account.finalBalance

Copy link

Choose a reason for hiding this comment

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

That's a pretty good explanation, with a nice example! Good job!

-- 4
-- I MAKE SURE I USE EVERY ACCOUNT BY UNION ALL!
Copy link

Choose a reason for hiding this comment

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

I like this consideration. It's not implicit in the exercise, but it's good to have it!

src/answers.sql Outdated
RAISE NOTICE 'Account balance from % is %', account_id_b, account_balance_b;

--b. Add a new movement with the information: from: 3b79e403-c788-495a-a8ca-86ad7643afaf make a transfer to fd244313-36e5-4a17-a27c-f8265bc46590 mount: 50.75
IF account_balance_a < 50.75 THEN
Copy link

Choose a reason for hiding this comment

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

You could have store 50.75 in a variable, so you don't need to update all places if you need to change the amount

Copy link
Author

Choose a reason for hiding this comment

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

I resolved it using transfer_mount and out_mount.
A variable for each transfer and out mount.

DO $$
DECLARE
    account_balance_a DOUBLE PRECISION;
    account_balance_b DOUBLE PRECISION;
	
    account_id_a UUID := '3b79e403-c788-495a-a8ca-86ad7643afaf';
    account_id_b UUID := 'fd244313-36e5-4a17-a27c-f8265bc46590';

    transfer_mount DOUBLE PRECISION := 50.75;
    out_mount DOUBLE PRECISION := 20;
BEGIN

VALUES (gen_random_uuid(), 'TRANSFER', account_id_a,account_id_b, 50.75);
RAISE NOTICE 'Transfer % USD from % to % MADE SUCCESSFULLY!', 50.75, account_id_a, account_id_b;
END IF;

Copy link

Choose a reason for hiding this comment

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

You could have add a savepoint here, to save your changes up to this point

Copy link
Author

@ronaldris21 ronaldris21 Dec 5, 2024

Choose a reason for hiding this comment

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

I could, but it'd lose the point of the transaction!

In case you may want to have a save point, I would most likely make 2 transactions.
This way it commits itself once the first is done or the second is finished.

TRANSACTION 1 for TRANSFER movement:

DO $$
DECLARE
    account_balance_a DOUBLE PRECISION;
	
    account_id_a UUID := '3b79e403-c788-495a-a8ca-86ad7643afaf';
    account_id_b UUID := 'fd244313-36e5-4a17-a27c-f8265bc46590';

    transfer_mount DOUBLE PRECISION := 50.75;
BEGIN

EXCEPTION
--
END;
$$;

TRANSACTION 2 for OUT movement:

DO $$
DECLARE
    account_balance_a DOUBLE PRECISION;
	
    account_id_a UUID := '3b79e403-c788-495a-a8ca-86ad7643afaf';
    account_id_b UUID := 'fd244313-36e5-4a17-a27c-f8265bc46590';

    out_mount DOUBLE PRECISION := 50.75;
BEGIN

EXCEPTION
--
END;
$$;

SELECT money into account_balance_b
FROM get_accounts_final_balance()
WHERE account_id = account_id_b;

Copy link

Choose a reason for hiding this comment

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

Where is the commit?

Copy link
Author

@ronaldris21 ronaldris21 Dec 5, 2024

Choose a reason for hiding this comment

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

Not need due to the sintaxis I'm using.

DO $$
DECLARE
--
BEGIN

--

EXCEPTION
--
END;
$$;

It treats the whole block as a transaction and only commit once its completely done

Copy link

@kevin-vm kevin-vm left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback

END AS mount
FROM movements
)
UNION ALL
Copy link

Choose a reason for hiding this comment

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

That's a pretty good explanation, with a nice example! Good job!

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.

3 participants