|
If there's a var called part_exist, and 1 (or true) means the part does not exist, then that's not personal taste, it's just plain bad programming! The entire reason for selecting a variable name is to accurately convey the data stored within...Some day I imagine AIs will be smart enough to warn us for those type of code smells
|
|
|
|
|
When the electronic planer does the eplan in the office and never goes to the production hall, and just copy paste things from other projects that never saw in person... it happens more often than it should.
At the end I had an agreement (after several discussion he logically didn't win). Once I got to the machine and did my IO-Tests... I would give him my variables list and he would accept it and correct the E-Plan without complaining.
M.D.V. 
If something has a solution... Why do we have to worry about?. If it has no solution... For what reason do we have to worry about?
Help me to understand what I'm saying, and I'll explain it better to you
Rating helpful answers is nice, but saying thanks can be even nicer.
|
|
|
|
|
asserts( ) can be removed entirely based on compiler settings. That would explain the warning.
|
|
|
|
|
I know, but I didn't have NDEBUG defined in that configuration so assert was active.
Don't get me wrong, I agree with having Intellisense looking over my shoulder and flagging potential issues but sometimes it gets annoying just like sometimes a coworker can be annoying when you do pair programming. And just as with a coworker, it feels good to bitch from time to time 
Mircea
|
|
|
|
|
They are hints. (resharper C++ also do that)
Sometimes they work, sometimes they don't.
In that case, it is not useful
I'd rather be phishing!
|
|
|
|
|
Maximilien wrote: They are hints.
I agree. Just suggestions. 
Maximilien wrote: In that case, it is not useful
Thanks for the confirmation. That's what I thought, but was wondering if I missed something.
Also, it really is just a matter of opinion on this one since either would work.
|
|
|
|
|
raddevus wrote: AI you have failed me.
Problem found. 
BTW... I am with you in this case.
M.D.V.
If something has a solution... Why do we have to worry about?. If it has no solution... For what reason do we have to worry about?
Help me to understand what I'm saying, and I'll explain it better to you
Rating helpful answers is nice, but saying thanks can be even nicer.
|
|
|
|
|
Nelek wrote: Problem found.
Nelek wrote: BTW... I am with you in this case.
Thanks. I thought all sane devs would be. The rest are not devs or are not sane or both. 
|
|
|
|
|
I'm curious if this error still pops up if you write more code below the if statement. I've never seen this before. My initial hunch is it might think you're doing the "if-good-else-bad" pattern since the only active code is in the if statement currently, and trying to move it around to the "early-exit/pre-condition" pattern which I personally do think is way cleaner. Or it may be attempting some pointless hyper-optimization? 
|
|
|
|
|
Jon McKee wrote: I'm curious if this error still pops up if you write more code below the if statement.
That's a very good question.
Here's the updated code that contains more code now. Note: this is simple code that is going in a book.
static void Main(string[] args)
{
if (args.Length < 1){
Console.WriteLine("Please provide 1 argument to indicate the command you want to run.\nUsage: getInfo <command-name>");
return;
}
switch (args[0].ToLower()){
case "os":{
Console.WriteLine($"OS : {Environment.OSVersion}");
break;
}
case "pwd":{
Console.WriteLine($"The current directory is: {Environment.CurrentDirectory}");
break;
}
case "cl":{
Console.WriteLine($"Command line was: {Environment.CommandLine}");
break;
}
case "sysdir":{
Console.WriteLine($"System dir: {Environment.SystemDirectory}");
break;
}
case "mname":{
Console.WriteLine($"Machine name: {Environment.MachineName}");
break;
}
}
}
Here's a snapshot of the invert if[^] that still shows up in VSC as a hint.
Here's the code you get if you invert now:
static void Main(string[] args)
{
if (args.Length >= 1)
{
switch (args[0].ToLower())
{
case "os":
{
Console.WriteLine($"OS : {Environment.OSVersion}");
break;
}
case "pwd":
{
Console.WriteLine($"The current directory is: {Environment.CurrentDirectory}");
break;
}
case "cl":
{
Console.WriteLine($"Command line was: {Environment.CommandLine}");
break;
}
case "sysdir":
{
Console.WriteLine($"System dir: {Environment.SystemDirectory}");
break;
}
case "mname":
{
Console.WriteLine($"Machine name: {Environment.MachineName}");
break;
}
}
}
else
{
Console.WriteLine("Please provide 1 argument to indicate the command you want to run.\nUsage: getInfo <command-name>");
return;
}
}
|
|
|
|
|
That's so bizarre. It seems like the purpose of this rule (from some short googling) is to reduce conditional nesting and/or cyclomatic complexity. But in this example it actually increases nesting by changing away from an early-exit guard clause and doesn't change the CC at all. It's like it's doing the exact opposite of what it should be doing
|
|
|
|
|
Jon McKee wrote: seems like the purpose of this rule (from some short googling) is to reduce conditional nesting and/or cyclomatic complexity.
Thanks for researching that and confirming that you find it odd too. And for putting some data behind it. I really thought it was strange but hadn't defined why very well.
Great info.
|
|
|
|
|
I don't like the args.Length < 1 test - it's either going to be greater than or equal to 1, or equal to 0.
Just for fun, you could also introduce some C# 8 into the mix, and support multiple commands:
static void Main(string[] args)
{
if (args.Length == 0)
{
Console.WriteLine("Please provide 1 argument to indicate the command you want to run.\nUsage: getInfo <command-name>");
return;
}
foreach (string arg in args)
{
Console.WriteLine(arg.ToLowerInvariant() switch
{
"os" => $"OS : {Environment.OSVersion}",
"pwd" => $"The current directory is: {Environment.CurrentDirectory}",
"cl" => $"Command line was: {Environment.CommandLine}",
"sysdir" => $"System dir: {Environment.SystemDirectory}",
"mname" => $"Machine name: {Environment.MachineName}",
_ => $"Unknown command: '{arg}'",
});
}
}
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
Richard Deeming wrote: I don't like the args.Length < 1 test
Yeah, admittedly this is just shorthand for "I only want to know if it is 0" otherwise keep on running. I talk about that in relation to the code however -- since this is really just part of a beginning code sample that is part of a larger context of explaining how to get arguments from command-line. I talk about this being a cheat that will simply ignore any arguments that are after the first one.
This is all part of a book (Programming Linux With .NET Core) and part of the beginning stages of teaching an early beginner.
The foreach code is a really good example of things that will come in later explanations and is much more elegant. Thanks
|
|
|
|
|
Whoa! Hold the phone. I hadn't really dived into the C# 8 stuff yet. So a switch supports an expression syntax now? That's clean.
|
|
|
|
|
|
That's a tool, not even a suggestion. And tools might be used when they are useful.
So: when?
E.g. you decided to add an else clause, and you prefer the order the other way round.
E.g. you have both the normal and the else clause, and decide the other way round looks better.
Then, the new code is just a mouse click away.
A tool. To be used when appropriate.
Not:
Here's a hammer. Now you have to use it, even when there's no nail.
Oh sanctissimi Wilhelmus, Theodorus, et Fredericus!
|
|
|
|
|
Bernhard Hiller wrote: Here's a hammer. Now you have to use it, even when there's no nail.
But if you only have a hammer... would not come the moment when all what you see is a nail?
M.D.V.
If something has a solution... Why do we have to worry about?. If it has no solution... For what reason do we have to worry about?
Help me to understand what I'm saying, and I'll explain it better to you
Rating helpful answers is nice, but saying thanks can be even nicer.
|
|
|
|
|
I agree. It's just that any time a tool attempts to have some "intelligence" about it then you begin to think it may have more context to the situation. But, this probably really is more like pop up code snippets and you take them or leave them. Also, because it has the initial pop up that states, "Show fixes (Ctrl+.)" it leads you to believe that something is wrong.
|
|
|
|
|
I find the second example in your post to be problematic from the standpoint that it will add unnecessary nesting of main program logic. The first example shows a quick exit on invalid function parameters.
|
|
|
|
|
I agree! Thanks
|
|
|
|
|
One possibility is that this is a performance suggestion. Because of pre-fetching, many CPUs perform better when the non-jump case is taken. If the tool is guessing that the non-jump case is actually the rare one, it might be making the suggestion on those grounds. My guess is not, but I wanted to mention it. I worked on a product where the tags <usual> and <rare> could be used to tag code paths to assist the compiler with optimization. We had customers who would raise hell if a release slowed down by more than 1%-2%, despite all their feature requests, so tagging frequently invoked functions this way alleviated some of the performance degradation.
|
|
|
|
|
Greg Utas wrote: Because of pre-fetching, many CPUs perform better when the non-jump case is taken. If the tool is guessing that the non-jump case is actually the rare one, it might be making the suggestion on those grounds.
This is an interesting idea and viewpoint.
Greg Utas wrote: worked on a product where the tags <usual> and <rare> could be used to tag code paths to assist the compiler with optimization.
Sounds like an interesting and challenging project. Good info, thanks for chiming in. 
|
|
|
|
|
I prefer your version, but VS can get weird with it's hints.
Sometimes it makes a suggestion, you let it implement it, and it immediately suggests something else and offers you the original code you wrote.
For example:
StringBuilder sb = new StringBuilder(120); Suggestion: Use an implicit type:
var sb = new StringBuilder(120);
Now it suggests Use an explicit type instead of 'var':
StringBuilder sb = new StringBuilder(120); Which is what I prefer anyway ...
"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!
|
|
|
|
|
That is a great example and it is really terrible.
But, it gives me hope that when AI does take over it will become confused and get into these circles of logic and I will step in and pull the plug and fix it all. 
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
