// if we don't have frequencies, use the default (this should never happen, but here it is)
if (tblFreq == null || tblFreq.Rows.Count == 0)
{
a = (float)0.02;
b = (float)0.02;
}
is kind of fishy. Why not bomb out instead of failing silently for things that are never supposed to happen? Not saying this is inherently wrong - I'm certainly not capable of actually understanding the details of the analysis
This guy that wrote this is a c# noob. He should just use a null conditional for the if, Epsilon for tiny but non-zero floats, and should very likely be using doubles instead of float since they're basically the same speed for double the precision.
Bonus points for not using the shorthand float literal syntax.
I would expect to see this many goofs only from someone that's been using C# for a month or so. I only used it for a couple years
Your criticism seems a bit overboard. You are making assumptions about things you don't know about, and are judging personal choice floating point representation.
I can overlook a lot of coding issues, but seeing so many in a few lines is a sign somebody doesn't know what they're doing. Just using a linter would probably find some of these automatically, so the fact that they're there is a strong signal that the dev didn't even know/care to use one.
I'm making assumptions based on experience with C# codebases and I think my argument on double vs float is solid. I don't think I've ever seen floats used in a production C# codebase because there's no point.
Overboard maybe :) but I still think whoever wrote that is very inexperienced.
Using out parameters, these are highly discouraged. C# let's you do terrible things (including goto) but the community is pretty aligned that it's bad practice. In my years of doing C# I've never used one once.
Not using var anywhere. This is C# type inference 101.
Throwing "Exception" in catch with only a message. This is horrific because it throws away the original context and any stack trace. You can just say "throw" (just the one word) or rethrow the exception with an innerexception with the original info trivially. This is unforgivable, an experienced C# developer would never do this.
Extraneous uses of "this" when it's the default.
Initializing variables to null
Not using shorthand object initializers(but this is only in newer versions of C#)
Using class variables instead of properties
Using parse instead of TryParse... Like Java exception handling is an expensive stack unwind on failure, not supposed to be used for control flow.
The worst though, is how did they didn't make a method to reuse the SQL connect and data table code and instead copy pasted it around 20 times.
If the input parsed is supposed to be well formed or isn't in a high performance area it's totally fine to use parse rather than the sorta ugly TryParse (which require using out params you just said were bad).
