Hacker News Reader

Comment by devillighter | Hacker News Reader

Code in "Comparison.cs" on line 1307

   // 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

View on HN

The only thing that should follow a "This should never happen" is an assert or exception.

Do we know who wrote this or at least which city agency? Or was it done by consultants or acquired?

Unless it’s a uniform prior... this is bad.

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.

Looks pretty bad to me. Null conditional doesn’t really matter because the code likely predates its existence. But...

https://github.com/propublica/nyc-dna-software/blob/master/F...

1. Knows how to use "using" but doesn't use it for myConnection.

2. myConnection.CreateCommand would be more idiomatic than new SqlCommand ... cmd.Connection = myConnection;

3. Doesn't know how to use "using"! using (DataTable myDt ... then goes on to "return myDT"

4. Also just noticed the oddity of myAdapter.SelectCommand = cmd although cmd is immediately disposed after that, then Fill is called later.

That's one function although the rest seem to repeat similar sins (CTRL+C, CTRL+V).

Nice! I can see more too.

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).

Especially if all they're going to do is throw.

I started using C# during the preview. I've never used CreateCommand.

Reply on news.ycombinator.com