|
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...
|
|
|
|
|
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!
|
|
|
|
|
for (int room = 0, roomCount = rooms.Count; room < roomCount; room++)
{
rooms[room] = new Room();
}
room can be replaced by pos (or anything more meaningful); anyway: anything not like this as a code will not go live for me...
Besides this point, when I had to manage core reviews, I dit it with the whole team so anybody could have seen and emphasis on different things and not only what I wanted to pinpoint.
One point missing, is that code reviews in 2021 aren't the same as those done in the 1990: now we have tools like R# Shart (no publicity intended) which are removing a lot of "discrepancies" (bugs), automating test tools, and so on. So now, code reviews are more about code's readability and maintenance.
Because of these (hey folks, dont forget you're using Intellisense which types most of the code for you!), so definitively no excuse to have readable code.
modified 8-Sep-21 13:12pm.
|
|
|
|
|
I would reject your pull request as well, if you had: a = b + c That 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 + c is not self-documenting.
|
|
|
|
|
I am glad we disagree to disagree
|
|
|
|
|
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! 
|
|
|
|
|
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.
|
|
|
|
|
|
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...
|
|
|
|
|
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
modified 7-Sep-21 5:59am.
|
|
|
|
|
design by committee... the plight of any big corp! ^_^
|
|
|
|
|
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
|
|
|
|
|
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 called originalSharedValues.
|
|
|
|
|
Super Lloyd wrote: For example a simple expression like a = b + c
can confuse me if you write instead myobjectBlu = 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.
|
|
|
|
|
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.
|
|
|
|
|
and now, C# 9 brings you "even more naked" instantiation:
List<int> xs = new();
List<int>? 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.
modified 7-Sep-21 10:28am.
|
|
|
|
|
This works as well because it's obvious from the single line of code the type of the variable.
|
|
|
|
|
This example works because the class name is in the same line of code.
|
|
|
|
|
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. 
|
|
|
|
|
Thinking about my own code I realize I do this as well. Descriptive names for globals or widely used variables and functions and short names for local use only.
|
|
|
|
|
In 23 years, I've never had my code reviewed. I feel lucky!
"Go forth into the source" - Neal Morse
"Hope is contagious"
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
