I don't like code reviews
-
Reflecting a bit upon myself.... Long expression take more time to parse. The number of characters matter here... it's strenuous to parse long wall of text.. I might have some sort of dyslexia or something. Affect me reading novel too... I sometimes have to read paragraphs again because I only superficially read each word....
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Super Lloyd wrote:
Long expression take more time to parse.
I think time-to-parse varies a lot depending on multiple individual factors, like education, mother-tongue, certain cognitive skills. imho, pragmatic issues can affect naming schemes; I often use Simonyi-Hungarian style names for public properties in UserControls because I want them to show up in the PropertyBrowser in a group. For me, longer is not a problem :) However, I keep in mind that:
Quote:
Edward Sapir wrote, "When it comes to linguistic form, Plato walks with the Macedonian swineherd, Confucius with the head-hunting savage of Assam.
Consider the Bantu Kivunjo people's language, where:
Quote:
... The verb "Näïkìmlyìïà," meaning "He is eating it for her," is composed of eight parts: • N-: A marker indicating that the word is the "focus" of that point in the conversation. • -ä-: A subject agreement marker. It identifies the eater as falling into Class 1 of the sixteen gender classes, "human singular." (Remember that to a linguist "gender" means kind, not sex.) Other genders embrace nouns that pertain to several humans, thin or extended objects, objects that come in pairs or clusters, the pairs or clusters themselves, instruments, animals, body parts, diminutives (small or cute versions of things), abstract qualities, precise locations, and general locales. • -ï-: Present tense. Other tenses in Bantu can refer to today, earlier today, yesterday, no earlier than yesterday, yesterday or earlier, in the remote past, habitually, ongoing, consecutively, hypothetically, in the future, at an indeter-minate time, not yet, and sometimes. • -kì-: An object agreement marker, in this case indicating that the thing eaten falls into gender Class 7. • -m-: A benefactive marker, indicating for whose benefit the action is taking place, in this case a member of gender Class 1. • -lyì-: The verb, "to eat." • -ï-: An "applicative" marker, indicating that the verb's cast of players has been augmented by one additional role, in this case the benefactive. (As an analogy, imagine that in English we had to add a suffix to the verb bake when it is used in 1 baked her a cake as opposed to the usual I baked a cake.) • -à : A final vowel, which can indicate indicative versus subjunctive mood. If you multiply out the number of possible combinations of the seven prefixes and suffixes, th
-
I have to agree with David O'Neal - short names are a bad idea. There isn't just the "Understandability" factor, though that is very significant - especially when you look at the code for the first time, or after a long break from it. More significantly one character names are more prone to error, because it's very easy to hit the wrong key and get a valid variable name. While Intellisense can make that happen with more descriptive names as well, it's a lot more obvious that it's wrong. To use your example:
Quote:
it'e E=mc^2, not Energy = Mass * SpeedOfLight^2, to vindicate me...
E=nc^2
Looks vaguely right and it's easy to miss the mistake when skimming, but the longer form:
Energy = numberOfCatsInTheBox * SpeedOfLight^2
Is immediately wrong. And ... if you are anything like me you read what you meant to write, rather than what you did type. So short names make life harder for you, me, and everyone else who has to work with the code - so yes, code reviews should pick them up! I'm not saying that all short names are bad, it's pointless to use a long name here:
for (int i = 0; i < rooms.Count; i++)
{
rooms[i] = new Room();
}But in general, they are a PITA!
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
OriginalGriff wrote:
Energy = numberOfCatsInTheBox * SpeedOfLight^2
Is immediately wrong.
Especially since
^is theXoroperator, not the "to the power of" operator. ;P
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
"These people looked deep within my soul and assigned me a number based on the order in which I joined" - Homer
-
OriginalGriff wrote:
Energy = numberOfCatsInTheBox * SpeedOfLight^2
Is immediately wrong.
Especially since
^is theXoroperator, not the "to the power of" operator. ;P
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
:-D
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
-
I have to agree with David O'Neal - short names are a bad idea. There isn't just the "Understandability" factor, though that is very significant - especially when you look at the code for the first time, or after a long break from it. More significantly one character names are more prone to error, because it's very easy to hit the wrong key and get a valid variable name. While Intellisense can make that happen with more descriptive names as well, it's a lot more obvious that it's wrong. To use your example:
Quote:
it'e E=mc^2, not Energy = Mass * SpeedOfLight^2, to vindicate me...
E=nc^2
Looks vaguely right and it's easy to miss the mistake when skimming, but the longer form:
Energy = numberOfCatsInTheBox * SpeedOfLight^2
Is immediately wrong. And ... if you are anything like me you read what you meant to write, rather than what you did type. So short names make life harder for you, me, and everyone else who has to work with the code - so yes, code reviews should pick them up! I'm not saying that all short names are bad, it's pointless to use a long name here:
for (int i = 0; i < rooms.Count; i++)
{
rooms[i] = new Room();
}But in general, they are a PITA!
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
How many times did I accidentally use the wrong shortly named variable (not sure mattered but anyway)? Once or twice last year. How many times I had to refactor long name into short name to improve my understanding? Every single time. There you have it. Refactoring variable name to short version is the first thing I do when refactoring. I will grant you that I might have some intellectual disability not shared by my peers (because, obviously, they have no problem with long names whereas, and I am not making this up, it's ultra confusing for me) and I have to suck it up... But that's how it work for me. And on that will disagree to disagree.
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
ok, sometimes there are very good comments... but every time the reviews are waaaaay too slow. and very often there are comments which are both useless, antagonistic and a big waste of time... for example I don't see the point of long variable name nor do I like them, particularly for a short liner like
double Value
{
get
{
var x = Calculation();
return flag ? x : 2 * x;
}
}And have to wait a few more hours because I was told 'not to use short variable name'. Unsure I renamed 'x' to 'aNumber', but that irks me... On top of that, that might be just me with my bad memory, but I find long variable name harder to read! :omg: For example a simple expression like
a = b + ccan confuse me if you write insteadmyobjectBlu = aCycleValueOrdinal + meteorStrikeOffsetTime. Why they not care about making the code easier to understand?! :(( ok, ok, I need to get over it. just venting here! :laugh: Joke aside, you might like long variable name, but you won't convince me. save everyone's time and let's just agree to disagree. Or disagree to disagree, if you prefer... EDIT Upon reflection, I might be part of a minority of people with reading disability.. :(( When reading long sentence I am skipping words and filling in by guess. Similarly long line of C# requires me multiple reading. And it kind of depends on the overall number of character, not words... So I guess normal people comes with their usually suck it up, I am fine... :sigh:A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
I would reject your pull request as well, if you had:
a = b + cThat is is just super lazy programming IMHO, and makes it very difficult for the next developer to understand. I expect all code that comes across my desk to be as self-documenting as possible, including my own. This crap:a = b + cis not self-documenting. -
How many times did I accidentally use the wrong shortly named variable (not sure mattered but anyway)? Once or twice last year. How many times I had to refactor long name into short name to improve my understanding? Every single time. There you have it. Refactoring variable name to short version is the first thing I do when refactoring. I will grant you that I might have some intellectual disability not shared by my peers (because, obviously, they have no problem with long names whereas, and I am not making this up, it's ultra confusing for me) and I have to suck it up... But that's how it work for me. And on that will disagree to disagree.
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Quote:
How many times did I accidentally use the wrong shortly named variable that you are aware of (not sure mattered but anyway)?
That's the point: if you notice, it didn't matter. But if you don't ... it might matter a lot. Getting rid of potential errors before they can be noticed at run time is why C# is a strongly typed language, why experienced developers normally prefer compiled languages to interpreted, and why teams insist on descriptive names ... :laugh:
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
-
Quote:
How many times did I accidentally use the wrong shortly named variable that you are aware of (not sure mattered but anyway)?
That's the point: if you notice, it didn't matter. But if you don't ... it might matter a lot. Getting rid of potential errors before they can be noticed at run time is why C# is a strongly typed language, why experienced developers normally prefer compiled languages to interpreted, and why teams insist on descriptive names ... :laugh:
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
you are become more and more bad faith.. or perhaps, conversely, your brain really do have problem with 1 letter variable that I dont. The only time I mistake them is because I used the wrong index because I just couldn't quite remember if it was index1 (i.e. i) or index2 (i.e. j) I should use.... I never mistake w for x and I doubt you did either.... further there is one single variable in this whole block, so I am not sure how you could use it wrong accidentally...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
ok, sometimes there are very good comments... but every time the reviews are waaaaay too slow. and very often there are comments which are both useless, antagonistic and a big waste of time... for example I don't see the point of long variable name nor do I like them, particularly for a short liner like
double Value
{
get
{
var x = Calculation();
return flag ? x : 2 * x;
}
}And have to wait a few more hours because I was told 'not to use short variable name'. Unsure I renamed 'x' to 'aNumber', but that irks me... On top of that, that might be just me with my bad memory, but I find long variable name harder to read! :omg: For example a simple expression like
a = b + ccan confuse me if you write insteadmyobjectBlu = aCycleValueOrdinal + meteorStrikeOffsetTime. Why they not care about making the code easier to understand?! :(( ok, ok, I need to get over it. just venting here! :laugh: Joke aside, you might like long variable name, but you won't convince me. save everyone's time and let's just agree to disagree. Or disagree to disagree, if you prefer... EDIT Upon reflection, I might be part of a minority of people with reading disability.. :(( When reading long sentence I am skipping words and filling in by guess. Similarly long line of C# requires me multiple reading. And it kind of depends on the overall number of character, not words... So I guess normal people comes with their usually suck it up, I am fine... :sigh:A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
I would also like to warn you that it may not be a good idea to publicly complain about your work related issues here, seeing that we all know you work for EA now and EA employees may frequent this site and forum. Just a thought...Job security and all. I'm sure you understand. ;)
-
I would reject your pull request as well, if you had:
a = b + cThat is is just super lazy programming IMHO, and makes it very difficult for the next developer to understand. I expect all code that comes across my desk to be as self-documenting as possible, including my own. This crap:a = b + cis not self-documenting.I am glad we disagree to disagree
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
How many times did I accidentally use the wrong shortly named variable (not sure mattered but anyway)? Once or twice last year. How many times I had to refactor long name into short name to improve my understanding? Every single time. There you have it. Refactoring variable name to short version is the first thing I do when refactoring. I will grant you that I might have some intellectual disability not shared by my peers (because, obviously, they have no problem with long names whereas, and I am not making this up, it's ultra confusing for me) and I have to suck it up... But that's how it work for me. And on that will disagree to disagree.
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
And one more thing:
Super Lloyd wrote:
How many times I had to refactor long name into short name to improve my understanding? Every single time.
If you do that on team code, you are going to upset - rightly - a heck of a lot of people who have invested effort in getting the names "right" and the code self documenting. It's a bit like pulling a whole branch, refactoring it to your prefered indentation style and pushing it back otherwise unchanged ... you are going to make enemies very quickly. I'd suggest that you learn to live with descriptive names or the company may decide you are more trouble than you are worth ... Are you on a probationary period with EA? Most new appointments are!
"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony "Common sense is so rare these days, it should be classified as a super power" - Random T-shirt AntiTwitter: @DalekDave is now a follower!
-
OriginalGriff wrote:
Energy = numberOfCatsInTheBox * SpeedOfLight^2
Is immediately wrong.
Especially since
^is theXoroperator, not the "to the power of" operator. ;P
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
Oh, I thought for a short while it was the Schrödinger operator.
Wrong is evil and must be defeated. - Jeff Ello
-
ok, sometimes there are very good comments... but every time the reviews are waaaaay too slow. and very often there are comments which are both useless, antagonistic and a big waste of time... for example I don't see the point of long variable name nor do I like them, particularly for a short liner like
double Value
{
get
{
var x = Calculation();
return flag ? x : 2 * x;
}
}And have to wait a few more hours because I was told 'not to use short variable name'. Unsure I renamed 'x' to 'aNumber', but that irks me... On top of that, that might be just me with my bad memory, but I find long variable name harder to read! :omg: For example a simple expression like
a = b + ccan confuse me if you write insteadmyobjectBlu = aCycleValueOrdinal + meteorStrikeOffsetTime. Why they not care about making the code easier to understand?! :(( ok, ok, I need to get over it. just venting here! :laugh: Joke aside, you might like long variable name, but you won't convince me. save everyone's time and let's just agree to disagree. Or disagree to disagree, if you prefer... EDIT Upon reflection, I might be part of a minority of people with reading disability.. :(( When reading long sentence I am skipping words and filling in by guess. Similarly long line of C# requires me multiple reading. And it kind of depends on the overall number of character, not words... So I guess normal people comes with their usually suck it up, I am fine... :sigh:A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
I'm sure your pull request wasn't rejected because your variable name was too short, it was rejected because the name is not descriptive. Your new name, aNumber, is equally undescriptive so should be rejected again. Let's say Value returns a wrong value and I had to fix it, I'd see a generic Calculation, an x and some flag. This tells me absolutely nothing about what the code does or is supposed to do. Heck, I don't even know what Value is, but perhaps that's clear from the class context (unless you named that C, which wouldn't surprise me now). All in all, very hard to debug, I can't make assumptions about the code, I'd have to go all the way up or down the stack to know where Value or flag came from or is used, I'd have to read the entire Calculation function to find out what is being calculated... In short, I'd have to read the entire code base just to get a gist of what this little piece of code does and remember it all! I'm with everyone else on this thread and on your team, apparently, your code is bad and you should feel bad.
Best, Sander Azure DevOps Succinctly (free eBook) Azure Serverless Succinctly (free eBook) Migrating Apps to the Cloud with Azure arrgh.js - Bringing LINQ to JavaScript
-
I would also like to warn you that it may not be a good idea to publicly complain about your work related issues here, seeing that we all know you work for EA now and EA employees may frequent this site and forum. Just a thought...Job security and all. I'm sure you understand. ;)
sshh dont advertise it now! ;P
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
I'm sure your pull request wasn't rejected because your variable name was too short, it was rejected because the name is not descriptive. Your new name, aNumber, is equally undescriptive so should be rejected again. Let's say Value returns a wrong value and I had to fix it, I'd see a generic Calculation, an x and some flag. This tells me absolutely nothing about what the code does or is supposed to do. Heck, I don't even know what Value is, but perhaps that's clear from the class context (unless you named that C, which wouldn't surprise me now). All in all, very hard to debug, I can't make assumptions about the code, I'd have to go all the way up or down the stack to know where Value or flag came from or is used, I'd have to read the entire Calculation function to find out what is being calculated... In short, I'd have to read the entire code base just to get a gist of what this little piece of code does and remember it all! I'm with everyone else on this thread and on your team, apparently, your code is bad and you should feel bad.
Best, Sander Azure DevOps Succinctly (free eBook) Azure Serverless Succinctly (free eBook) Migrating Apps to the Cloud with Azure arrgh.js - Bringing LINQ to JavaScript
This!
Wrong is evil and must be defeated. - Jeff Ello
-
ok, sometimes there are very good comments... but every time the reviews are waaaaay too slow. and very often there are comments which are both useless, antagonistic and a big waste of time... for example I don't see the point of long variable name nor do I like them, particularly for a short liner like
double Value
{
get
{
var x = Calculation();
return flag ? x : 2 * x;
}
}And have to wait a few more hours because I was told 'not to use short variable name'. Unsure I renamed 'x' to 'aNumber', but that irks me... On top of that, that might be just me with my bad memory, but I find long variable name harder to read! :omg: For example a simple expression like
a = b + ccan confuse me if you write insteadmyobjectBlu = aCycleValueOrdinal + meteorStrikeOffsetTime. Why they not care about making the code easier to understand?! :(( ok, ok, I need to get over it. just venting here! :laugh: Joke aside, you might like long variable name, but you won't convince me. save everyone's time and let's just agree to disagree. Or disagree to disagree, if you prefer... EDIT Upon reflection, I might be part of a minority of people with reading disability.. :(( When reading long sentence I am skipping words and filling in by guess. Similarly long line of C# requires me multiple reading. And it kind of depends on the overall number of character, not words... So I guess normal people comes with their usually suck it up, I am fine... :sigh:A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
I agree with you, I don't have a problem using x or i for throw away values like in the example you gave or in OriginalGriff's example. Anything longer can detract from the logic. I found the comment under the Edit interesting because I have the same issue you have, I've always suspected that I'm slightly dyslexic (never investigated though).
// TODO: Insert something here
Top ten reasons why I'm lazy 1.
-
I agree with you, I don't have a problem using x or i for throw away values like in the example you gave or in OriginalGriff's example. Anything longer can detract from the logic. I found the comment under the Edit interesting because I have the same issue you have, I've always suspected that I'm slightly dyslexic (never investigated though).
// TODO: Insert something here
Top ten reasons why I'm lazy 1.
Yeah, glad to know I am not alone! :) Seeing all the strong opposite reaction, I am starting to believe, just as long variable names are a real impediment to my comprehension. Many might have a sort of opposite problem. Though, and I am digressing here, from the strongly opinionated people I fear an even worst problem. Focus on names at the detriment of the logical operation...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
so we disagree to disagree then? I find the later no easier.. In fact some interesting brain chemistry must be at work here... I was reflecting how physicist (that's my background), prefer short name too, i.e. it'e
E=mc^2, notEnergy = Mass * SpeedOfLight^2, to vindicate me... Anyway, regardless, it's more interesting to consider what psychological factor lead from one to another. I know that for me, bad work memory favor short variable names. Long variable names are just too hard, I have to read the statement 2 or 3 times to get it. 1 or 2 time to get all the variables involved, and one more time to get the computation. I can get all that in one go/read with shorter text - i.e. short variable names and simple math. Maybe I have some sort of dyslexia or something, I tend to not read big wall of text very accurately. Not just in code but also in plain English... Hence for me shorter variable name increasing my accuracy / understanding... :sigh:A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Another angle: I do use single-letter Field names when: 1) they are used only in the scope of a Method/Function and 2) they are, imho, easily recognized, in context, as representing logical attributes. So, in a Method that takes a Rectangle as a parameter: I might, as the need arises, use 'w for 'Width, 'h for 'Height, etc. I only use 'i, 'j, 'k in for loops. But, if an employer wanted longer names, no problem; the 'name issue is not a "religious" issue for me, but, I value consistency.
«The mind is not a vessel to be filled but a fire to be kindled» Plutarch
-
Yeah, glad to know I am not alone! :) Seeing all the strong opposite reaction, I am starting to believe, just as long variable names are a real impediment to my comprehension. Many might have a sort of opposite problem. Though, and I am digressing here, from the strongly opinionated people I fear an even worst problem. Focus on names at the detriment of the logical operation...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Super Lloyd wrote:
Seeing all the strong opposite reaction
I think you are seeing a polarization that is not as intense as you may be experiencing it. I see the gist of the comments here as focusing on code readability, maintainability ... in the context of a project with multiple programmers, and a code base where any accidental semantic clash, or mis-interpretation of the meaning of names, can have disastrous consequences, cause needless confusion, etc. I am reminded of when I joined the Illustrator team at Adobe, and, out of curiosity, looked in the source for functions that converted whatever to hexadecimal: there were 17 different functions, most of which were duplicates ... that no one dared to change :)
«The mind is not a vessel to be filled but a fire to be kindled» Plutarch
-
ok, sometimes there are very good comments... but every time the reviews are waaaaay too slow. and very often there are comments which are both useless, antagonistic and a big waste of time... for example I don't see the point of long variable name nor do I like them, particularly for a short liner like
double Value
{
get
{
var x = Calculation();
return flag ? x : 2 * x;
}
}And have to wait a few more hours because I was told 'not to use short variable name'. Unsure I renamed 'x' to 'aNumber', but that irks me... On top of that, that might be just me with my bad memory, but I find long variable name harder to read! :omg: For example a simple expression like
a = b + ccan confuse me if you write insteadmyobjectBlu = aCycleValueOrdinal + meteorStrikeOffsetTime. Why they not care about making the code easier to understand?! :(( ok, ok, I need to get over it. just venting here! :laugh: Joke aside, you might like long variable name, but you won't convince me. save everyone's time and let's just agree to disagree. Or disagree to disagree, if you prefer... EDIT Upon reflection, I might be part of a minority of people with reading disability.. :(( When reading long sentence I am skipping words and filling in by guess. Similarly long line of C# requires me multiple reading. And it kind of depends on the overall number of character, not words... So I guess normal people comes with their usually suck it up, I am fine... :sigh:A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Variable names should be long enough to explain what they do but short enough to be succinct. Very long names show you haven't thoroughly worked out what they are for or you are being too restrictive. For example, the other day I was writing some code that I hadn't thoroughly thought through and I created a variable called something like
locationOfCommonSharedValuesBeforeTheConfigurationWasJoinedToAnotherEnvironmentsConfiguration. After much soul searching, it is now calledoriginalSharedValues. -
Super Lloyd wrote:
Seeing all the strong opposite reaction
I think you are seeing a polarization that is not as intense as you may be experiencing it. I see the gist of the comments here as focusing on code readability, maintainability ... in the context of a project with multiple programmers, and a code base where any accidental semantic clash, or mis-interpretation of the meaning of names, can have disastrous consequences, cause needless confusion, etc. I am reminded of when I joined the Illustrator team at Adobe, and, out of curiosity, looked in the source for functions that converted whatever to hexadecimal: there were 17 different functions, most of which were duplicates ... that no one dared to change :)
«The mind is not a vessel to be filled but a fire to be kindled» Plutarch
design by committee... the plight of any big corp! ^_^
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
