They get Forge card to learn all sets where card was printed

[Max mtg]

http://svn.slightlymagic.net/websvn/dif ... &peg=18604

Why don't you get cardrules and query sets the card was issued from there?

[Agetian]

This is exactly the thing I'm always curious about when I'm about to implement something - I have to dig through the code manually looking for a way to do something, and usually there's more than one way to do something (commonly on the order of at least three or four different approaches), and no indication as to which one is better or easier. The easier/preferred one is also not always the most obvious one to find, sometimes it's highly possible to do something "the hard way" and miss the easy and preferred way out.

When I was about to join the team, I asked around if there was any documentation for the Forge code base, as well as any style guidelines or recommendations, but outside of what was documented on the wiki (and things like these weren't), I was basically told there was no other documentation or recommendations anywhere, so outside of whatever recommendations are given to me here on the boards (and I'm always very grateful for them and I try to make the most out of them), I have to find my own way of implementing some feature or another.

As such, I'm commonly implementing things the way I find them to be implementable. If you know of a more optimal way to implement something, feel free to correct me and notify me of it, I'd be grateful for your assistance, but your bitter irony and sarcastic tone is not exactly warranted if we're talking about nearly 180,000-line code base with virtually zero documentation and you haven't volunteered to help out, document it somewhere publicly for everyone (including potential future members who could join) to see, or at least point in the right direction for wherever it may be already documented (and in this case, it just isn't).

As you probably and hopefully know, I'm always willing to cooperate *and* I'm always looking for the more optimal ways of doing things. Personally, I hate suboptimal code. And even more I hate the fact that I have to resort to suboptimal code every now and then inadvertently, simply because I have no idea whether there's a more optimal of doing it or not and if there is, what it is exactly. I have to learn from my own mistakes, and I make sure that I'm learning as I go along. I also don't have any problem correcting my previous implementations where it's due. The problem is that locating optimal ways among a myriad of classes and a hundred times more methods most of which don't even have (any or at least up to date) javadocs can be a rather big issue.

If you're willing to help, by the way, and if you're afraid of suboptimal code, please take a look at the sideboarding WIP thread before I flood the SVN with more suboptimal things relatively soon, and I can pretty much guarantee that, minus help and recommendations, they will be suboptimal (or, rather, not optimal at all) because, like I honestly said there, I have no idea what the optimal implementation would be and even if the current Forge code base has ways of coming up with one easily. Also, at least at the moment of this writing, no one has come up with suggestions about coding yet, and if no one does, I'll have to invent my own ways of doing things which are likely to be ridiculous from the point of view of an experienced long-time Forge coder. Long story short, it's a perfect spot to help out before you have to clean up a ton of badly written code after me and spend time posting more ironic remarks.

P.S. Just out of curiosity, I checked out CardRules and indeed found out that I could get a list of sets the card was printed in that way, but that does not answer the question how the random set code from that list can then be assigned to the card without converting it to Card via toForgeCard() anyway because neither CardPrinted nor CardRules has the appropriate mutator like setCurSetCode or anything like that - however, converting to Card is, I believe, something you wanted me to avoid. Please, at least make your explanations complete if you're going at your teammates like that for supposedly bad code *and* actually want them to make it better. I don't mind constructive criticism at all, moreover, I welcome it, but I've never really understood the mere "look at how stupid this is" approach.

P.P.S. Speaking of card classes - at this moment, the exact necessity for and difference between the card classes (there's Card, there's CardPrinted, there's CardRules, but there are also CardCharacteristics and InventoryItem and probably more), as well as interaction between them and the necessity of mutual conversion, as well as the same deal for all the different deck-related classes, are mostly mysteries for me.

- Agetian

[Max mtg]

That's all about habits. Learning how the code works from documentation is a common strategy, but quite unreliable. I don't understand what would stop one from opening source code of a class he is going to use. That's a great way to discover what the class really does and how it can be used in a more effective way.
This approach also makes you more independent from documentation and... people who don't volunteer to share their knowledge.

It is always a good idea to evaluate every approach that might solve the problem and learn which side effects each implementation would create, how much memory and computing time that would take... or ask here at least.
One might also search on how a similiar problem was solved in different parts of project.

In this specific case creating or copying a Forge Card is really expensive (just see what happens as toForgeCard() is called), that means slower loading times and worse player experience.

I don't intend to offend anyone, sorry if my post has.
Yet I would love people to read more of source codes and learn some better approaches without external help or guidance.

PS: once you have a collection of editions, if it's enumerable (get a keySet from map) Aggregates.Random will return you a random set. CardPrinted instances are immutable, and every possible non-foil card is created at startup when all the rulefiles are read. Thus you will have to request a matching cardprinted instance from CardDb querying it by name and set.

PPS: CardPrined = {name+set+artindex+isfoil} - the least info needed to refer to a card, decks hold these structures.
CardRules used to hold most of information needed out of game (deckeditors, the basic deck generation we had at that time). Innistrad made us (HellFish implemented that I guess) move each side of card to special Characteristics classes
Card is ingame class with all things like sickness, damage dealt, controller and many-many more.
Yet you could also learn it all from source code.

There's also a thread with discussion on how it should be dating back to the time when the cardprinted class was written. viewtopic.php?f=52&t=5240

[Agetian]

Yes, it's true that "the source code should speak for itself" or however that goes, but I think that nothing would beat a good documentation or at least directions/javadocs around the code when the code base is big enough. When Forge grows from 200,000 to 1,000,000 lines (for instance), it's going to be even more difficult to find things and make sure they're optimal. If all we do is scare away newcomers by mocking them and telling them they're stupid, I don't think anyone will be coding Forge in five or seven years from now, at least except for the several old-timers who would hopefully still be around (and I don't want to see a thing like that happen because I really want the project to evolve).

Yet I would love people to read more of source codes and learn some better approaches without external help or guidance.

We all want everyone to be perfect, don't we? This is basically what I'm trying to do, only the degree of success varies from one attempt to another. It's surprising how different approaches to achieving the same thing may actually already be found in different parts of Forge. What might seem like a valid approach in one place could actually not work at all in another, and vice-versa. I didn't take the toForgeCard() conversion out of nowhere, in fact, I saw it elsewhere in the code. And in fact, I can even quote you the exact place I got it from - prepareSingleLibrary in GameNew.java, which converts a CardPrinted into a Card with a toForgeCard call to make modifications to the card art, and then adds that to the library. I did not write that, but I borrowed that as a strategy valid for solving another task because it was already used by someone and as such seemed legit. The number of toForgeCard calls involved there was also roughly similar to the number I was going to use, because it was used in the iteration over the deck. Perhaps it's the only way of doing it there, perhaps it's also suboptimal and I'm not the only one who didn't know about "how it should be done", or perhaps it's even optimal in that particular case and just my case is too different from that one, but it's not immediately evident from the code, and in order to make sure it will work in my case I'd have to read the entire Forge source, keep in mind all the class interactions (some of the names of the classes are not even exactly intuitive, I mean, I would not even have had an idea of looking inside CardRules for the set information because, judging by the name of the class, it's not what would hold such information; it's also not very intuitive that getCard() method of CardPrinted unexpectedly returns CardRules, not Card, and the two are drastically different, but... meh). Also, I'd need to read the entire history of threads here and make sure that all my findings are even still valid because certain methods and interfaces change every N months dramatically enough to invalidate previous discussions at least in part. In other words, I would have been stuck just reading for months to come, and there would have been still no guarantee that my findings would not have misled me in any way even if I did decide to do it.

To summarize it, I do believe in the "clean source is self-explanatory" and "read the source, Luke" idea, but I think that the value of these expressions diminish with every extra zero in the number of lines of code the project consists of, and when it gets to six-digit and especially seven-digit numbers, nothing beats at least several lines of quick documentation spread around the code. If every person writing Forge spent some time adding at least one short javadoc line to each method/class he or she writes (I do, just in case), I think working with the Forge code base would have been a much more intuitive and less frustrating experience, especially for people just joining.

Anyhow, I think I've spent enough time on this discussion, and I believe I could have best spent it on digging through the code and starting to implement sideboarding, because no one else is doing that and because no one else has responded to my questions about coding recommendations there, so I'd best start experimenting. Whatever monstrosity you're going to see as a result of that, by the way, is going to be the direct result of "reading the code, trying to learn from it, and then coding using whatever seemed to be the perfect approach" strategy, which, as you can see, never works perfectly, at least for me.

Thank you for the explanations, they did make certain things clearer for me. Also, I updated my former code using the strategy you recommended, hope it's OK now. Like I said, I have no problem optimizing when I'm told to.

- Agetian

[Max mtg]

Well, my personal opinion is that the 200k lines we have is not really a large project. Used to work at 800k project written in C++ with no documentation. The coding policy said "choose method and parameter names so that they don't require extra description." The knowledge the lead developers shared at a whiteboard was about subsystems and architecture that game had.

prepareSingleLibrary in GameNew.java, which converts a CardPrinted into a Card with a toForgeCard call to make modifications to the card art

That's reasonable there (and only there) - this code is building a library for the game being initiated. It's a right place to make forge.Card classes out of CardPrinted ones. These are the cards to be given to players and enter the battlefield.

I don't like the idea of aspiring to perfection, because it's unreacheable. Prefer the idea of improving skills while remembering that you can be highly qualified but will never be perfect.

getCard() method of CardPrinted unexpectedly returns CardRules

Looks like a strong reason to rename that method into getRules()

the value of these expressions diminish with every extra zero in the number of lines of code the project consists

It's impossible to embrace the whole project at once. But once you focus on the source of the area you are to improve, the number of lines drops to some 3 to 4 digit numbers.

If every person writing Forge spent some time adding at least one short javadoc line to each method/class

I don't know what to say here. As for me the method signature is enough. If there remain questions, look at method's source code.

Don't think any newcomer has to look through the history of threads. It's old timers' task to find a thread answering some common questions.

Thank you for the explanations, they did make certain things clearer for me. Also, I updated my former code using the strategy you recommended, hope it's OK now. Like I said, I have no problem optimizing when I'm told to.

Thank you, it's much better now.

[Agetian]

I agree that 200k is not too much. Linux, for instance, has a 8-digit number for the line count if I'm not mistaken.
Yeah, sounds like getCard() should be renamed getCardRules() or getRules() because it's confusing.

OK, I'll see if I'm any luckier with my source code inspection in the future.

- Agetian