3 ms·

Your criticism seems a bit overboard. You are making assumptions about things you don't know about, and are judging personal choice floating point representatio

• slackingoff2017 9y ago
  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.
  • taspeotis 9y ago
    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/FST.Common/Database.cs#L122 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).
    • slackingoff2017 9y ago
      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.
      • MichaelGG 9y ago
        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.
    • MichaelGG 9y ago
      I started using C# during the preview. I've never used CreateCommand.