Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I came across polymorphism in the book that I'm reading and decided to do a little experiment. Essentially what I did was to create a base class called Asset and two subclasses that derive from Asset, called Property and Stock. I created instances of these two types and passed them to this function:

public static void PrintAsset(Asset theAsset)
{
    Console.WriteLine(theAsset.name);
    Console.WriteLine(theAsset.GetType().ToString());

    string x;

    if (theAsset.GetType() == typeof(Stock))
    {
        Stock theStock = (Stock)theAsset;
        x = (theStock.numShares * theStock.stockPrice).ToString();
    }
    else
    {
        Property theStock = (Property)theAsset;
        x = (theStock.value).ToString();
    }

    Console.WriteLine(x + "\n");
}

Initially, the method only consisted of the first two lines, and the output shocked me since I would've figured the incoming reference (theAsset) would've been cast to Asset, but .GetType().ToString() surprisingly produced "...Stock" and "...Property" in the Console output.

I'm assuming the fact that they allow you to see the true class of the object being passed to the method for a reason, so doing something like this should be considered acceptable, but I was wondering if perhaps you SO/SE folks might disagree. Is there some unforeseen problem that this causes?

share|improve this question
1  
Just because something is allowed doesn't mean it's a good idea to do it. –  svick Feb 14 '13 at 11:08
add comment

1 Answer

You have to learn a lot about OOP... There is no polymorphism in your code, as this principle is designed to tackle exactly the kind of code you've written.

The code that uses Asset should never branch its logic based on actual derived type (otherwise it would brake Open-Closed Principle), instead classes should use polymorphism to specify differences in logic. Here is simple example based on your code:

public abstract class Asset
{
    public string Name {get; set;}
    public abstract decimal CalculateValue();
    //... other common properties and methods of Asset
}

public class Property : Asset
{
    public decimal Value {get;set;}

    public override decimal CalculateValue()
    {
        return Value;
    }
}

//usage
public static void PrintAsset(Asset theAsset)
{
    Console.WriteLine(theAsset.Name);
    Console.WriteLine(theAsset.GetType().ToString());

    string assetValue = theAsset.CalculateValue().ToString();
    Console.WriteLine(assetValue + "\n");
}
share|improve this answer
    
Everything is reasonable. But I wouldn't say that in original code there is no polymorphism. PrintAsset() takes the whole family of types which belongs to the class Asset directly or indirectly (descendants Stock and Property). Thus that function is kinda polymorphic (handles more than one type). And have type-dependent behaviour (which isn't an indicator of polymorphism). Usually in OOP-emphased languages only functions that use leaf types (i.e. in C# that enforced for sealed classes and structs) are polymorphic. –  ony Feb 14 '13 at 11:25
    
You probably ment Liskovs Substitution Principle and not OCP. –  jgauffin Feb 14 '13 at 12:43
1  
Liskov's substitution principle will also be broken, but I did mean Open-Closed Principle. From Wikipedia: "The idea was that once completed, the implementation of a class could only be modified to correct errors; new or changed features would require that a different class be created" (open to extension, closed to modification) –  almaz Feb 14 '13 at 12:50
    
I understand abstract/virtual/override, but I'm afraid I don't yet understand o/c principle. Hopefully I'll understand it after reading this 1,000-page book! As far as not performing the calculation of the value via member functions (i.e. methods, which would be very OOP), I've been mainly working in VB6, and OOP is a bit sketchy there, so my VB6 code was a 50/50 mix of OOP and procedural. In C#, I would indeed use an OOP approach (as opposed to the static void external to the classes on which it operates shown here), but I'm afraid I don't understand your justification for doing it that way. –  Michael Feb 15 '13 at 10:44
1  
There is a set of recommended principles for writing "proper" OOP code called SOLID, and Open-Closed Principle is one of them. It's probably better for you to read (and understand) those principles first, don't think I can explain it better... As to my justification - your code will not work properly if I pass something other than Stock or Property. So it means that whenever someone adds a new type of asset he should update this method as well. It breaks Open-Closed Principle, and make code hard to maintain –  almaz Feb 15 '13 at 11:11
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.