convert MidiEvent to data class #34
Conversation
|
I can also convert other classes to data classes but I haven't used the library that much and I don't know what classes to change |
|
Thanks for this PR too! At this state I have to say I'm rather negative to this change. Data class would mean semantic changes on equality like you suggested in the code, and that is not efficient for arrays which are of not fixed length. If I understand data class correctly, one guideline on whether a class should be a data class or not would be that if it is not efficiently comparable then it is probably not for data classes. It is also a breaking change so I will have to make various changes to my apps. IF data classes bring in significant, not just nominal, improvements, then I would go for it though. But I wasn't given any proof so far.
I'm not familiar enough with how kotlin compilers and linkers work so correct me if wrong, but my idea was that those readonly fields cost extra space on memory while getters don't. (It was how .NET worked when I ported this library from C#.) |
| } else if (other.extraData != null) return false | ||
| if (extraDataOffset != other.extraDataOffset) return false | ||
| if (extraDataLength != other.extraDataLength) return false | ||
| if (statusByte != other.statusByte) return false |
There was a problem hiding this comment.
The rest of those fields are part of value so there is no need to compare them.
| result = 31 * result + (extraData?.contentHashCode() ?: 0) | ||
| result = 31 * result + extraDataOffset | ||
| result = 31 * result + extraDataLength | ||
| result = 31 * result + statusByte |
I guess I'll close the PR for now but I might get back to it later if I figure this out. Right now I feel like you are right and this class shouldn't be a data class.
Yes, that's right. I just thought it's not that important here. |
|
If you do not mind I would rather reopen the PR and leave it as is, it's better not have it hidden.
It is kind of :-) MIDI messages could be thousands on a song. Also IN CASE there are people who try to get closer to realtime MIDI processor, people would like to have MIDI buffers that never allocates smaller. |
Ok! I might return to it later
Thanks for the explanation! |
|
In my recent planned changes in ktmidi v0.6, |
|
looks fine to me! |
|
Thanks. The changes in |
#31
It's mentioned that the class is complicated but I feel like it's ok to make it a data class. I don't know why getters were used on some properties but I removed them, I'll get them back if needed