Skip to content

London_ITP_Jan|Aung_Ye_Kyaw |Module-Structuring-Testing-Data |Coursework/sprint 3 #329

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 59 commits into from
Closed

London_ITP_Jan|Aung_Ye_Kyaw |Module-Structuring-Testing-Data |Coursework/sprint 3 #329

wants to merge 59 commits into from

Conversation

sarawone
Copy link

Hi ,

I hope you are well!

I have implemented functions .
Kindly check and let me know for any requirements and provide feedback or advice for better way for the codes.

Thank you so much.

Thanks and Regards,
Aung Ye Kyaw

@sarawone sarawone mentioned this pull request Feb 25, 2025
Open
@sarawone sarawone added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Feb 25, 2025
@sarawone sarawone self-assigned this Feb 25, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Mar 7, 2025

Can you (perhaps with the help of a volunteer) fix this branch so that it only contains modified files in the Sprint-3 folder?

@sarawone
Copy link
Author

Can you (perhaps with the help of a volunteer) fix this branch so that it only contains modified files in the Sprint-3 folder?

I did it . Could you please provide feedback. Thank you .

@cjyuan
Copy link
Contributor

cjyuan commented Mar 14, 2025

There are still unmodified files in Sprint-1 and Sprint-2 folders in the current branch. You can see all the modified files by clicking the "Files changed" tab in this PR (see pic):
image

There is a chance that you made the changes successfully on local repository but did not "force push" the change to remote repository. If you simply tried to sync the files with remote repository, Git might pull the remote files first before carry out the push action, making the changes you made undone.

@sarawone
Copy link
Author

hi CJ .. I did the change and force push . I hope this time it is success . Please check and kindly let me know .
Thank you.

@sarawone sarawone added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 17, 2025
cjyuan
cjyuan reviewed Mar 18, 2025
View reviewed changes
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Code have been improved! Good job!

I think some of the functions and descriptions can still be improved.

Sprint-3/1-key-implement/3-get-card-value.js Outdated

function getCardValue(card) {

let cards = card.slice(0,-1); // drop one last letter from string
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use "rank" as variable name?

On separate note, we usually use plural form of an identifier for arrays.

Copy link
Author

Choose a reason for hiding this comment

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

I have changed the variable as ranks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why use the plural form ranks (instead of rank)?

Copy link
Author

Choose a reason for hiding this comment

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

I use the ranks because it stored many items . (2-9) that is why .
shall i change to singular ?

Copy link
Contributor

@cjyuan cjyuan Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure why you think ranks store many items.

If card' is "2♥", then after let ranks = card.slice(0,-1);, ranks will become "2".
It only stores the substring that represents the rank of a card. Its value depends on the value of card, but it does not store more than one string value.

On the other hand, numberCards is a good name choice for a variable storing an array of multiple strings.

It is up to you whether to change it or not. Just hope you get my point.

Copy link
Author

Choose a reason for hiding this comment

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

noted! ..
I have change the variable .

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 18, 2025
@sarawone sarawone added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 19, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Mar 20, 2025

Changes look good. Well done.

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 20, 2025
@sarawone
Copy link
Author

Thank you so much for the reviews and feedback ..
I will change this PR to complete ..

@sarawone sarawone added the Complete Volunteer to add when work is complete and all review comments have been addressed. label Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjyuan cjyuan cjyuan left review comments

Assignees

@sarawone sarawone

Labels
Complete Volunteer to add when work is complete and all review comments have been addressed. Reviewed Volunteer to add when completing a review with trainee action still to take.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sarawone @cjyuan @illicitonion