string parsing crashes

All code submission.
mnguyen
Posts: 229
Joined: Thu Apr 29, 2010 4:13 pm

Re: string parsing crashes

Post by mnguyen » Mon Nov 15, 2010 4:46 pm

Could we also suggest at least one blank lines between methods? In practice, I use two but one should be sufficient. I find that when methods get spaced too close together it gets harder to differentiate the beginning and ending of a method. They sometimes look like they are part of the method prior to it.

-Mike

mnguyen
Posts: 229
Joined: Thu Apr 29, 2010 4:13 pm

Re: string parsing crashes

Post by mnguyen » Mon Nov 15, 2010 4:59 pm

In my experience, commenting out code is useless in a source controlled environment. Unless there is a specific reason for doing so commented out code should be deleted from the source. If kept in, there should be an explanation as to why it was kept in. There are a few places in the code where entire method implementations have been commented out without any explanation. Leaving them in can cause confusion as to the purpose of the method. You are forced to ask if the code was abandoned or set aside for the next installment. The latter argues for commenting, the former for removal.

How do we handle these types of cases? Leave them alone? proactively remove them?

wololo
Site Admin
Posts: 3727
Joined: Wed Oct 15, 2008 12:42 am
Location: Japan

Re: string parsing crashes

Post by wololo » Mon Nov 15, 2010 10:55 pm

mnguyen wrote:Could we also suggest at least one blank lines between methods? In practice, I use two but one should be sufficient. I find that when methods get spaced too close together it gets harder to differentiate the beginning and ending of a method. They sometimes look like they are part of the method prior to it.

-Mike
I don't think we should make that a rule, but I agree that one line space sounds good.
For the confusion, your IDE should be able to help you knowing immediately in which function you are.
mnguyen wrote:In my experience, commenting out code is useless in a source controlled environment. Unless there is a specific reason for doing so commented out code should be deleted from the source. If kept in, there should be an explanation as to why it was kept in. There are a few places in the code where entire method implementations have been commented out without any explanation. Leaving them in can cause confusion as to the purpose of the method. You are forced to ask if the code was abandoned or set aside for the next installment. The latter argues for commenting, the former for removal.

How do we handle these types of cases? Leave them alone? proactively remove them?
I like what you said. Proactively remove dead code, unless it comes with a relevant explanation(such as: //this is pending fix of bug 123....)

mnguyen
Posts: 229
Joined: Thu Apr 29, 2010 4:13 pm

Re: string parsing crashes

Post by mnguyen » Tue Nov 16, 2010 12:13 am

wololo wrote:
mnguyen wrote:Could we also suggest at least one blank lines between methods? In practice, I use two but one should be sufficient. I find that when methods get spaced too close together it gets harder to differentiate the beginning and ending of a method. They sometimes look like they are part of the method prior to it.

-Mike
wololo wrote:I don't think we should make that a rule, but I agree that one line space sounds good.
For the confusion, your IDE should be able to help you knowing immediately in which function you are.
Well actually confusion is the wrong word. I think of reading code like reading an essay or page of text. Paragraphs for the most part are spaced out by at least some space or some visual cue. I see method implementations the same way. True, the editor will differentiate what method you are viewing but why would you need to look up if you can visually see the break? As you say, it doesn't have to be a rule. I just wanted to voice my opinion about what I've seen and done.
mnguyen wrote:In my experience, commenting out code is useless in a source controlled environment. Unless there is a specific reason for doing so commented out code should be deleted from the source. If kept in, there should be an explanation as to why it was kept in. There are a few places in the code where entire method implementations have been commented out without any explanation. Leaving them in can cause confusion as to the purpose of the method. You are forced to ask if the code was abandoned or set aside for the next installment. The latter argues for commenting, the former for removal.

How do we handle these types of cases? Leave them alone? proactively remove them?
wololo wrote:I like what you said. Proactively remove dead code, unless it comes with a relevant explanation(such as: //this is pending fix of bug 123....)
not in this edit but as I come across them later I will remove dead/commented out code without explanations. I want this initial commit to be only about reformatting.

Zethfox
Posts: 3022
Joined: Thu Jun 10, 2010 11:28 pm

Re: string parsing crashes

Post by Zethfox » Tue Nov 16, 2010 6:23 am

i want to start by saying, sense ive learned C++ coding from decryptic this source, i think my points are 100% valid and with reason.
readablitiy of this sources many many misnamed varibles, functions, code words, extreme uses of shorthand senseless butchering of varible names simply because the person didnt want to type 3 extra letters.was an absolute NIGHTMARE for me as a beginner...thats the disclaimer.


** 6) any maximum number of nested blocks depth? (this is of course flexible..it means the coder will be strongly encouraged to refactor when this happens)
> No.
Though I think 3 is a reasonable average, and I'd live with it. It's a nice rule of thumb, but I see little point in stating it as a coding standard. If you have someone dumb enough to write a 5-nested unreadable loop when there are better ways to write it, I do not think the rule will make that person rewrite it in a more sensible way - they'll probably just make huge functions with a lot of calls, try to unroll the loops and generally deal with it in a syntactic manner, which won't help at all.

LOL i HAD to laugh at this. careful as i think youre calling wololo and possibly his mentor "dumb"
reffer to Aiplayer.cpp and GuiCombat.cpp for the most epic nested loops ever.

and before anyone even thinks of stating that this is perfectly understandable code

Code: Select all

        for (inner_iterator it = attackers.begin(); it != attackers.end(); ++it)
          if ((*it)->card == event->card->previous || (*it)->card == event->card )
            {
              AttackerDamaged* d = *it;
              if (activeAtk == *it) activeAtk = NULL;
              attackers.erase(it);
              trash(d);
              return 1;
            }
          else
            for (vector<DefenserDamaged*>::iterator q = (*it)->blockers.begin(); q != (*it)->blockers.end(); ++q)
              if ((*q)->card == event->card->previous || (*q)->card == event->card)
                {
                  DefenserDamaged* d = *q;
                  (*it)->blockers.erase(q);
                  trash(d);
attackerdamaged is actually the blocking creature, defenserdamaged is the ATTACKING creature.
activeATK has no real life definition. theres no comment on what it actually is.

and this is just a TINY peice of that code.



on that note, i have a request for coding guide lines.

no cryptic naming of varibles.
activeATK <---should be something that means something.
activeAttacker ???? doesnt this make more sense?

if (result) stuffHappened = 1; <--- NO....just NO....it might have seemed cute at the time but if we're going to turn this into some sort of text book quility coding, then things like this need to better reflect EXACTLY what this varible is.


mObjects <----menuObjects? why cut off the letters "enu" and turn it into some sort of code word.


kAlternativeKeyword <---do these varibles really need to have "K" infront of them...if not remove them or fill in the missing letter.
k"-----"AlternativeKeyword

mScale = mTargetScale; <---menuScale? mainScale? what is this m for?

mParticleSys->Update(dt);

mParticleSys =======>menuParticleSystem......Please.....

stw->countSpellsPerCostAndColor[convertedCost] <----the function name in red is perfect...however inspect the item in bold....not so perfect.
stw should be a human readable varible or class name. not this.

mTrans <-----this......


if (tc) =========> if (targetChooser) ....please....

sumDamages() <---i would put money that everyone except maybe *maybe* wololo has no idea EXACTLY what this function does....i can tell you right now, it has NOTHING to do with what you read...sum damage....no.....this type of function naming needs to stop.

doDamageTest <---the naming of this function has always bothered me, since it is also another case of it being "not exactly" what you read.

MTGRemovedFromGame * removedFromGame; <----this needs to be removed and/or renamed into Exile. the "removedFromGame zone" and exile, was a nasty nasty pointing setup....instead of changing removedFromGame zone into exile the person who recoded this simply pointed exile to it...


Lands selfLands ....myLands? why use selfLands, this doesnt make sense in english.
which will lead me to another thing, i notice alot of nonenglish terms used in the SVN, varible namings ect...the source is clearly written in english, it should be consistent through out. Defenser for exsample....its not the blocking creature if thats what you think....a creature that is defenser means it was blocked. can you see where not being consistent in english use can lead to confusions?

WSrcCards <----no....just no....


dest =====>destination

mLayers? menuLayers?

wEvent ===>worldEvent? if possible these 1 letter short hands should be changed into EXACTLY what they are. not only does it make it MUCH easier to read, but you wont have to go searching for the actual meaning of the short hand, and you wont have to guess.


lcname <----10 dollars if anyone other then wololo can honestly tell me the meaning of "lc" WITHOUT looking in the source.

function name "magicTexts" and function varible "magicText" niether are really what they appear. yes they deal with cards abilities, however not exactly as you think they would.

useExpandedCardNames <---this is the type of varible naming and function naming we should be aiming for...notice. i can see EXACTLY what this is doing just by reading it. it is "using expended card names" tho the expanded part kinda makes no sense, this is much better then alot of things i see in source.

stillInUse(MTGCardInstance * card){return 0;}; <---this is not really what you think it is just by reading it, varibles/functions named like this that do something even the slightest bit different need to be renamed to better reflect EXACTLY what they do.


KeybGrabber <---the grabber of the "b" key? how about we just call it "KeyBindGrabber" which is what this ACTUALLY is.

remaskBlkViews <----Blk? Blk in the surrounding context is "Blocker" ....why do i have to guess if Blk means blocker here?

removeOne(DefenserDamaged* blocker, CombatStep); i will tell you what this does....then i will let you decide...remove a assigned blocker from the attackers blockers.list.....

BLK, ATK, OK, NONE<--------what?

storeCommonAttribs() ======>storeCommonAttributes() ???isnt this much better?

getDoneTasks <--getFinishedTasks?

nbminuses..........no

abf.parseCounter<-----just no......

ABF seriously???? AbilityFactory.parseCounter ...doesnt this look MUCH better?

NB_BASIC_ABILITIES i see this NB ALOT....what exactly does NB mean? nbcards, nbZones....nb????i cant think of a single english acronym that includes nb except "notbound" that would make sense in the context its being used. this needs to be changed into something a little easier to understand a first glance.and not another acronym.

with this said, there are MILLIONS of of exsamples, but i think i made my point pretty clear.
the use of short hand, nicknames, and acronyms should be forbidden we should start to move to a clearer more transparent source, one that would have made my life and learning C++ ALOT easier.
knowing what i know now, it makes me angry to see that you can call a varible or pointer ANYTHING you want...it makes me angry to see that people were just too lazy to type out whole words and resorted to covering the source in acronyms and strange codeword shorthands...either lazy or trying to look like they were some kind of elitist....lets do this, lets make the source transparent.

wololo
Site Admin
Posts: 3727
Joined: Wed Oct 15, 2008 12:42 am
Location: Japan

Re: string parsing crashes

Post by wololo » Tue Nov 16, 2010 7:03 am

Zethfox wrote: LOL i HAD to laugh at this. careful as i think youre calling wololo and possibly his mentor "dumb"
reffer to Aiplayer.cpp and GuiCombat.cpp for the most epic nested loops ever.
We've done some things that I would call nasty in there for the sake of "short code". What some of us think is easy to read might not match other people's standards. I personally find the code you quoted quite hard to read, Jean might disagree.
If somebody can provide more readable code for this specific part, I'm of course open to discussion, and I think Jean as well.
Note that I see 3 nested blocks here, which is within the limit of what I suggested.

Let's keep in mind two important things here:
1) These rules are rules of thumbs for most of them. As I said, people who try to enforce them should be thanked, people who don't won't be shot in the head, and there might be times when we think the rules don't apply.
2) The current code should never be used as a "reason" to code like crepes. We all make mistakes, and the rules also change. What was ok one year ago might not be today. We are trying to adapt to the changes in the team and the size of our codebase. I also don't remember saying the current code was "good" or should be used as a "model". I myself learned C++ while coding Wagic, and there are lots of parts in the code where it shows. Coding alone (Wagic was a "one man" project for 18 months, you can imagine that's a lot of lines of code) is different from coding with a dozen people's team.

Zethfox
Posts: 3022
Joined: Thu Jun 10, 2010 11:28 pm

Re: string parsing crashes

Post by Zethfox » Tue Nov 16, 2010 7:38 am

i edited my above post QUITE a bit. atfirst it was going to be a simple comment about the nested loops, which btw in THAT code you see 3, in the Aiplayer there are exsamples that go as deep as 6+ loops....

please read my edited version as i feel your feedback is needed on it to see any action taken on it.

wololo
Site Admin
Posts: 3727
Joined: Wed Oct 15, 2008 12:42 am
Location: Japan

Re: string parsing crashes

Post by wololo » Tue Nov 16, 2010 8:21 am

with this said, there are MILLIONS of of exsamples, but i think i made my point pretty clear.
the use of short hand, nicknames, and acronyms should be forbidden we should start to move to a clearer more transparent source, one that would have made my life and learning C++ ALOT easier.
knowing what i know now, it makes me angry to see that you can call a varible or pointer ANYTHING you want...it makes me angry to see that people were just too lazy to type out whole words and resorted to covering the source in acronyms and strange codeword shorthands...either lazy or trying to look like they were some kind of elitist....lets do this, lets make the source transparent.
I overall agree, but read along
activeATK <---should be something that means something.
activeAttacker ???? doesnt this make more sense?

if (result) stuffHappened = 1; <--- NO....just NO....it might have seemed cute at the time but if we're going to turn this into some sort of text book quility coding, then things like this need to better reflect EXACTLY what this varible is.

WSrcCards <----no....just no....

stw->countSpellsPerCostAndColor[convertedCost] <----the function name in red is perfect...however inspect the item in bold....not so perfect.
stw should be a human readable varible or class name. not this.


KeybGrabber <---the grabber of the "b" key? how about we just call it "KeyBindGrabber" which is what this ACTUALLY is.

remaskBlkViews <----Blk? Blk in the surrounding context is "Blocker" ....why do i have to guess if Blk means blocker here?

getDoneTasks <--getFinishedTasks?


removeOne(DefenserDamaged* blocker, CombatStep); i will tell you what this does....then i will let you decide...remove a assigned blocker from the attackers blockers.list.....

BLK, ATK, OK, NONE<--------what?

ok
sumDamages() <---i would put money that everyone except maybe *maybe* wololo has no idea EXACTLY what this function does....i can tell you right now, it has NOTHING to do with what you read...sum damage....no.....this type of function naming needs to stop.

doDamageTest <---the naming of this function has always bothered me, since it is also another case of it being "not exactly" what you read.


function name "magicTexts" and function varible "magicText" niether are really what they appear. yes they deal with cards abilities, however not exactly as you think they would.

stillInUse(MTGCardInstance * card){return 0;}; <---this is not really what you think it is just by reading it, varibles/functions named like this that do something even the slightest bit different need to be renamed to better reflect EXACTLY what they do.
If these functions don't do what they are supposed to do, it is fine (recommended) to rename them.

You have to understand that the day these functions were created, they did what their name says. Unfortunately, these function were created several years ago, and have probably been changed by dozens of different people.
This is of course not an excuse, but it is called legacy code, and it happens all the time.
MTGRemovedFromGame * removedFromGame; <----this needs to be removed and/or renamed into Exile. the "removedFromGame zone" and exile, was a nasty nasty pointing setup....instead of changing removedFromGame zone into exile the person who recoded this simply pointed exile to it...
Fair enough. Note that pointing exile to RemovedFromGame was the best way to support both the legacy and the new naming without major refactoring headache. also, for those of us who starting playing Magic more than a year ago, "Removed from game" still makes more sense than "exile".
As long as we keep backwards compatiblity (that is, custom cards created more than 1 year ago are still using "removedfromgame" in their auto lines, and need to be supported), I'm fine if you want to refactor this.

Lands selfLands ....myLands? why use selfLands, this doesnt make sense in english.
which will lead me to another thing, i notice alot of nonenglish terms used in the SVN, varible namings ect...the source is clearly written in english, it should be consistent through out. Defenser for exsample....its not the blocking creature if thats what you think....a creature that is defenser means it was blocked. can you see where not being consistent in english use can lead to confusions?
When I'm making a mistake in English, it is VERY fine to let me know about it (I will actually thank you for that), but don't believe I intentionally coded an incorrect word variable name in the source, this is definitely not the goal. (Keep in mind that not all of us are native english speakers)
Regarding defenser, I'm pretty sure it doesn't mean the creature was blocked... I remember there was some kind of "trick" in there, but I think it works like this:
creatureA->defenser = creatureB
means : creatureA is a blocker, blocking creatureB

if (tc) =========> if (targetChooser) ....please....
dest =====>destination

abf.parseCounter<-----just no......

ABF seriously???? AbilityFactory.parseCounter ...doesnt this look MUCH better?
I disagree, especially for tc. We use this variable all over the place, there is NO way to not know it's a TargetChooser and you can call it common knowledge if you want.
I'm not a big fan of variable names that are one kilometer long, let's avoid the Java Frenzy if possible.
"af" or "abf" for ability factory looks good to me. also you have to keep in mind that the scope of these variables should be small, a few lines or something.

storeCommonAttribs() ======>storeCommonAttributes() ???isnt this much better?
I don't see a difference. "Attrib" is a well accepted abbrevation for attributes, as well as "attr" by the way.
NB_BASIC_ABILITIES i see this NB ALOT....what exactly does NB mean? nbcards, nbZones....nb????i cant think of a single english acronym that includes nb except "notbound" that would make sense in the context its being used. this needs to be changed into something a little easier to understand a first glance.and not another acronym.

nbminuses..........no
I use "nb" for "number". NB_BASIC_ABILITIES is "Number of Basic Abilities". You're not the first one to mention this to me. It's an abbreviation I've used for years, but it is probably a French abbreviation.
nbminuses : number of "minus" symbols.
Maybe "Amount" is better in english ?
mObjects <----menuObjects? why cut off the letters "enu" and turn it into some sort of code word.
mScale = mTargetScale; <---menuScale? mainScale? what is this m for?

mParticleSys->Update(dt);

mParticleSys =======>menuParticleSystem......Please.....
mTrans <-----this......
mLayers? menuLayers?

"m" in front of a variable is our coding rule for attributes of a class, so you better get used to it. It stands for "my" I believe, and it is a majorly accepted prefix among programmers in general
kAlternativeKeyword <---do these varibles really need to have "K" infront of them...if not remove them or fill in the missing letter.
k"-----"AlternativeKeyword
"k" stands for "constant" I believe. This is also a majorly spread among programmers. I'd personally rather use UPPERCASE for constants but haven't decided on a rule yet.
wEvent ===>worldEvent? if possible these 1 letter short hands should be changed into EXACTLY what they are. not only does it make it MUCH easier to read, but you wont have to go searching for the actual meaning of the short hand, and you wont have to guess.
"w" stands for "Wagic", per opposition to "j" which stands for "JGE". I don't think it's a problem
lcname <----10 dollars if anyone other then wololo can honestly tell me the meaning of "lc" WITHOUT looking in the source.
Ok, I am ready to bet money on this one. Most programmers should now what "lc" stands for.

Zethfox
Posts: 3022
Joined: Thu Jun 10, 2010 11:28 pm

Re: string parsing crashes

Post by Zethfox » Tue Nov 16, 2010 8:43 am

if this is all coding lingo, then atleast a comment explaining exactly what the varible does or function is supposed to do. im seeing wagic is going to be heading this direction of strict coding rules, acronyms, shorthands, ect...those are not transparent. yes you understand all the coding lingo, but you made this project open source....that means your lingo, mikes lingo, moots lingo...you cant slap them in anymore.
i think what i suggest with forbidding acronyms, shorthand, abbrivaitions makes this OPEN SOURCE projects more uniform and understandable by ALL that take part in it. because while you and jack might know what "nb" stands for, im POSITIVE mike and i didnt know.
attrabs vs attrabutes....the difference is that youre using REAL words. transparency my friend. this is after all open source.
understand?


i want to EPICALLY point this out...

Most programmers should now what "lc" stands for.

you see wololo....you see...keyword...MOST......

if you were to name it EXACTLY what it is..."prestringedCardName" whatever it actually means....then the "most" would become "ALL" understand?


also for "readability"
were going as far as putting brackets and braces on their own seperate lines.....
seriously? and you wont even consider what i suggest in making the coding much more uniform for ALL to code in...basically remove those comfy acronyms that YOU might understand and are use to that make NO SENSE to the next guy, a little sense to the next, is understood by that guy, but looks like hyroglyphs to the 4th programmer.

mnguyen
Posts: 229
Joined: Thu Apr 29, 2010 4:13 pm

Re: string parsing crashes

Post by mnguyen » Tue Nov 16, 2010 8:46 am

As wololo has stated, different people will have different views on what code is readable or not. If you read my posts you can see I agree. However, there are also conventions used in programming that are acceptable or at least recognized that you may not know of. I do agree with you about your remarks about non-descriptive variable naming being widespread. However, there are things like prefixes like "m" or "_" actually refer to that being a member variable to the object. In the specific case you mentioned, "mObjects" could perhaps be renamed to mMenuItems or mMenuObjects if that is more descriptive to you. In C++ there is a notion of global scope which is why this is important. A "g" prefix typically means global, although I don't think that it is used in the code currently. Also, "nb" seems to be a common abbreviation for "number" in the code base. In my experience, I've seen variations of this, such as "num" and "number". "nb" may have come about as a result of trying to shorten variable names as it made sense the developer. It didn't really take me that long to understand nb meant number but that may be experience speaking more than anything else. Another point, is that "lcname" is fairly common in programming to mean "lowercase name". for many standard functions, a reduction of the names into the letters representing their syllables is common. "uc" is typically short for "uppercase", "str" for "string", "ptr" for "pointer".

There are also other conventions as well that use abbreviations that may seem non-standard to you, but are well known in the developer community. Take a look at Hungarian Notation or Java's naming conventions. (MootPoint can comment on the correct names of the different conventions here :) ) All of which use prefixes and abbreviations that may seem strange to those that have not yet learned about them. That being said, I believe wololo also said something about feeling free to rename variables as necessary to ensure they convey meaning. If there is a question about what a variable means, "Defenser" for example, might be a candidate to be renamed to something appropriate in English. However, we are working with a diverse group of people from around the world and a change like that may require some notification, in case there is some misunderstanding in the naming of the variable. The k notation for the constants I think I'm responsible for a couple of them. I typically use all caps with underscores to separate words. In this case, since no standards were set, I used another piece of code in Wagic as a template for those. They should now be renamed to follow the guidelines to make things more consistent.

All this being said, I am all for more descriptive variable names. The code is meant for human eyes after all, it makes no difference to the compiler what you name your variables anymore. I typically use longer variable names to make sure it is clear to me what I am using it for. It may if you were writing low level code, but we hopefully don't have to operate on that level here. ;)


In regards to some of the method names not representing what they actually do may be due to code degradation over time. We should definitely try to rectify those situations as they will confuse even the most experienced developer. I do get why this may happen, as developers change the use of the method to fit their needs. However, if that is the case they should either create a new method if the old method is still being used, or just rename the method to what it should be called. One other thing you want to avoid are "side-effects" in method calls. You want to make sure that a method call does exactly what it is described to do and not anything else in addition. You don't want your "getter" method to also set variables not related to the the getter itself. There may be some initialization that may be required, but you don't want something like "getWidth()" to modify the name of the object as well.

Cheers
--Mike

Post Reply