TL;DR I wouldn't worry too much about what is said in these code reviews. Sometimes, unfortunatly, the people you work for have no idea what they are doing.
True but unfortunately thanks to GIT and tools like Sami, Github, Gitlab, Stash etc.. a lot of us no longer have the luxury of ignoring code reviews as until the reviewer (no matter how clueless) has agreed that they like the code we can't physically merge the code from a pull request.
TL;DR I wouldn't worry too much about what is said in these code reviews. Sometimes, unfortunatly, the people you work for have no idea what they are doing.
True but unfortunately thanks to GIT and tools like Sami, Github, Gitlab, Stash etc.. a lot of us no longer have the luxury of ignoring code reviews as until the reviewer (no matter how clueless) has agreed that they like the code we can't physically merge the code from a pull request.
Yes, that is true. There is a lot more of this going on with all the new tool.
What I was trying to say is not to ignore them, but just don't put too much importance on these reviews. If you let things like this determine your skill as a programmer, it can drive you crazy when they couldn't find their ass with both hands.
Hi,
This isn't strictly game related, but I like you guys so I thought I'd post it here. I work with C# for my job, and we have to undergo code reviews. Yesterday I finished a work item and used code similar to the following in several places:
SomeType GetSomething
{
get
{
if (somecondition)
{
return something;
}
else
{
return null;
}
}
} My code review was completed as "needs work" on the grounds that the above code was "very misleading", which I don't get at all. I understand the "else" isn't necessary here, but I think it not only
There is also this thing, that can be quite nasty depending on the context, that you do not reevaluate the 'somecondition' variable that drive's null response to the very asking caller.
What do you mean by "not reevaluating the condition"? As soon as the value of the property is requested, the "get"-code is executed, thus the "somecondition" is evaluated. (And we can assume it's not just a variable, but a shortening of the real code.)
I was reading the code literal, in case it is not a member property, no further code can run.
In case of wishing to recive null if the state of getted object is not in particular on-go, one should reevaluate/check it is so at that very moment of the object requested. If not , one has to track the "somecondition" value altering it permanently with on-go state changing.
It depends though on the program usage which one is more run-time suitable. One can return the object without condition and have callie check something else than being null to see for themself wheather the object is in on-go, anyway, lot of things needs to be though about when one finds himself at the construct that OP is in.
@Graelig:
It really depends on the "somecondition", and we just don't know, what it actually is. If it's a simple null-check for "something" and "something" is just a private or protected member, just returning it would be the most simple thing. But if you're not just returning a member or if the condition is more than a simple null check, you can't omit the condition.
And by the way: it's a (C#) property, not a method. The name GetSomething is misleading (it should be called GetSomething instead), but you still can't call/invoke it. ;)
@JohnnyCode:
What do you mean by "not reevaluating the condition"? As soon as the value of the property is requested, the "get"-code is executed, thus the "somecondition" is evaluated. (And we can assume it's not just a variable, but a shortening of the real code.)
Well what I mean is it should do just what it says. So if it's getSomething that is the only thing it should do, a simple get.
If it's something else then it should be named differently and handled differently. I am guessing that's why they are confused by it. It's not too clear. I really doubt it's the style if if and else clause. If it really is then they are going to be hell to work with, just look how many answers people gave for what style they prefer.
But sadly, this was the problem, since the accepted solution is:I really doubt it's the style if if and else clause. If it really is then they are going to be hell to work with, just look how many answers people gave for what style they prefer.
What was that pattern?
Personally I'm not a huge fan of that, since it forces the reader to track the state of "result" as they look through the code, but it's used in many other places in our codebase, and consistency adds value.SomeType result = null; if (somecondition) { result = something; } return result;
Does it matter for the calling site, whether or not the value is cached in a variable, or calculated on the fly?
Yes, of course. You should know the performance characteristics of a piece of code.
In my opinion, "GetMagnitude" would be more suitable, since it doesn't define how the magnitude is retrieved. A "GetMeSomething" method doesn't imply the return of a member variable, and it shouldn't. You can see it this way: if you have a Vector and you want to get it's magnitude, you call it with "give me your magnitude" (assuming you can talk with your Vector, which I honestly can't do), what translates to the method name "GetMagnitude".
IMO, property access in C# should be relatively lightweight. You might to some lazy initialisation, but as a general rule, I should be able to treat a property like variable. If the property access is expensive, I would make it a method and I would probably call it "CalculateX", rather than "GetX".
Before you respond, you should keep in mind: for me, object orientation isn't that much about the manipulation of data (member variables), but it's more about objects and their interactions.
The answer here is "it depends". In high level, non-performance critical parts of a system, sure, that level of abstraction is appropriate. But lower down, you have to think about the data; it's really all there is.
The point is, in any non-trivial project you can't just blindly use objects without a care in the world. You have to understand the basics performance characteristics. Equally when you are authoring a class, you must think about how it will be used and communicate it's use (through the interface) to potential clients of the class.
"Method is more expensive than property" is an easy shorthand for this, and one I think most C# programmers instinctively use.
