+972 52-548-6969
The seven deadly sins for the developer (* some restrictions apply)
time to read 2 min | 354 words
Originally posted at 3/30/2011
I have seen all of those in production code…
-
throw new NullReferenceException();
-
public class AbstractController : Controller // or // public class AbstractPag : Page { public static bool IsAdmin { get;set; } }
-
public static class BlogMgr { public static void AddPostToBlog(Blog blog, Post post) { blog.Posts.Add(post); post.Blog = blog; } }
* I could only think of three, but I didn’t want to modify the title. Can you complete the list?
Comments
How about:
lock (new object()) {
...stuff
}
how about my favourite of all time
try
{
//something
}
catch
{
//absolutely nothing
}
This too:
try
{
// no code here - catching nothing :)
}
catch(Exception ex)
{
}
Can you explain more, why they are deadly sins? (I am a begginer)
Thank
How about:
try {
// ...
} catch(Exception ex) {
logger.Log(ex);
throw ex;
}
My favorite prod code was:
if(personId == null) {
/* null personId */
} else {
// do something with personId
}
OR
if(something.GetType() == typeof(TypeA)) {
var typeASomething = (TypeA)something;
// do something with type A thing
} else if(something.GetType() == typeof(TypeB)) {
var typeBSomething = (TypeB)something;
// do something with type B thing
} else if ...
public static class Helper
I'm refactoring one legacy project and this is quite often:
if(true) { //or other condition which is 100% true
return;
}
// hundreds of code lines here doing complex stuff, which never executes :)
T SuperDescriptiveFunctionName() {
// Doing something completely different...
// With matching totally unrelated comments...
}
Hi @Ako - Here's my take on them.
In (1) the developer is throwing a framework-level exception. For a developer encountering this it will be indistinguishable from just accessing the null reference in the first place and be a PITA to find and fix. In fact, the code to check this and throw is entirely unnecessary. The framework will detect the situation and throw the same exception.
(2) is storing something session specific in a static variable (which is site-wide). Very scary. Also, a nitpick, if a class has abstract in the name, I'd make it an abstract class
(3) The logic for how to perform this action belongs in one or the other of the classes. The very fact that you CAN write this method means that people can change the state of the model without using it. i.e. Someone might set the Blog property on the Post object without updating the Blog. This needs to be internal to the Post/Blog so that the operation is atomic.
Stream stream = new ...
// use stream
stream.Flush();
stream.Close();
stream.Dispose();
stream = null;
Instead of
using(var stream = new ...)
{
// use stream
}
What bothers me most is the null assignment, do people still don't know .net uses a garbage collecter and not reference counters?
// Ryan
These look more like WTFs than 'deadly'. Confusing and verbose perhaps, but 'deadly' ?
if(someString.Length <= 0)
{
}
[In the domain public getters increase coupling, public setters kill you]
person.Name= foo;
person.Age = bar;
repository.Save();
[Violation of Tell, don't ask]
{
}
public class Square : Rectangle
{
}
[violation of Liskov]
What amazes me the most is how often I can see the following code in production:
if ( sthThatEvaluatesToBool( args) ) {
return true;
} else {
return false;
}
Quite good collection is here: stackoverflow.com/.../common-programming-mistak...
I came across such things:
public static class WhatEver : IDisposable
{
}
Er, entitling an open-ended resultset the 'seven deadly sins'?
@Fredrik I doubt you have come across that! It is not possible for static classes to implement interfaces - and it wouldn't compile if you tried.
"public static bool IsAdmin" in a web app means every one is an Admin or could be an admin!
Response.QueryString["XXXX"].ToString()
In a win forms applications, something like this...
public abstract class FormBase : Form {
private void SubmitClicked(object sender, EventArgs e){
}
}
@Bob:
// quote
lock (new object()) {
...stuff
}
// end-quote
I'm very guilty of this one. It seemed like a really good fix at the time until I learned what it was really doing.
Here's a nasty one from the Xaml world:
public class CustomerViewModel : DependencyObject {}
ServiceLocator.Resolve()
One of my favourites:
DateTime startTime = DateTime.Now;
do
{
try
{
dataAdaptor.Fill(dataSet);
return dataSet.Tables[0];
}
catch (Exception err)
{
if (DateTime.Now.Subtract(startTime).TotalMinutes > 1)
{
throw new Exception(err.Message);
}
Thread.Sleep(3000);
}
} while (true);
Translation: If at first you don't succeed, try try again.....but only for a minute (taking a 3 second rest after every try), then give up!
Haha, throwing NullReferenceException, sweet chums...
Personally, I totally hate analogs to
if (foo == true)
{
return true;
}
else
{
return false;
}
instead of return foo;
class SomeClass {
static object syncRoot = new Object();
....
public void SomeMethod() {
lock (syncRoot) {
// perform db work in a transaction
}
}
}
@Krzysztof,
"ServiceLocator.Resolve()"
There's an app here that insists on doing this in every method, I won't go into the reasoning why because it's stupid and they won't listen to reason. But just for sh!ts and giggles, I just pulled open their solution from Source and did a quick count for a grand total of 3258 Resolves in a single app.
And so I'm contributing, here's one I caught where I used to work:
try
{
return true;
}
catch (Exception ex)
{
throw;
}
@Rob Eisenberg
What problem with this except that DependencyObjects are not marked as serializable?
[Test]
public void TestSomeThings()
{
TestWithParameters("blarg", "", expectedError, null, 4, null);
TestWithParameters("bad value", "exclude", expectedError, null, 2, null);
// and 10 more lines of this
}
The tests SHARE STATE.
I also saw tests that connect to a special unittest database and expect certain values to be present. (There's nothing wrong with connecting to an actual database rather than an in-memory database, precisely; but the tests should expect a blank database to start.)
Nulling out a reference to a disposed object might be an attempt to provoke an exception that almost certainly won't get caught if someone tries to use it again.
1) Premature optimization
2) Speculative generality
@Mike
about--
try {
// ...
} catch(Exception ex) {
logger.Log(ex);
throw ex;
}
I agree it smells, and it's definitely a cop-out. The developer is basically saying "I have no idea what could go wrong in this code, if anything. If something goes wrong, hopefully I'll find out later." But It makes me a little optimistic that the person is rethrowing. Usually I feel like I see a less evolved form of this, like:
catch(Exception ex) {
logger.Log(ex);
}
Rethrowing ex makes me feel much better about the situation.
@Luke:
If the developer doesn't know what could go with his code, he is facing shitload of problems anyway. No optimism is in order here...
Catch "Exception" only once in your app.
use "throw", not "throw ex;".
@Moti
Well, limited optimism definitely. At least the exception is not getting swallowed completely. Like I said, the rethrowing makes me feel a little better; this person has learned that "log and ignore" is not a good strategy, and at least they're favoring "log and fail" instead.
That's a really good point-- I hadn't thought known about this, but sure enough, "throw ex" loses the stack trace (perhaps it has other downsides as well). Thanks for pointing that out.
foreach (x in collection1) // ~ 3000 instances
foreach (y in collection2) ~ 7 instances
talk about N^3+1 :)
notice that the query doesn't depend on x
I swear I saw this in an application having multi-language as one of the main requirements:
if (language="en") { return "Hello"; }
else if (language="es") { return "Hola"; }
//and so on... for each string literal.
Now, try to add a new language... and search for this code in all of your application. My eyes are still crying after seeing this. =8-O
@frank
Why is deadly? It's just unnecessary code which runs fine without possible errors?
Any time I see commented out code checked in.
95% of times I see a Singleton or static class added.
Most times I see a class "newed" in-line.
Just about any comment generated by GhostDoc. (As being either completely pointless, or completely missing the point.)
I.e:
<summary
/// Gets the name of the unique.
<paramThe propname.
<paramThe name.
<paramThe prefix.
<returnsthe unique name
A minor "wtf":
try
{
// Do stuff
}
catch
{
throw;
}
literally around every block of code. (Some kind of weird template.)
Just lazy... only writing 3 out of 7.
public class SomeWrapper : IDisposable
{
private AnotherDisposableResource _resource = null;
public SomeWrapper(AnotherDisposableResource resource)
{
}
public void Dispose()
{
}
}
// C# .net code
Application.DoEvents();
NOOOOOOOOOOOO!!!! THE MOST EVIL METHOD EVER WHEN USED ON A PLATFORM THAT FULLY SUPPORTS ADVANCED THREADING!!!
Debugging re-entrancy issues due to this old chestnut has consumed much of my life I will never get back.
@Tony
Of the original 7 deadly sins, from the Bible, I don't think any of them were actually deadly in an immediate sense. In fact, a few of them are quite enjoyable. Likewise, so are the deadly sins I've listed for programmers.
@Fredrik
How is it possible you've seen this in production c ode?
public static class WhatEver : IDisposable
{
// code
}
This would be a compile time error... static classes cant implement interfaces...
+1 for:
if ( sthThatEvaluatesToBool( args) ) {
return true;
} else {
return false;
}
and saw:
class FooViewModel
{
public void Show(ViewModelBase viewModel)
{
}
}
SomeClass instance = new SomeClass();
instance = FunctionThatReturnSomeClass();
@Simon (4/13/2011 2:39 PM): so true: the class itself was not static, of course, but almost its members and state were static, so it effectively became a "use-once-per-process" type.
Fun with singletons:
public class MySingleton
{
private static MySingleton _instance = null;
private MySingleton() { }
public static MySingleton GetInstance()
{
if(_instance == null)
{
// expensive initialisation logic that should only get called once here
}
return _instance;
}
}
And this is in a heavily traffic'd web app :-)
PS: Ayende, formatting code in these comments is such a pain
I saw this in a locked down TFS branch (accessible only by the architect)
public int GenericAddRecordToTableMethod(string SQL)
{
// instantiate connection objects etc
object result = SqlCommand.ExecuteNonQuery();
return Convert.ToInt32(result.ToString());
}
The rest of the site had similar issues with boxing/unboxing and casting down to object
Read this post and comments as I was reviewing some legacy code for a customer - not necessarily worthy of deadly sin status, but certainly worth a giggle.
[SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate", Justification =
public decimal GetSomeValueOrOther()
{//literally loads of code...}
Excuses, excuses excuses - just fix it already!
and of course within this very same method:
catch (Exception ex)
{
}
names changed to protect the guilty of course.
Thanks for a good laugh!
We had this lovely piece that made it into prod, for a short while until it was asked why it wasn't working. Of course it worked in dev/test where debug was enabled for logging and failed in prod where it was obviously set at error.
public void someMethod() {
....
....
}
Maybe this is more in the 'clearly the programmer does not understand pass by reference", but anyways:
public void ClearToken(Token token)
{
if (token != null) token = null;
}
Another good one:
try{
....
}catch{
MessageBox.Show("An error occured");
}
// Todo: Must fix
bool isValid = false;
....
if (isValid == null)
{
}
I've seen this a lot in the last couple of years, often in Microsoft sample code and conference sessions!
var a = b as C;
a.SomeMethod();
It seems that this viral confusion started somehow. Someone in a trusted position probably wrote some code or issued some best practice advice that since "as" is a "safe" cast which won't throw if it fails, it should be used instead of a regular typecast.
Then the likes of Scott Hanselman and Rob Conery started doing it.
NB: I can't be remember for sure that I saw this in either of these guys code, so I'm saying the likes of these guys ;)
Several days ago I stumbled on this in our Silverlight application:
public SomeUserControl() {
try {
}
catch(Exception) {
}
}
i found it so funny so I didn't touch this :)
@Steve py
I've seen your minor wtf a lot. People like to put a breakpoint in the throw; so that it stops there instead of crashing.
And yes, no one listens when I say there's an option to do that automatically.
This was actually in production for several years:
if(status == "Continue")
{
if(status == "Cancel")
{
}
//some logic
}
I got a good one
If(UserID == 123)
{
UserID = 321;
}
//... continue login process with a different user. I wonder what will happen on prod...
Seen a few good ones.
// we need to wait for middleware server for 5 seconds
DateTime currentTime = DateTime.Now
while((DateTime.Now - currentTime).TotalSeconds < 5) {
}
(or something to that effect, not tested). This made it to production server on one of most popular price comparison sites a few years ago, debugging 100% cpu kept the team going for while. One of the guys found it so funny he printed and framed it, but he was on the other side of the office so the victim was none the wiser.
Also someone decided to reinvent HtmlEncode and UrlEncode, inlined in same class doing the web scraping of course.
This little golden nugget was written by a contract developer earning a fairly decent daily rate. Needless to say he has gone!!
Private Function DoesFileNameStartWithATimeStampValue(ByVal FName As String) As Boolean
I like a lot this boolean redundancy like:
if (!confirm('A senha será cancelada, confirma?')) {return false;};
Or
bool validUser = Service.ValidateUser(Username, Password);
if (!validUser)
{
}
return true;
public void DoStuff()
{
// ...
return; // <- Really?
}
bool isJohnGalt = this.WhoIsJohnGalt();
if (isJohnGalt.Equals(true))
//do something
//better still
if(isJohnGalt = true)
//do something more
//yet another one
Customer item = new Customer();
while (reader.Read())
{
item.IsJohnGalt = reader.GetBoolean(0);
item.CustomerCode = reader.GetInt32(1);
item.Name = reader.GetString(2);
}
return item;
//Most common one - same as vjd pointed out earlier doing a ToString to something that is already a string
Comment preview