Skip to content

Conversation

@pankaj-bind
Copy link
Contributor

@pankaj-bind pankaj-bind commented Mar 25, 2025

Issue #21: About NilNode
Problem: CTAVLNode methods like childrenDo:, do:, height, and isBalanced contain explicit ifNil: checks, which you noted could be avoided with a proper Null Object pattern.
Fix: Enhanced CTAVLNilNode to implement all necessary methods (e.g., childrenDo:, do:, height) with no-op or default behavior, and removed ifNil: checks from CTAVLNode methods, relying on polymorphism.

Issue #23: Can we get rid of isNilNode
Problem: CTAVLTree >> size uses isNilNode to check if the root is a CTAVLNilNode, which is unnecessary with a Null Object pattern.
Fix: Removed isNilNode from the class hierarchy, introduced a polymorphic nodeSize method (0 for CTAVLNilNode, recursive count for CTAVLNode), and refactored CTAVLTree >> size to simply call root nodeSize.

Issue #24: Can we get rid of AVLTree >> isNil
Problem: CTAVLTree >> isNil checks if the root is a CTAVLNilNode, which you questioned as redundant for a tree’s API.
Fix: Removed CTAVLTree >> isNil, relying on isEmpty (inherited from Collection) which uses size = 0, made possible by the nodeSize refactoring.

@pankaj-bind
Copy link
Contributor Author

Hey @Ducasse, I'm aware of the issue I created previously (#19), but I found that it may cause a conflict with this Nill issue. That's why I thought I should resolve this one first.

@Alokzh
Copy link
Collaborator

Alokzh commented Mar 25, 2025

Hi @pankaj-bind Thnx for the PR but I was already working on explicit check issue ( as discussed in #22 ) & was almost ready with PR.

However if Stephan thinks this implementation is ok we can merge it.
Also some suggestion I think we should only have one approach either childrenDo: or children/withAllChildren: , I think childrenDo: is better one.

@pankaj-bind
Copy link
Contributor Author

@Alokzh if you think you can make it, you are good to go buddy, @Ducasse please look for his PR. I'm closing mine.

@Alokzh
Copy link
Collaborator

Alokzh commented Mar 25, 2025

@pankaj-bind I don't have any problem with your PR ( just inform from next time that you are interested ).
Why did you close it ? Only few changes were needed in your PR I think

@pankaj-bind pankaj-bind reopened this Mar 25, 2025
@jordanmontt
Copy link
Member

So how de we proceed? Do we merge this PR? @Alokzh @pankaj-bind ?

it looks good to me

@akevalion
Copy link
Collaborator

Could we use a singleton for this code?

CTAVLNilNode new

to

CTAVLNilNode uniqueInstance

@pankaj-bind
Copy link
Contributor Author

@jordanmontt wait I must check concern of @Alokzh and @akevalion

@Alokzh
Copy link
Collaborator

Alokzh commented Mar 26, 2025

Looks good to me Jordan

@pankaj-bind
Copy link
Contributor Author

Hey @akevalion yes you could use a singleton for CTAVLNilNode to save memory, but it’s not necessary and risks future complexity unless justified by performance needs.

@akevalion
Copy link
Collaborator

Looks good for me, I will merge it

@akevalion akevalion merged commit e1d0dae into pharo-containers:main Mar 26, 2025
2 checks passed
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.

4 participants