-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Introduce Order as a return of <=>
instead of Int32
#2913
Conversation
@@ -0,0 +1,53 @@ | |||
# Order is the order between two objects. It returns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"It is used when"
Not sure I agree, one can do |
|
Currently, many language uses integer value as a return of compare function/operator. However, this is a vice coming from C because it makes some magic numbers -1/0/1. I and almost all programmers hate magic numbers as you know, so I introduce Order enum class. It has three values LT/EQ/GT, they can replace -1/0/1.
43ebe51
to
722aa1b
Compare
I updated some documents. If you google which And I have a question, how many costs is |
I mean, instead of writing I think your change is good, I just need a bit more time to think it. The first message was my first reaction to it 😊 |
I think it is saner this way. |
@rvion I have not understood meaning difference between And, I think |
@makenowjust: I have no strong opinion, but here are my 2 cents
When
I don't think it's too long at all. First of all, few people will need to type it. It's only usefull when crafting custom sorting functions, or checking if a traversable container is sorted. There will be no need to type
Anyway, I also think I initially +1'd your proposal because if crystal use ints to encode ordering, people will make bugs in their application code. when I create a custom class, I would like to be able to easilly write a |
I just added record Person, name : String, age : Int32, height : Int32, weight : Int32 do
include Comparable(self)
def <=>(other : self)
(name <=> other.name)
.eq_or { age <=> other.age }
.eq_or { height <=> other.height }
.eq_or { weight <=> other.weight }
end
end However, I am not sure |
@makenowjust: sincere question: why do you think that's usefull ? if you want to check that a list of conditions are all matched, why not using I would -1 this function (also, I agree that naming eq_or is not great) |
@rvion Sorry, I used meaning of "order" as "lexicographical order", but you didn't. It caused a mistake. When you implement lexicographical order, You want this? require "partial_comparable"
record Person, name : String, age : Int32, height : Int32, weight : Int32 do
include PartialComparable(self)
def <=>(other : self) : Order?
{% begin %}
{% props = %w(name age height weight).map &.id %}
return Order::EQ if {{ props.map { |p| "#{p} == other.#{p}" }.join(" && ").id }}
return Order::LT if {{ props.map { |p| "#{p} <= other.#{p}" }.join(" && ").id }}
return Order::GT if {{ props.map { |p| "#{p} >= other.#{p}" }.join(" && ").id }}
nil
{% end %}
end
end (But I think to need such a case is rarer than to need lexicographical order because it is not totally ordered.) |
After this long discussion, my feeling is that it's much easier with numbers:
Basically, always take the meaning as comparing the number to zero. To sort in a descendent way you invert the order: just apply Less classes and named involved and needed to be remembered. So my vote still stays 👎 for this change. |
@asterite Yes, this memorization is right, but we are not learning for an exam of C now. I have one question: where do you write this in a document? Don't tell me this should be written in Most important thing is "ordering is not just a integer value." . For example, now def <=>(other : self) : Int32
r = major <=> other.major
return r if r != 0
r = minor <=> other.minor
return r if r != 0
r = patch <=> other.patch
return r if r != 0
pre1 = prerelease
pre2 = other.prerelease
prerelease <=> other.prerelease
end By This PR, I can rewrite: def <=>(other : self) : Int32
(major <=> other.major)
.eq_or { minor <=> other.minor }
.eq_or { patch <=> other.patch }
.eq_or { prerelease <=> other.prerelease }
end If we have |
@makenowjust We can also do: {major, minor, patch, prerelease} <=>
{other.major, other.minor, other.patch, other.prerelease} |
@asterite Ah, it looks good. We can call |
@makenowjust I'm just not sure the benefits are much. Plus it'll break all code out there. But I'd like to know others' opinions. |
{major, minor, patch} <=> {other.major, other.minor, other.patch} ❤️ |
Conceptually it's better, but as always I'm one of those cycle-squeezers; what does a benchmark say? (aside that it's just a flip of the coin, breaking code and all..) |
This PR seems like style bikeshedding and Sayre's Law, over-engineered code-churn. |
Closing as the consensus seems to be that using ints to model results in more idiomatic code. Ironically I do like having a specific Order type :P, but Crystal tries not to introduce types unless there's a very compelling and pragmatic reason. |
Currently, many language uses integer value as a return of compare function/operator. However, this is a vice coming from C because it makes some magic numbers
-1
/0
/1
. I and almost all programmers hate magic numbers as you know, so I introduce Order enum class. It has three valuesLT
/EQ
/GT
, they can replace-1
/0
/1
.