Random missile generator and air defense system

Question

My task in C#:

1. Random missile generator generating 2 types of missiles each second: ThermoNuclearBomb and FacetBomb

   • Missiles have properties Serial Number, Type

2. Air Defense System, that is identifying the 2 types of missiles and neutralizes each of them with a different gun (AntiThermoGun and AntiFacetGun)

   • Air Defense system has a method AcceptMissile that accepts a Missile as a parameter
   • Every Gun has a method Neutralize that accepts an appropriate Missile as a parameter and neutralizes only such types of missiles.
   • The Air Defense system must decide which missile will be neutralized with which gun.

I've got two interfaces:

public interface IMissile
    {
         int SerialNumber { get; set; }
         string Type { get; set; }

    }

public interface IGun
    {
         string Name { get; set; }
         void Neutrilize(IMissile missile);
    }

Of course FacetBomb and ThermoNuclearBomb class implement this interface IMissile:

public class FacetBomb : IMissile
    {
        public int SerialNumber { get; set; }
        private string type;
        public string Type { get; set; }
    }

    public class ThermoNuclearBomb:IMissile
    {
        public int SerialNumber { get; set; }
        public string Type { get ; set ;  }

    }

Here are my two classes AntiThermoGun and AntiFacetGun that implement IGun:

public class AntiThermoGun  : IGun
    {
        private string name;
        public string Name 
        {   get {return name;}
            set { name = "AntiThermoGun"; } 
        }
        public  void Neutrilize(ThermoNuclear gun)
        {
            Console.WriteLine(this.Name + "neutrilizes" + gun.Type);

        }
    }

   public class AntiFacetGun : IGun
    {
        private string name;
        public string Name
        {
            get { return name; }
            set { name = "AntiFacetGun"; }
        }
        public void Neutrilize(FacetBomb missile)
        {
            Console.WriteLine(this.Name + "neutrilizes" + missile.Type);
        }    
    }

Here is my AirDefence class which accepts a missile and decides which missile will be neutralized with which gun:

public class AirDefenceSystem
    {

        public void InterceptMissile(IMissile missile)
        {
            IGun igun = null;

            switch (missile.Type)
            {
                case "ThermoNuclear":

                    igun = new AntiThermoGun();
                    break;

                case "FacetBomb":
                    igun = new AntiFacetGun();
                    break;
            }
            igun.Neutrilize(missile);
        }
    }

In my program.cs:

 static void Main(string[] args)
        {
            FacetBomb facetBomb = new FacetBomb { SerialNumber = 100, Type = "FacetBomb" };
            FacetBomb facetBomb2 = new FacetBomb { SerialNumber = 200, Type = "FacetBomb" };
            ThermoNuclear thermoBomb = new ThermoNuclear { SerialNumber = 500, Type = "ThermoNuclear" };

            IMissile[] objects = new IMissile[] { facetBomb, facetBomb2, thermoBomb };
            AirDefenceSystem system = new AirDefenceSystem();
            foreach (var item in objects)
            {
                system.InterceptMissile(item);

            }

        }

Answer 1

Consider var

Using the var keyword when the right-hand-side of an assignment makes the type obvious can shorten your code and make changing types much easier.

The missile type is unused.

You use Type, but not type. Remove that.

Use constructors

For your two missile types, the properties don't change after they've been set, so make them getter-only properties and set them in the missile constructors.

public interface IMissile
{
     int SerialNumber { get; }
     string Type { get; }
}

public class FacetBomb : IMissile
{
    public int SerialNumber { get; private set; }
    public string Type { get; private set; }

    public FacetBomb(string type, int serialNumber)
    {
        Type = type;
        SerialNumber = serialNumber;
    }
}

Avoid using strings for type information

This is what types are for, so remove the Type property entirely. When you need to check a type, you can use type checks.

public void InterceptMissile(IMissile missile)
    {
        IGun igun = null;

        if(missile is ThermoNuclearBomb)
        {
           igun = new AntiThermoGun();
        }
        else if(missile is FacetBomb)
        {
           igun = new AntiFacetGun();
        }

        igun.Neutrilize(missile);
    }

It's "Neutralize" or "Neutralise"

Naming

Your interface is IMissile but your concrete types all end in Bomb. You have a variable called igun which belies its type. If you change the type you'd then have to go around changing the name of the variable too. Consider instead a variable name that specifies its purpose, not its type.

What is the serial number for?

It is never used, as far as I can tell.

Use a getter to return a specific value, not a setter

This doesn't make much semantic sense.

    private string name;
    public string Name 
    {   
        get {return name;}
        set { name = "AntiThermoGun"; } 
    }

If I call .Name = "foo" .Name will next return "AntiThermoGun". Instead, stop me from setting .Name at all:

    public string Name 
    {   
        get {return "AntiThermoGun";}
    }

Answer 2

When I first looked at your code I thought it wasn't a bad start for a beginner. But after carefully looking at it there are so many mistakes here and there that I can no longer say so, so excuse me for my ruthlessness.

Most of this mistakes happen with your properties, let's look at an example:

public class AntiFacetGun : IGun
{
    private string name;
    public string Name
    {
        get { return name; }
        set { name = "AntiFacetGun"; }
    }
}

The only reason I can think of you wrote that it's because you don't fully know how properties work. When you create your AntiFacetGun with var gun = new AntiFacetGun(), the gun name would be null. When you finally set the the gun name with gun.Name = "thisName" the name will be AntiFacetGun. The final result will be a sad programmer thinking of why gun.Name is not "thisName".

A better approach would be to have all your properties read-only.

public class AntiFacetGun : IGun
{
    public string Name
    {
        get { return "AntiFacetGun"; }
    }
}

This not only solves the problem described before but also doesn't let the properties of your guns to be changed. This is special relevant for your AirDefenceSystem. If I create a FacetBomb with Type different from FacetBomb your InterceptMissile will throw a NullReferenceException putting the lives of millions at risk.

In your FacetBomb class you also left over the field type there that is not being used anywhere.

Last but not least you don't have way to know if you successfully Intercepted a missile or not. If a new type of Missile appears the NullReferenceException will be thrown as described before. This is because your switch statement doesn't have a default case ignoring all other possible guns.

It would be nice to extract the Gun creation logic into a separate method due to single responsibility pattern.

This would be the resulting code if you would have applied everything I said.

public interface IMissile
{
     int SerialNumber { get; }
     string Type { get; }
}

public interface IGun
{
     string Name { get; }
     void Neutrilize(IMissile missile);
}

public class FacetBomb : IMissile
{
    public FacetBomb(string serialNumber){
        SerialNumber = serialNumber;
    }

    public int SerialNumber { get; private set; }
    public string Type { get { return "FacetBomb"; } }
}

public class ThermoNuclearBomb : IMissile
{
    public FacetBomb(string serialNumber){
        SerialNumber = serialNumber;
    }

    public string Type { get { return "ThermoNuclearBomb"; } }
}

public class AntiThermoGun  : IGun
{
    public string Name { get { return "AntiThermoGun"; } }

    public void Neutrilize(ThermoNuclear gun)
    {
        Console.WriteLine(this.Name + "neutrilizes" + gun.Type);
    }
}

public class AntiFacetGun : IGun
{
    public string Name { get { return "AntiFacetGun"; } }

    public void Neutrilize(FacetBomb missile)
    {
        Console.WriteLine(this.Name + "neutrilizes" + missile.Type);
    }    
}

public class AirDefenceSystem
{
    private IGun GetGun(IMissile missile)
    {
        switch (missile.Type)
        {
            case "ThermoNuclear":
                return new AntiThermoGun();
            case "FacetBomb":
                return new AntiFacetGun();
            default:
                return null;
        }
    }

    public bool InterceptMissile(IMissile missile)
    {
        var gun = GetGun(missile);
        if(gun == null){
            return false;
        }
        igun.Neutrilize(missile);
        return true;
    }
}