Is it Intellisense-abuse?

djork

I found a few odd classes in our company application framework. The basic pattern I see is that there is one class called SomeKindOfThingName, and then a class called SomeKindOfThingItem, where the "Name" class has nothing but read-only properties that return new instances of the "Item" class.

The "Item" class, however, is nothing more than a class that has one instance variable, a string, that is set in the constructor, and accessed through a get accessor. This just seems like a waste of good code and keystrokes, when I could just type "Sprocket" when I need it instead of WidgetItemName.Sprocket. Someone is obviously deathly afraid of "hard coding," to a fault

public class WidgetItemName {
	public WidgetItem Sprocket {
		get { return new WidgetItem("Sprocket"); }
	}
public WidgetItem Gadget {
	get { return new WidgetItem("Gadget"); }
}

}
public class WidgetItem {

private string _Text;
public WidgetItem(string Text) {
	_Text = Text;
}

public Text {
	get { return _Text; }
}

... all sorts of operators follow (=, <, >)

}

sirhegel

Hard coding values, such as strings, should be avoided as far as possible, and IntelliSense helps to type in values quickly! How about this example:

     Public Enum FirstName
        John
        Dick
        Elaine
    End Enum

    Public Enum LastName
        Harrison
        Smith
        Ivanovich
    End Enum

    Public Function GetFirstName(ByVal Name As FirstName) As String
        Return Name.ToString
    End Function

    Public Function GetLastName(ByVal Name As LastName) As String
        Return Name.ToString
    End Function

    Sub Main()
        Console.WriteLine("My name is " & GetFirstName(FirstName.Dick) & " and my last name is " & GetLastName(LastName.Harrison))
    End Sub

PSWorx

We don't know the context in which these classes are used, but it would make some sense to me if the plan to localize the project. Instead of search-and-replacing all the string occurences, they conveniently have them all on one place.

Don't ask me though, why they didn't simply use resources then...

Another reason could be that they planned to add more information to the [...]Item class in future. So with this design, they already have a convenient and consistent container where they can add this data.

Doesn't seem like a WTF per se to me, although it could very likely be one.

PSWorx

What's definately a WTF though, is that they actually create a new [...]Item object, every time someone accesses a [...]Name property, even though the returned objects are absolutely identical. That seems just a waste of memory and processing time to me...

Also, shouldn't the members of [...]Name at least be static? o.O Did the guy who wrote this actually know what "static" means?

...okay, I revert my post above, that thing IS a WTF...

djork

My bad, they are indeed static in the original code. As for the use cases of these classes... just an enum would do the trick entirely. I'll have to post the entirety of the *Item class when I get to work. It's fun.

djork

Here we go. This is really used as a pattern in several places. That's right, there are many classes with basically the exact same code but different names.

public class SomeKindaItem
{
	private string _Text = string.Empty;
public SomeKindaItem(string text)
{
	this._Text = text;
}
public string Text
{
	get
	{
		return this._Text;
	}
}
public override string ToString()
{
	return this.Text;
}
public static bool operator== (SomeKindaItem lhs, SomeKindaItem rhs)
{
	return lhs.Text == rhs.Text;
}
public static bool operator!= (SomeKindaItem lhs, SomeKindaItem rhs)
{
	return lhs.Text != rhs.Text;
}
public override bool Equals(object obj)
{
	return base.Equals (obj);
}
public override int GetHashCode()
{
	return base.GetHashCode ();
}

}

PSWorx

Okay... overriding the base methods with functions that do nothing than ... call the base methods seems slightly silly to me. Apart from that though, it looks to me like an infrastructure built with future extensions in mind. The classes may be identical now, but if other members should be added some time in the future, they may not be anymore. Likewise, equality tests and everything may become more complex. Okay, you could propably tuck away those methods in some base class instead of copy-pasting them, but if the classes (albeit i fact being identical right now) had completely different purposes, you might end up with a class hirarchy that has no logical counterpart in your design scheme. Which would be an even bigger WTF IMO.

RaspenJho

It looks like an incomplete implementation of IEqualityComparer. (It doesn't actually implement the interface). Implementing the interface requires that you provide the GetHashCode and Equals methods. The other overrides don't do the same thing as the base class at all.

too_many_usernames

Seems more like an abuse of OOP than IntelliSense to me... o.0

PSWorx

@RaspenJho said:

The other overrides don't do the same thing as the base class at all.

I meant to refer to GetHashCode and Equals here, sorry for the bad wording.

On second view, that code does have a few quirks though. What's up with String.Empty instead of just ""? Same goes for the overuse of the [code]this[/code] keyword. This could indeed be a sign for intellisense abuse but for a five char word? I mean, come on, you had to type "this" so you saved a single keystroke...

Maybe this code was machine generated?

RaspenJho

String.Empty executes faster than "".

Using "" in your code will create a new string object every time it occurs. String.Empty always references the same empty string.

I can't speak to the "this" usage. It's the same kind of discussion you get when talking about hungarian notation and such.

djork

@RaspenJho said:

String.Empty executes faster than "".

Using "" in your code will create a new string object every time it occurs. String.Empty always references the same empty string.

I can't speak to the "this" usage. It's the same kind of discussion you get when talking about hungarian notation and such.

But in practice, it makes no difference. Really, both of them are exactly as fast in reality, that is they are both [i]incomprehensibly fast[/i] in human terms. You'll never reclaim in CPU time what you waste in typing "string.Empty"

\

As for the "this." usage... that's just some cargo cult programming that goes on here. I did a search on our code base and found eighty thousand "this." constructs that basically have no reason to exist.

RaspenJho

@djork said:

You'll never reclaim in CPU time what you waste in typing "string.Empty"

I don't agree with this statement.

If you are filtering user input then maybe, but if you are reading a million+ line file, with multiple columns that may contain nulls or zero-length strings, then it is always best practice to eek out the best performance you can get.

sirhegel

There is yet another good reason for referring to String.Empty instead of "". Whoever knows if Microsoft decides to change the very definition of an empty string in the future? Or if the empty string may be platform- or culture-specific. ;-)

RaspenJho

<sarcasm>
They could trim up the framework by treating System.DBNull the same as String.Empty, ala Oracle-style...
</sarcasm>