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

docs: Improve clarity for Radix and complete AVL docs #77

Merged
merged 3 commits into from
Feb 9, 2024

Conversation

4ndrelim
Copy link
Owner

@4ndrelim 4ndrelim commented Feb 8, 2024

No description provided.

Copy link
Collaborator

@yeoshuheng yeoshuheng left a comment

Choose a reason for hiding this comment

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

Have only reviewed the portion on Radix sort.
Have a few suggestions, but they don't need to be followed :)

LGTM.

@@ -372,6 +372,10 @@ private T successor(Node<T> node) {
return null;
}


Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add this to all the current implementations (eg. segment portions of code based on it's purpose) for consistency?

// Bit masking here to extract each segment from the integer.
int mask = ((1 << NUM_BITS) - 1) << (segment * NUM_BITS);
return (num & mask) >> (segment * NUM_BITS);
// bit masking here to extract each segment from the integer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I've been thinking - for students taking CS2040S, they have probably not touched CS2100 yet, so the concept of bit representation might be a bit complex for them.

Would it be better to maybe insert more helpful links regarding how computers represent numbers (eg. float, integer).

Maybe we could add this link: https://cheever.domains.swarthmore.edu/Ref/BinaryMath/NumSys.html or somethign else to help clarify things.

Copy link
Owner Author

Choose a reason for hiding this comment

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

added

Copy link
Collaborator

@yeoshuheng yeoshuheng left a comment

Choose a reason for hiding this comment

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

AVL Portion LGTM too.

Copy link
Collaborator

@kaitinghh kaitinghh left a comment

Choose a reason for hiding this comment

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

LGTM!

@4ndrelim 4ndrelim merged commit 0cff81c into main Feb 9, 2024
1 check passed
@4ndrelim 4ndrelim deleted the branch-RefactorAVL branch February 10, 2024 15:06
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