Hey @Rainbolt @murgatroid99, I have two new versions of the ManaCost/ Symbol implementation, let me know what you think! I also included a list of the benefits of each system (and links to the two branches) here: github.com/c0d3br34k3r/magic/issues/4#issuecomment-100300996
@codebreaker At a quick glance, I already prefer solution 2
Primarily because of this:
> We don't have to do two different things (other solution makes you check the Numeric and Repeatable symbols) every time we want to do a calculation, such as converted mana cost.
This is important not just for ease of use but for extensibility
@codebreaker here's one option: make a HybridSymbol class that has 2 colors (not symbols, just colors), and a MonocolorHybridSymbol class that has 1 color, and always returns 2 for CMC
The fact is that it's extraordinarily unlikely that they will make any other kinds of hybrid symbols
@murgatroid99 It's true, I could do that, and that's sort of how I did it in my initial version, I just like the idea of having TwoPart symbols that are actually made up of two different Symbols.
The way I see it, that's their problem. If it implements the interface, it should work. The fact that they're using your library in a way that you didn't intend isn't your problem
And I would suggest that searching for exact symbol matches is less important than costs matching a color, costs payable by a set of colors, and costs with a specific converted mana cost, all of which are O(n) anyway
@codebreaker Please understand that I'm not just trying to tear down your vision, or whatever
Also, your NumericGroup thing seems kind of like overkill to me. As far as I can tell, you only use it once, in MonocoloredHybridSymbol, to represent the number 2
That's part of why I suggested MonocolorHybridSymbol as a separate class entirely
For parsing, with the ManaCostElement I suggested earlier, you could basically tokenize to get each {<something>} element of the string, then pass it to ManaCostElementFactory
ManaCostElementFactory or ManaCostElementParser or whatever you call it would still need a significant switch statement, but overall I think it would be simpler
Actually, you probably wouldn't need to go that far. The ManaCostParser class could just have a function that returns a ManaCostElement for a given string
and, at that point, maybe it would be better to make parse a class method on ManaCost.
I'm mostly just thinking out loud here (so to speak)
A SymbolParser class might be useful, and mana cost parsing would basically just be doing that in a loop, so it probably wouldn't be too much more complex
Yeah, I think it would be good having the logic of what text corresponds to what symbol in its own class
I think that what I was trying to get at before with the MonocoloredHybridSymbol thing is that {2/W} can, in a sense, be defined as a symbol with color=White, CMC=2, payable with anything.
If you have it be an implementation of Symbol that defines those methods in that way, then that's all you need
One benefit here is that the implementation of each of the Symbol methods in each implementer is dead simple, and almost always returns either a constant or an instance variable
Oh, you're probably going to want a Symbol.devotionTo(color1, color2) because it's not exactly the same as devotionTo(color1)+devotionTo(color2)
if you have a cost {R/G}{R/G} it adds 2 to your "devotion to red", 2 to your "devotion to green", and 2 to your "devotion to red and green" That's what I was talking about
@codebreaker otherwise, yes, you can do that
I think, though, that it would be better to have that logic in each Symbol/ManaCostElement so that the logic in ManaCost is very simple, just add up the devotion of each element
If you do it right, just about every method on ManaCost ends up as the sum or union of that method applied to each element
The one thing that's a bit of a pain is that if you want to use ManaCostElement(Generic, 0) for {0}, you probably want to special case 0 in other ManaCostElement methods
Otherwise ManaCostElement(Color(White), 0) is a payable White mana cost with CMC 0, which is strange
Validate that count>=0 if Symbol is Generic, and that count>=1 otherwise
And then do a layer of validation on top of that in ManaCost
Though, maybe {0}{W} should not be an error
I think it's not really necessary to make it one
@codebreaker The other benefit of my suggestion over your second solution is increased type safety. It is impossible, with that suggestion, to create a symbol that does not appear on a real Magic card
Board and Card Games
Board and Card Games