Home .NET Top 10 mistakes in C# projects in 2016

Top 10 mistakes in C# projects in 2016

by admin

Top 10 mistakes in C# projects in 2016
To evaluate the quality of our analyzer and to popularize the static analysis methodology, we regularly check open-source projects forerrors and write articles about them.The year 2016 was no exception and it was also noteworthy because it was a time of peculiar "growing up" of the C# analyzer. PVS-Studio got a lot of new C# diagnostics, an improved mechanism of handling virtual values (symbolic execution) and much more. Based on the results of our team’s work, I’ve compiled a kind of hit list of the most interesting errors found in C# projects in 2016.
Tenth place : when there are not always 60 seconds in a minute
I will start my hit list with a bug that was detected while checking the Orchard CMS project. A description of the bug can be found at article In general, all articles about checking projects can be found here
V3118 Secondscomponent of TimeSpan is used, which does not represent full time interval. Possibly ‘TotalSeconds’ value was intended instead. AssetUploader.cs 182

void IBackgroundTask.Sweep(){....// Don't flood the database with progress updates;// Limit it to every 5 seconds.if ((_clock.UtcNow- lastUpdateUtc).Seconds > = 5){....}}

Instead of TotalSeconds in this case the developer mistakenly used Seconds Thus, it will not be the full number of seconds contained between the dates _clock.UtcNow and lastUpdateUtc as calculated by the programmer, but only the residual value of the range. For example, for a range value of 1 minute and 4 seconds, the result will be 4 seconds, not 64. Unbelievable, but even experienced developers make such mistakes.
Ninth place : the expression is always true
The following error is given in article about checking the GitExtensions project.
V3022 Expression ‘string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)’ is always true. GitUI FormFormatPatch.cs 155

string rev1 = "";string rev2 = "";var revisions = RevisionGrid.GetSelectedRevisions();if (revisions.Count > 0){rev1 = ....;rev2 = ....;....}elseif (string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)) // <={MessageBox.Show(....);return;}

Note the keyword else It probably doesn’t belong here at all. Careless refactoring or banal programmer’s tiredness and now we have a drastic change of program logic leading to unpredictable behavior. It’s good that the static analyzer never gets tired.
Eighth place : possible typo
In article about checking the FlashDevelop source code, an interesting typo error is given.
V3056 Consider reviewing the correctness of ‘a1’ item’s usage. LzmaEncoder.cs 225

public void SetPrices(....){UInt32 a0 = _choice.GetPrice0();UInt32 a1= _choice.GetPrice1();UInt32 b0 = a1 + _choice2.GetPrice0(); // <=UInt32 b1 = a1 + _choice2.GetPrice1();....}

I agree with the analyzer as well as the author of the article. Instead of the variable a1 in the marked line we should use a0 In any case, it would be a good idea to give the variables clearer names.
Seventh place : logical fallacy
Motivated by the Umbraco project re-examination, it was also written by article An example of what I think is an interesting mistake from that article.
V3022 Expression ‘name!= «Min» || name != «Max»’ is always true. Probably the ” operator should be used here. DynamicPublishedContentList.cs 415

private object Aggregate(....){....if (name != "Min" || name != "Max"){throw new ArgumentException("Can only use aggregate min or max methods on propertieswhich are datetime");}....}

Exception of the type ArgumentException will be thrown at any value of the variable name All because of the mistaken use of || instead of operator in the condition.
Sixth place : erroneous loop exit condition
Article about checking the Accord.Net project contains descriptions of several interesting bugs. I chose two, one of which again involves a typo.
V3015 It is likely that a wrong variable is being compared inside the ‘for’ operator. Consider reviewing ‘i’ Accord.Audio SampleConverter.cs 611

public static void Convert(float[][] from, short[][] to){for (int i= 0; i < from.Length; i++)for (int j= 0; i < from[0].Length; j++)to[i][j] = (short)(from[i][j] * (32767f));}

The error is in the second loop condition for , the counter of which is the variable j Using variable names of the form i , j for counters is a kind of classic of the genre. Unfortunately, these variables are very similar in spelling and developers often make typos in this kind of code. I do not think that in this case we should recommend the use of more meaningful names. No one will do so anyway :). So I will give you another recommendation: use static analyzers!

Top 10 mistakes in C# projects in 2016

Fifth place : using bit operator instead of logical operator
Another interesting and fairly common error from article about the Accord.Net project verification.
V3093 The ” operator evaluates both operands. Perhaps a short-circuit ” operator should be used instead. Accord.Math JaggedSingularValueDecompositionF.cs 461

public JaggedSingularValueDecompositionF(....){....if ((k < nct) (s[k] != 0.0))....}

Obviously, even if the first condition is not satisfied, the erroneous use of the operator instead of will cause the second condition to be checked, which in turn will cause the array to be out of bounds.
Fourth place: one quote, two quotes
In fourth place is an error from article About checking the Xamarin.Forms project.
V3038 The first argument of ‘Replace’ function is equal to the second argument. ICSharpCode.Decompiler ReflectionDisassembler.cs 349

void WriteSecurityDeclarationArgument(CustomAttributeNamedArgument na){....output.Write("string('{0}')", NRefactory.CSharp.TextWriterTokenWriter.ConvertString((string)na.Argument.Value).Replace("'", "'"));....}

In this case, it will replace the quote with a… quote. I don’t think this is exactly what the developer was looking for.

Top 10 mistakes in C# projects in 2016

And the analyzer is good!
Third place : ThreadStatic
The Mono project, about the verification of which is written article , turned out to be rich in interesting bugs. And one of them is a rare guest indeed.
V3089 Initializer of a field marked by [ThreadStatic] attribute will be called once on the first accessing thread. The field will have default value on different threads. System.Data.Linq-net_4_x Profiler.cs 16

static class Profiler{[ThreadStatic]private static Stopwatch timer = new Stopwatch();....}

In brief: an invalid initialization of a field marked with the attribute ThreadStatic. In documentation to the diagnostic provides a detailed description of the situation and tips on how to avoid such mistakes. A great example of an error which is not easy to find and correct by normal means.
Second place : Copy-Paste, the benchmark!
One of the reference, in my opinion, examples of Copy-Paste type errors is contained in the previously mentioned article about checking the Mono project.
V3012 The ‘?:’ operator, regardless of its conditional expression, always returns one and the same value: Color.FromArgb (150, 179, 225). ProfessionalColorTable.cs 258
Here is a brief code snippet where the error was found :

button_pressed_highlight = use_system_colors ?Color.FromArgb (150, 179, 225) :Color.FromArgb (150, 179, 225);

You might ask, "What’s the big deal?" Was it really necessary to put such an obvious error on the second place of the hit-parade? The point is that this code fragment was formatted additionally for clarity. Now get ready and imagine that you have a task to find such an error in your code without using any tools. So, please leave those weak-willed people alone. Below is a screenshot of a full code fragment with the error :

Top 10 mistakes in C# projects in 2016

It’s obvious that any programmer can make such an error regardless of his/her skill. And successful search for such errors shows the power of static analysis.
First place : PVS-Studio
Yes, you are not imagining things. It really does say "PVS-Studio. And it is in the first place of our hit-parade. But not only because it’s a good static analyzer. It is also because we have ordinary people working in our team who make common human errors in code. This is exactly what was once written about article An error, or to be more exact, two of them at once, which were detected in the PVS-Studio code with the help of PVS-Studio.
V3022 Expression ‘RowsCount > 100000’ is always false. ProcessingEngine.cs 559
V3022 Expression ‘RowsCount > 200000’ is always false. ProcessingEngine.cs 561

public void ProcessFiles(....){....int RowsCount =DynamicErrorListControl.Instance.Plog.NumberOfRows;if (RowsCount > 20000)DatatableUpdateInterval = 30000; //30selse if (RowsCount > 100000)DatatableUpdateInterval= 60000; //1minelse if (RowsCount > 200000)DatatableUpdateInterval = 120000; //2min....}

The result of this code fragment (provided that RowsCount > 20000 ) will always be the value DatatableUpdateInterval equal to 30000.
Fortunately, we have already done some work in this direction.

Top 10 mistakes in C# projects in 2016

Due to the widespread use of incremental analysis in our team, it is highly unlikely that there will be articles about finding errors in PVS-Studio with PVS-Studio in the future.
I would like to point out that now any of you can make your own hit list of errors found by taking the opportunity to free PVS-Studio static analyzer to check your own projects.
Download and try PVS-Studio: http://www.viva64.com/ru/pvs-studio/
For questions about purchasing a commercial license, please contact with us in the mail. You can also write to us to get a temporary license for a comprehensive study of PVS-Studio if you want to remove restrictions of the demo version.
Top 10 mistakes in C# projects in 2016

If you want to share this article with an English-speaking audience, please use the translation link : Sergey Khrenov. Top 10 C# projects errors found in 2016
Read the article and have a question? Often the same questions are asked to our articles. We’ve compiled the answers to them here : Answers to questions from readers of articles about PVS-Studio, version 2015 Please review the list.

You may also like