I've recently started at another job and the project uses one C# (Windows Forms) application. Now I had never before programmed in C# ("only" in C, C++, Objective-C, Java, LISP, and a few others), but it looks as if my predecessor had never programmed before at all. Here is one awful bit of code, the body of a function with a single parameter (enumparameter) that can take three different values. This is a sort of "spot the differences". I replaced the long variable names by p1 until p9:

bool bResult = false;
switch( enumParameter )
{
    case enumvalue1:
        bResult = AppInst.Instance.Database.UpdateX( enumvalue1,
            p1, p2, p3, p4, p5, p6, p7, p8, p9, 
            (Int32)( new TimeSpan( m_ViewSettings.start.Ticks - m_ViewSettings.refStart.Ticks ) ).TotalHours );
        break;
    case enumvalue2:
        bResult = AppInst.Instance.Database.UpdateX( enumvalue2, 
            p1, p2, p3, p4, p5, p6, p7, p8, p9, 
            (Int32)( new TimeSpan( m_ViewSettings.start.Ticks - m_ViewSettings.refStart.Ticks ) ).TotalHours );
        break;
    case enumvalue3:
        bResult = AppInst.Instance.Database.UpdateX( enumvalue3,
            p1, p2, p3, p4, p5, p6, p7, p8, p9, 
            (Int32)( new TimeSpan( m_ViewSettings.start.Ticks - m_ViewSettings.refStart.Ticks ) ).TotalHours );
        break;
    default:
        break;
}
if ( true == bResult )
{
    bResult = SaveX( p2, p3, enumParameter, description );
}

Yes, the calls are all identical except for the enum value. This could easily have been replaced by a single call. And the (Int32) cast is a bit superfluous as well. Oh, and the use of .Ticks --I think-- is better replaced by the Subtract method or whatever it is called.

There is more enum madness:

public enum E_LogActionType
{
    Copy = 0,
    Compute = 1,
    Approve = 2,
    Reject = 3,
    Other = 4
};

Where did the need come from to give them all explicit values? There is also such an enum with all days of the week.


Then there is Programming By Pattern Obfuscation. In quite a few classes, there are members called m_someThing, and then there is a property SomeThing with a trivial get and set method. What's the point? I know for sure he was not payed per LOC, so why?

And then I found the mother of shooting yourself in the foot with C#. One definition of it is "You shoot yourself in the foot, and the only reason C# lets you, is because that feature exists in Java." However, I discovered that it offers a special way to do it better. I tried to figure out where the connection to the database gets opened. It was nowhere to be seen, or so I thought. Then I stopped the debugger at the first call to the ODBC, traced back the stack and there it was:

ApplicationInstance.Instance.Database.OpenDatabase = true;

Yes, the set property for OpenDatabase actually opens the database when set to true. And closes it when set to false, although this never happens in the application. Something that looks to any non-C# programmer as a harmless assignment turns out to be a function call. Talk about side effects!

Ok, they're not horribly great WTFs, but my first encounter with C# did not make me think that I should have taken it on earlier...

WTFy for sure, but it doesn't look like any of this is the fault of the language to me.  WTFiness transcends languages!

TGV:

In quite a few classes, there are members called m_someThing, and then there is a property SomeThing with a trivial get and set method.

To be honest I thought this was fairly standard in C# to avoid declaring members as public, at least prior to VS2008 where you can just write

public SomeThing { get; set; }

TGV:

I've recently started at another job and the project uses one C# (Windows Forms) application. Now I had never before programmed in C# ("only" in C, C++, Objective-C, Java, LISP, and a few others), but it looks as if my predecessor had never programmed before at all. Here is one awful bit of code, the body of a function with a single parameter (enumparameter) that can take three different values. This is a sort of "spot the differences". I replaced the long variable names by p1 until p9:

SNIPped to prevent making readers' brains bleed twice

Yes, the calls are all identical except for the enum value. This could easily have been replaced by a single call. And the (Int32) cast is a bit superfluous as well. Oh, and the use of .Ticks --I think-- is better replaced by the Subtract method or whatever it is called.

I've seen someone do that in C. I once asked him, "Why do you do that, instead of just directly passing the enum value to the function?" His response was to stare open-mouthed, then say, astonished, "C lets you pass an enum value?" I wouldn't be at all surprised if the same sort of thing was in your predecessor's head...

TGV:

There is more enum madness:

public enum E_LogActionType
{
    Copy = 0,
    Compute = 1,
    Approve = 2,
    Reject = 3,
    Other = 4
};

Where did the need come from to give them all explicit values? There is also such an enum with all days of the week.

I'm gonna say it. Pointless enums are one of my pet peeves. Big time. They're legitimately useful in many places. That's why they exist. But I see them used for so many things that they shouldn't be used for, especially in databases. Storage is cheap, people, stop using integers for strings in your DBs... the self-documentation is worth it. And when you use enums to make it easier to use integers, not only does the DB itself become harder to understand later, but there will always be that one jackass who memorizes the numbers and uses them to save typing.

I will at least credit your guy for specifying numeric values. I once watched while an entire database went boom because the company switched to a different compiler and it assigned numbers on different rules than the previous compiler did. Oops...

TGV:

And then I found the mother of shooting yourself in the foot with C#. One definition of it is "You shoot yourself in the foot, and the only reason C# lets you, is because that feature exists in Java."

Fitting... A Java-coding friend of mine routinely bitches that half the ways you can shoot yourself in the foot in Java exist only because C++ has the relevant feature. :) Though a friend of mine, upon reading this, will have more justification for pronouncing the language name "C Hash" instead of "C Sharp."

I'm going to close by referencing an old music joke, the punchline of which is: "If you don't C sharp, you'll B flat." Negate the conditional, and I guess we can make a derivative joke...

I like that last one, it is a nice feature, when you don't abuse it like that.

TGV:

I tried to figure out where the connection to the database gets opened. It was nowhere to be seen, or so I thought. Then I stopped the debugger at the first call to the ODBC, traced back the stack and there it was:

ApplicationInstance.Instance.Database.OpenDatabase = true;

You can do things like this in many languages. For example, in perl, one can tie a scalar to a class, and then reading and setting the value on that scalar will call methods in the class.

That having been said, most programmers I've encountered won't abuse that functionality quite like that. Ouch.

TGV:

but it looks as if my predecessor had never programmed before at all.

TGV:

Then there is Programming By Pattern Obfuscation. In quite a few classes, there are members called m_someThing, and then there is a property SomeThing with a trivial get and set method. What's the point? I know for sure he was not payed per LOC, so why?

That whole m_ thing smells of Microsoft Visual C++. It used to be the nominated format for labeling member variables in classes, but is not used in C#. So I guess that your colleague *has* programmed in other languages, and is actually carrying over his C++ mannerisms into C#.

The whole get/set idiom in C# is the preferred way to expose public member class variables in a controlled manner. If you wanted to you could directly read/write the underlying member variable but then you lose the isolation between design and implementation which is not a good design practice. Isolating it properly means having an accessor function of some sort. Using get/set at least imposes a regular method of writing accessor functions rather than having every man and his dog writing a class accessor with a different signature. (Why are those methods called SetFred() and getFred() in this class, when we have BarneySet() and BarneyGet() in another class)

On the other hand abusing the get/set to perform functionality such as opening a DB is something your colleague should have been hung drawn and quartered for. I assume that his code never saw a review?

TGV:

There is also such an enum with all days of the week.

 

I'm assuming that neither of you know about System.DayOfWeek.

wolftaur:

I will at least credit your guy for specifying numeric values. I once watched while an entire database went boom because the company switched to a different compiler and it assigned numbers on different rules than the previous compiler did.

Quite true, but I usually make a point of commenting why (in your case, compiler portability) I did something that looks totally unnecessary. I also tend to do stuff like that to deal with unchangeable inherited databases over which I have no control. Again, with a comment indicating why I'm doing something that otherwise looks stupid.

snoofle:

wolftaur:

I will at least credit your guy for specifying numeric values. I once watched while an entire database went boom because the company switched to a different compiler and it assigned numbers on different rules than the previous compiler did.

Quite true, but I usually make a point of commenting why (in your case, compiler portability) I did something that looks totally unnecessary. I also tend to do stuff like that to deal with unchangeable inherited databases over which I have no control. Again, with a comment indicating why I'm doing something that otherwise looks stupid.

Yeah. That sort of stuff has to be commented or someone else will come by later and break it in horrible ways. Hell, even if you comment, that can happen, but at least if it's commented and you can prove it, you don't look like the dumb one. :)

Sometimes I see #defines used the same way. I actually use both. Which I use in any given situation isn't actually some hard and fast rule, either. But I have seen one truly mind-boggling way of doing this...

A table in the database named "enumerations", which had... Many. I looked at it, and thought, "Ok, someone's taking abstraction a few levels too far..."

TGV:

"You shoot yourself in the foot, and the only reason C# lets you, is because that feature exists in Java."

 

Properties in Java were actually added because of C#, not the other way around.  Java properties get a bad reputation because they are (were?) slow.  Languages like python have properties as well, but are assigned explicitly using a property() function or decorator. Properties are an essential way of executing code when a value has changed and keeping private members private, but I agree that using a property as through it were a method to open a db connection is pretty WTF.

TGV:

public enum E_LogActionType { Copy = 0, Compute = 1, Approve = 2, Reject = 3, Other = 4 };

 

I don't see a WTF here. If you should ever need to convert the enum's values to int or whatever, at least you get well defined values... still, I think the enum should be declared as public enum ... : int

TGV:

In quite a few classes, there are members called m_someThing, and then there is a property SomeThing with a trivial get and set method. What's the point? I know for sure he was not payed per LOC, so why?

Because 1) it's not good style to use public member variables and 2) it is the only way to later integrate validation code or other stuff when setting the value.

TGV:

ApplicationInstance.Instance.Database.OpenDatabase = true;

That's not C# specific - you can do that in Delphi, C#, Java, etc. Actually, this is quite common. For example:

• To run the .NET Timer (the one you place on your form), you set its Enabled property to true.
• Most properties of graphical components in .NET call the Invalidate() or Update() method when their value is changed (e.g. the background color).

While I agree with you that the property in this case is very poorly named, I don't see a WTF here.

I use C# every day at work and I like it a lot personally. I've used many other languages before and I liked them as well. I don't really care if people think C# is just a Java-look-alike or whatever. For me it's a great tool, no more, no less. And I always try to remember that while good programmers will write good code in any given programming language, bad programmers will write bad code no matter which language they use. Maybe your predecessor should take some classes on coding style ;-)

 

 

 

 

tdittmar:

TGV:

public enum E_LogActionType { Copy = 0, Compute = 1, Approve = 2, Reject = 3, Other = 4 };

 

I don't see a WTF here. If you should ever need to convert the enum's values to int or whatever, at least you get well defined values... still, I think the enum should be declared as public enum ... : int

 

I don't know about C#, but in C++ there are rules for automatic enum value assignment.  In short, if the first enumerant isn't given a value, it will become zero.  Any subsequent value-less enumerant will get the value of the previous enumerant plus one.  Thus, even something like enum { True, False, FileNotFound }; is perfectly well-defined and portable when it comes to the values.

Exactly, this is the same in C#, wtf is everyone talking about, the default basetype for an enum in C# IS int, declaring it as public enum ... : int is just stupid and unneeded, same for the 0..4

upsidedowncreature:

public SomeThing { get; set; }

The most common version is:

public SomeThing { get; protected set; }

marking the variable as read-only outside of the class it is in. Personally I'd prefer a keyword like "writeprotected" that you'd use instead of public, but still.

Having actual CODE in a set accessor should only be used for setting the underlying variable or updating or invalidating cached data based on what you're setting. e.g. changing a sprite's texture updating it's width and height, and not for actually DOING anything. Setting "bConnected" to true to connect to a database is just stupid.

Thief^:

upsidedowncreature:

public SomeThing { get; set; }

The most common version is:

public SomeThing { get; protected set; }

marking the variable as read-only outside of the class it is in. Personally I'd prefer a keyword like "writeprotected" that you'd use instead of public, but still.

 

you mean like:

 public readonly int SomeThing;

 

 

Thief^:

Having actual CODE in a set accessor should only be used for setting the underlying variable or updating or invalidating cached data based on what you're setting. e.g. changing a sprite's texture updating it's width and height, and not for actually DOING anything. Setting "bConnected" to true to connect to a database is just stupid.

I disagree.  Use property accessors when it makes the code using it easier to understand and read.  The official .NET design book agrees with you, but when I worked at Microsoft, everyone agreed that the guidelines book was exactly that "guidelines," which are to be ignored almost always.

tster:

Thief^:

public SomeThing { get; protected set; }

marking the variable as read-only outside of the class it is in. Personally I'd prefer a keyword like "writeprotected" that you'd use instead of public, but still.

 

you mean like:

 public readonly int SomeThing;

 

No, "readonly" makes a variable only be able to be written to in the class's constructor, which is different to "protected set", which makes a variable only be able to be written to from anywhere inside the class

EDIT: I don't really see much use in "readonly". It's almost identical to "const".

tdittmar:

TGV:

In quite a few classes, there are members called m_someThing, and then there is a property SomeThing with a trivial get and set method. What's the point? I know for sure he was not payed per LOC, so why?

Because 1) it's not good style to use public member variables and 2) it is the only way to later integrate validation code or other stuff when setting the value.

If it's not good style, it can only be because you're not supposed to change certain members. This is simply making the variable public with a different name. There is absolutely no point.

tdittmar:

TGV:

ApplicationInstance.Instance.Database.OpenDatabase = true;

That's not C# specific - you can do that in Delphi, C#, Java, etc. Actually, this is quite common. For example:

• To run the .NET Timer (the one you place on your form), you set its Enabled property to true.
• Most properties of graphical components in .NET call the Invalidate() or Update() method when their value is changed (e.g. the background color).

Perhaps they shouldn't have done that. At least .NET doesn't take it as far giving the class List a member ".Last", so you could do: list.Last = new_element (instead of .Add()). CommentMode = Smiley;

tdittmar:

Maybe your predecessor should take some classes on coding style ;-)

He certainly should. Today I've seen some other code that was really, well, different.

tster:

Use property accessors when it makes the code using it easier to understand and read.

Aye, there's the rub -- easier to understand and read according to whom?  I really dislike the idea that something that looks like an assignment should do much, if anything other than simple assignment.  I like the idea that if a specific statement in a language is designed to look so strongly like C, it should act like C.

But maybe I'm just an old fart who can't accept today's newfangled ways.

It's largely a moot point if you spend all of your time in one language for months or even years at a time.  But if you're like me and can easily be working in 5 different languages in the space of a week (ah, the joys of contract work), making the context switch for things that look the same but act different is a real PITA.

AssimilatedByBorg:

tster:

Use property accessors when it makes the code using it easier to understand and read.

Aye, there's the rub -- easier to understand and read according to whom?  I really dislike the idea that something that looks like an assignment should do much, if anything other than simple assignment.  I like the idea that if a specific statement in a language is designed to look so strongly like C, it should act like C.

But maybe I'm just an old fart who can't accept today's newfangled ways.

It's largely a moot point if you spend all of your time in one language for months or even years at a time.  But if you're like me and can easily be working in 5 different languages in the space of a week (ah, the joys of contract work), making the context switch for things that look the same but act different is a real PITA.

 

 so what if the accessor sets the member and notifies listeners that the value has changed?  The the caller all he cares is that the member is set.  When you are working in a language you should at least grasp the fundamentals of the language well enough to know what code is going to run and when.  Accessors in C# are basic and everyone writing code in it should understand what they do.

AssimilatedByBorg:

But maybe I'm just an old fart who can't accept today's newfangled ways.

Behold, the new wave of programming is here: Assignment-Oriented Programming!  No more pesky function calls with parameters!  All operations will be done with assignment, like such:

File file;
file.filename = "foo.txt"; // Opens the file
file.data += "Hello, World!"; // Append data to the file
Socket socket;
socket.remote_address = "www.google.com:80"; // Connect to Google
socket.http_path = "/search?q=the+daily+wtf"; // Search for our favourite site
System::console.screen += socket.http_data.as_plain_text; // Fetch content and display it in console as plain text
 

And the most wonderful thing is that most of the popular languages today can already do this with suitable libraries!  C++, C#, Java, Python, and others.  Don't be left behind, start learning AOP today!

tster:

 so what if the accessor sets the member and notifies listeners that the value has changed?  The the caller all he cares is that the member is set.  When you are working in a language you should at least grasp the fundamentals of the language well enough to know what code is going to run and when.  Accessors in C# are basic and everyone writing code in it should understand what they do.

I agree 100% that everybody writing C# needs to understand accessors, I'm just saying code that allows "surprising" things to go on behind the scenes does not necessarily make it easier to understand and read. Naturally, "surprising" is in the eye of the beholder -- the caller may very much care that lots of other things are occurring other than just "that the member is set".  Now the caller needs to care about the side effects of "assignments" just as much as function calls.  This is where you can cross the "easier to understand and read" line.

I'm not trying to argue that accessors are inherently bad, just pointing out that they can be used badly, like just about any programming language feature.

TGV:

If it's not good style, it can only be because you're not supposed to change certain members. This is simply making the variable public with a different name. There is absolutely no point.

So lets say that you have an integer field that is part of a class. This integer field will be used as the denominator of a fractional computation. Of course, we don't want a 0 as the denominator. Using this "absolutely no point" programming style, you only need to do the check in ONE PLACE, which would be in the "set" part of the property. Do you have an easier/better way of doing this check EVERY TIME this value is set?