11
\$\begingroup\$

I had a test before and had to do this task:

Create classes: Cat, Dog and Petshop. Types Cat and Dog have fields Name and Breed and method Introduce() which prints text "I'm (Name) of (Breed). I'm a cat (or dog)".

The class Petshop collects different pets in its container. We may add new pet to container using method AddPet(), and we may display information about all pets by calling method IntroduceAll().

Which hierarchy of classes is the best solution of this problem?

Write short code to demonstrate your solution. Your code should include class (interface) aggregation, inheritance, should use .NET BCL collections or generics, and should implement exception handling.

My answer:

using System;
using System.Collections.Generic;

namespace ConsoleApplication1
{
    interface IPet
    {       
        string MyName { get; set; }
        string MyBreed { get; set; }
        void Introduce();
    }

Cat

    class Cat : IPet
    {
        string Name;
        string Breed;

        public Cat()
        {
            Name = "";
            Breed = "";                
        }

        public string MyName
        {
            get
            {
                return Name;
            }
            set
            {
                Name = value;
            }
        }

        public string MyBreed
        {
            get
            {
                return Breed;
            }
            set
            {
                Breed = value;
            }
        }

        public void Introduce()
        {
            Console.WriteLine("I'm " + Name + " of " + Breed + " I'm a cat");
        }        
    }

Dog

    class Dog : IPet
    {
        public string Name;
        public string Breed;

        public Dog()
        {
            Name = "";
            Breed = "";
        }

        public string MyName
        {
            get
            {
                return Name;
            }
            set
            {
                Name = value;
            }
        }

        public string MyBreed
        {
            get
            {
                return Breed;
            }
            set
            {
                Breed = value;
            }
        }

        public void Introduce()
        {
            Console.WriteLine("I'm " + Name + " of " + Breed + " I'm a dog");
        }        
    }

PetShop

    class PetShop
    {
        List<Object> objList = new List<object>();
        public void AddPet(Object obj)
        {
            objList.Add(obj);
        }
        public void IntroduceAll()
        {

            foreach (object element in objList)
            {
                Console.WriteLine("One more");
                Type mytype = element.GetType();
                if (mytype == typeof(Cat))
                {
                    var el = (Cat)element;
                    el.Introduce();
                }
                else if (mytype == typeof(Dog))
                {
                    var el = (Dog)element;
                    el.Introduce();
                }
                else
                {
                    throw new NotFoundException();
                }
            }
        }
    }

Program

    class Program
    {        
        static void Main(string[] args)
        {
            Cat pussy = new Cat();
            pussy.MyName = "Murlon";
            pussy.MyBreed = "Supernatural";
            Dog doggy = new Dog();
            doggy.MyName = "Oscar";
            doggy.MyBreed = "Blonde";
            Cat pussy1 = new Cat();
            pussy.MyName = "Murlon1";
            pussy.MyBreed = "Supernatural1";
            Dog doggy1 = new Dog();
            doggy.MyName = "Oscar1";
            doggy.MyBreed = "Blonde1";
            //Console.WriteLine(pussy.MyBreed);
            //pussy.Introduce();            
            PetShop ps = new PetShop();
            Exception ex = new Exception();
            ps.AddPet(pussy);
            ps.AddPet(doggy);
            ps.AddPet(pussy1);
            ps.AddPet(doggy1);
            ps.AddPet(ex);
            try
            {
                ps.IntroduceAll();
            }
            catch (NotFoundException)
            {
                Console.WriteLine("Exeption");
            }
            Console.ReadKey();
        }
    }

NotFoundException

    class NotFoundException : Exception
    {
        /* Implement all of the Exception constructors. Notice that 
        the constructors simply execute the base class constructor. 
        Because NotFoundException adds nothing to Exception, 
        there is no need for any further actions. */
        public NotFoundException() : base() { }
        public NotFoundException(string message) : base(message) { }
        public NotFoundException(string message, Exception innerException) :
            base(message, innerException) { }
        protected NotFoundException(
        System.Runtime.Serialization.SerializationInfo info,
        System.Runtime.Serialization.StreamingContext context) :
            base(info, context) { }
    }
}

I scored 7.8 out of 10. Test was auditing remotely and I haven't had an opportunity to get explanations about the mark.

What could I have done to improve on my answer to get a better rating?

\$\endgroup\$

migrated from stackoverflow.com Jul 14 '15 at 19:43

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ I would have used a List<IPet> instead of List<object>. And AddPet would have taken an IPet instead of object. That would have greatly simplified your IntroduceAll method. \$\endgroup\$ – juharr Jun 30 '15 at 19:13
  • 9
    \$\begingroup\$ I think that 7.8 is rather high for code that uses == typeof() for polymorphism. \$\endgroup\$ – Henk Holterman Jun 30 '15 at 19:15
  • 1
    \$\begingroup\$ Notice any repetitive code in your cat & dog? \$\endgroup\$ – Mikko Viitala Jun 30 '15 at 19:16
  • 2
    \$\begingroup\$ I'm guessing "implement exception handling" meant to check for null being passed to AddPet not creating a custom exception type to use. \$\endgroup\$ – juharr Jun 30 '15 at 19:21
14
\$\begingroup\$

One mistake could be using List<Object> in your class PetShop

List<Object> objList = new List<object>(); 

instead you could have a List<IPet> and then your method AddPet should expect a parameter of type IPet , add Cat and Dog objects to that list. Later in your method IntroduceAll you could have done:

public void IntroduceAll()
{
    foreach (IPet element in objList)
    {
      element.Introduce();
    }
}

instead of comparing types and then calling Introduce method, this would show the polymorphic behaviour.

Other thing could be having a parameterized constructor, since you are instantiating the objects for Cat and Dog and then setting properties.

\$\endgroup\$
6
\$\begingroup\$

I think the biggest gap is that you are not using polymorphism at all. You are defining an interface IPet and deriving your animals from it. But then, you use type checks.

You can verify this by removing IPet. Your code will run anyway.

You should always use IPet in your later code like your data list and the loop.

\$\endgroup\$
  • \$\begingroup\$ Yeah, you are right \$\endgroup\$ – AndriiGro Jul 2 '15 at 18:50
6
\$\begingroup\$

Tweaking a little further based on comments from @Henk and @Mikko mentioned is that you are using TypeOf and also copied and pasted the body of the Cat and Dog classes when you could use an abstract class and accomplish the same goal once:

abstract class PetStoreAnimal : IPet
{
    string name;
    string breed;
    public string MyName { get {return name; } set {name = value;} }

    public string MyBreed { get {return breed; } set {breed = value;} }

    public void Introduce()
    {
        Console.WriteLine(String.Format("I am {0} of {1} and I am a {2}", MyName, MyBreed, this.GetType().Name));
    }
}

This reduces the repetition, and then Cat and Dog effectively become:

 class Cat : PetStoreAnimal
{
    public Cat()
    {
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! Nice first post! I would add that the vertical whitespace could be reduced to enhance readability here (getter/setter would each be 1 line), and private fields could be readonly and camelCase, with the property name matching the backing field - actually, these properties are so trivial they could be implemented as auto-properties. \$\endgroup\$ – Mathieu Guindon Jul 15 '15 at 2:51
  • 1
    \$\begingroup\$ You can omit the empty public constructor - those are generated by default. \$\endgroup\$ – Dan Lyons Jul 15 '15 at 17:46
  • \$\begingroup\$ @DanLyons you're right, it's unnecessary, I do tend to be explicit with these things. When someone is learning, things that happen without them doing it adds confusion, so I left it for that reason. Thanks for pointing it out though, it's important :) \$\endgroup\$ – jaeckyl Nov 1 '15 at 14:02
5
\$\begingroup\$

TL;DR You should be very happy with a score of 7.8

Now, to be honest I am a tough marker :) So, if I was marking this I would mark you down for each of these issues:

namespace ConsoleApplication1 - give it a meaningful namespace instead.

interface IPet, class Dog : IPet etc - always use an access modifier, eg public interface IPet, public class Dog

public string MyName { ... } - properties should use automatic getters and setters, eg public string MyName { get; set; }

public string MyName { ... } - the requirements specifically said that the properties must be Name and Breed, you have MyName and MyBreed.

public Cat() - you should include name and breed in the constructor so that it can be passed in when the variable is created.

Not using the obvious superclass / subclass relationship:

public abstract class Animal : IPet 
{
    public string Name { get; set; }
    public string Breed { get; }
    public string Introduce()
    {
        Console.WriteLine(String.Format("I am {0} of {1}. I'm a {2}", 
                          Name, Breed, this.GetType().Name));
    }
}

public class Cat : Animal
{
    public Cat(string name, string breed) { Name = name; Breed = breed; }
}   // repeat this for Dog

And also for:

List<Object> objList = new List<object>(); should be List<IPet> pets = new List<IPet>(); - use the actual interface you have, and name the variable properly.

public void IntroduceAll() should be implemented very simply now that you have an actual list of IPet to work with: foreach (var pet in pets) { pet.Introduce(); }

As for the requirement to use exceptions, this means you need to handle an exception. Not create a new exception variable and try to add it to your pets list. Something like:

try
{
    petshop.Add(new Cat());
    petshop.Add(new Dog());
    petshop.Add(new Animal()); // change petshop so it throws an exception here
}
catch (Exception ex)
{
    // handle the exception here
}

Your Introduce method does not output the exact string that was requested: "I'm (Name) of (Breed). I'm a cat (or dog)". Yours says "I'm (Name) of (Breed) I'm a cat (or dog)" instead. You missed the full stop - I know this is picky, but details matter.

And some more points off for naming:

  • PetShop ps = new PetShop(); - use the whole name, var petshop = new PetShop(); to be clear.
  • Cat pussy = new Cat(); - are you trying to be cute here? Just use var cat = new Cat()

So, you wouldn't be getting a very high score sorry. You haven't actually implemented any polymorphism, you haven't handled exceptions as requested, and your properties do not match the names required. Attention to detail matters :)

\$\endgroup\$
  • \$\begingroup\$ Why should he always use an access modifier? And Breed has no setter, your code does not seem to compile. \$\endgroup\$ – ANeves Jul 15 '15 at 9:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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