I don't like code reviews
-
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!
-
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
yea, exactly my case... but from the strong reaction here, I think there some brain type at work...
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!
Super Lloyd wrote:
so we disagree to disagree then?
You agree to disagree. If you disagree to disagree, then you agree.
-
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!
Super Lloyd wrote:
For example a simple expression like
a = b + ccan confuse me if you write insteadmyobjectBlu = aCycleValueOrdinal + meteorStrikeOffsetTime.Good example. Without greater context, I can't tell whether "b + c" is the correct thing to do to get "a". Given your second example however, if I know that in order to get myobjectBlu, the equation needs meteorStrikeOffsetTime to be multiplied by a constant before being added to aCycleValueOrdinal, then I can spot this. [Edit] Just being devil's advocate. I have to imagine that someone reviewing this would see the greater context and not just look at the one line.
-
Oh, I thought for a short while it was the Schrödinger operator.
Wrong is evil and must be defeated. - Jeff Ello
Some say the Schrodinger operator is kind of half-assed; I agree with them half the time :wtf:
«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!
Super Lloyd wrote:
double Value { get { var x = Calculation(); return flag ? x : 2 * x; } }
Here's my comment - do NOT use var! Using var instead of double forced me to look in a second place in your code to figure out what I was reading. var is an abomination.
-
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!
The code bases of the existing Adobe products I worked on, PhotoShop, Illustrator, were the opposite of design by committee: they grew by accretion. New, "hot," features always took precedence over any systematic re-org. By the way, I have complete respect for the world-class programmers I had the chance to work with, and, I felt continually humble in their presence ... for good reasons: I was a one-trick pony :) When they did the PC version of PhotoShop, they found the Mac code (using the MacApp framework) impossible to port "native," so they re-implemented MacApp on Windows, which took a tremendous effort. [^]
«The mind is not a vessel to be filled but a fire to be kindled» Plutarch
-
Super Lloyd wrote:
double Value { get { var x = Calculation(); return flag ? x : 2 * x; } }
Here's my comment - do NOT use var! Using var instead of double forced me to look in a second place in your code to figure out what I was reading. var is an abomination.
obermd wrote:
var is an abomination.
when used improperly. Our shop uses var all the time.
var myClass = new MyMostExcellentClassInTheWholeWideWorld();this is acceptable IMHO and the opinions of hundreds of thousands of developers world wide, if not millions, billions, and trillions.var something1 = something2.GetSomething3();this is not acceptable. This is a hotly debated topic, I know. To each there own. -
obermd wrote:
var is an abomination.
when used improperly. Our shop uses var all the time.
var myClass = new MyMostExcellentClassInTheWholeWideWorld();this is acceptable IMHO and the opinions of hundreds of thousands of developers world wide, if not millions, billions, and trillions.var something1 = something2.GetSomething3();this is not acceptable. This is a hotly debated topic, I know. To each there own.and now, C# 9 brings you "even more naked" instantiation: List xs = new(); List? ys = new();
«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!
I agree, the length of a variable name should be proportional to its scope and visibility. The reader should be able to see a one-letter variable and just know that it has only local scope. Any developer who gets confused by such a thing does not belong on my team. The fewer the characters, the easier to read. :-D
-
and now, C# 9 brings you "even more naked" instantiation: List xs = new(); List? ys = new();
«The mind is not a vessel to be filled but a fire to be kindled» Plutarch
BillWoodruff wrote:
List<int> xs = new(); List<int>? ys = new();
stuff like this is just retarded. it is really for the people who hate "var". These people want to see the instantiation on the left, rather than the right. It makes me laugh and cry all the way home.
-
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!
-
obermd wrote:
var is an abomination.
when used improperly. Our shop uses var all the time.
var myClass = new MyMostExcellentClassInTheWholeWideWorld();this is acceptable IMHO and the opinions of hundreds of thousands of developers world wide, if not millions, billions, and trillions.var something1 = something2.GetSomething3();this is not acceptable. This is a hotly debated topic, I know. To each there own. -
BillWoodruff wrote:
List<int> xs = new(); List<int>? ys = new();
stuff like this is just retarded. it is really for the people who hate "var". These people want to see the instantiation on the left, rather than the right. It makes me laugh and cry all the way home.
-
I agree, the length of a variable name should be proportional to its scope and visibility. The reader should be able to see a one-letter variable and just know that it has only local scope. Any developer who gets confused by such a thing does not belong on my team. The fewer the characters, the easier to read. :-D
-
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 use "i" in for loops. That's it. Edit: Also using "x and y" currently: for position coordinates.
It was only in wine that he laid down no limit for himself, but he did not allow himself to be confused by it. ― Confucian Analects: Rules of Confucius about his food
-
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!
Super Lloyd wrote:
so we disagree to disagree then?
No. If I was the boss and an employee used
Calculateas a function name I'd shitcan him or her, unless it was obviously a joke and they fixed it as soon as the laugh was had. I would not let a codebase I am responsible for be polluted by such meaningless names that all coders from there on out are going to have to spend precious time figuring out the intent of even something this simple. Among the oldest of business mantras is "Time is Money". And coding like this wastes everyone else's time. Making a lone coder happy, even if they are good, isn't worth it in cases like this.The Science of King David's Court | Object Oriented Programming with C++
-
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 use descriptive names, which tend to be long. Short names in my code depend upon context. Here's an example:
for (int JMi = 0; JMi < JettingModules.Length; JMi++)
{
JettingModules[JMi].DoStuff(...);
}I will occasionally use short names synonymously to long names in order to make a complex expression simpler to understand or read:
bool ready = StitchCalibration.Context.Ready(...);
bool online = Framework.Online && DFE.Service.Connected;
int retry = 0;
while (ready && online && (retry < 3))
{
//...
}My names are chosen to tie related concepts together and for clarity of expression.
Software Zen:
delete this;