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

Branch: 17557-added isAbstract to Boolean, Fixes #17557 #17974

Open
wants to merge 1 commit into
base: Pharo13
Choose a base branch
from

Conversation

sanyakapoor27
Copy link
Contributor

@sanyakapoor27 sanyakapoor27 commented Mar 10, 2025

Fixes #17557

  • Class Boolean returns True now on Boolean isAbstract.
  • Opened a new PR to fix branch related issues and ensure cleaner commits.

@ErikOnBike
Copy link
Contributor

ErikOnBike commented Mar 12, 2025

Hi @sanyakapoor27
Nice to see your PR!
The issue (as well as fix) might seem pretty simple at first. You've chosen an implementation which is not uncommon in Pharo (although you could also have chosen self == Boolean to prevent having to access the class' name and a slightly more common approach, although I didn't actually count the occurrences).

An interesting approach could also be, to NOT perform any checks/comparisons. We know there are exactly 2 subclasses of Boolean. Boolean's have a very interesting way to help us have something other languages call 'conditionals'. The ifTrue: and ifFalse: methods are implemented in such a way that we do not need to check for the condition itself. Could you come up with a solution without performing any actual comparison?
Just answer/put a response here, don't update the code just yet! :-)

@sanyakapoor27
Copy link
Contributor Author

Hi there @ErikOnBike !
Thank you for your feedback! To avoid using any comparisons or checks, could we create Boolean isAbstract and have it return true while overriding it in True & False classes to return false? Please let me know if this aligns with what you're thinking of, otherwise I'll think of another solution! thanks :)

@ErikOnBike
Copy link
Contributor

That was indeed my thought. In line with the instance side implementation of Boolean methods.

The interesting result is that the methods are easy to understand, very small and fast to execute. The fact that 3 methods are needed instead of 1 can be considered less efficient from source code perspective though. I think that is not a big problem and ease of understanding weighs heavier.
To be honest, your code is also not very difficult to understand :-D.

But please let someone else make the decision. I'm not sure my idea is appreciated by others. Let's wait for some more feedback.

@jecisc jecisc closed this Mar 17, 2025
@jecisc jecisc reopened this Mar 17, 2025
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.

Class Boolean should be abstract
3 participants