Up, up and away in a Retro Rocket

Question

I have seen the Writings on the wall...

1. Retro Rocket ASCII Art
2. Nested for-loop art
3. Print the Retro Rocket
4. Maybe a golfed C# answer from me

But I took this relatively serious and created a VerticalRocket class that I would like reviewed, kind of like a little "am I getting better at writing code" project/review experience (granted this was done while waiting for code to compile and queries to run).

What can I learn from this experience?

class VerticalRocket
{
    private int _FuselageSections = 2;
    public int FuselageSections
    {
        get
        {
            return _FuselageSections;
        }
        set
        {
            if (value <= 100 && value > 0)
            {
                _FuselageSections = value;
            }
        }
    }

    public List<string> rocketPartsList = new List<string>();

    private string Divider = "+=*=*=*=*=*=*+";

    private List<string> upDesigns = new List<string>
    {
          @"|../\..../\..|"
        , @"|./\/\../\/\.|"
        , @"|/\/\/\/\/\/\|"
    };

    private List<string> downDesigns = new List<string>
    {
          @"|\/\/\/\/\/\/|"
        , @"|.\/\/..\/\/.|"
        , @"|..\/....\/..|"
    };
    private List<string> NoseParts = new List<string>
    {
          @"      /\"
        , @"     /**\"
        , @"    //**\\"
        , @"   ///**\\\"
        , @"  ////**\\\\"
        , @" /////**\\\\\"
    };

    private List<string> BoosterParts = new List<string>
    {
          @"    //**\\"
        , @"   ///**\\\"
        , @"  ////**\\\\"
        , @" /////**\\\\\"
    };
    /// <summary>
    /// Creates a Generic Rocket with 2 Fuselage Sections.
    /// </summary>
    public VerticalRocket()
    {
        rocket(FuselageSections);
    }
    /// <summary>
    /// creates a rocket with specified number of Fuselage sections
    /// </summary>
    /// <param name="sections">Number of Fuselage Sections Desired</param>
    public VerticalRocket(int sections)
    {
        FuselageSections = sections;
        rocket(FuselageSections);
    }

    public List<string> Fuselage(bool diamondShape)
    {
        List<string> fuselage = new List<string>();

        if (diamondShape)
        {
            foreach (string line in upDesigns)
            {
                fuselage.Add(line);
            }
            foreach (string line in downDesigns)
            {
                fuselage.Add(line);
            }
        }
        else
        {
            foreach (string line in downDesigns)
            {
                fuselage.Add(line);
            }
            foreach (string line in upDesigns)
            {
                fuselage.Add(line);
            }
        }
        return fuselage;
    }

    public List<string> Nose()
    {
        List<string> nose = new List<string>();
        foreach (string line in NoseParts)
        {
            nose.Add(line);
        }
        return nose;
    }

    public List<string> Booster()
    {
        List<string> booster = new List<string>();
        foreach (string line in BoosterParts)
        {
            booster.Add(line);
        }
        return booster;
    }

    public void rocket(int sections)
    {
        bool wantDiamond = sections % 2 == 0 ? true : false; //this will keep the X at the bottom

        rocketPartsList.AddRange(Nose());
        rocketPartsList.Add(Divider);
        for (int i = 0; i < sections; i++)
        {
            rocketPartsList.AddRange(Fuselage(wantDiamond));
            wantDiamond = !wantDiamond;
            rocketPartsList.Add(Divider);
        }
        rocketPartsList.AddRange(Booster());
    }

    public override string ToString()
    {
        string output = "";
        foreach (string line in rocketPartsList)
        {
            output += line + Environment.NewLine;
        }
        return output;
    }

}

Note

Up-To-Date code can be found on my Github Repo

Answer 1

Naming

Methodnames should be made out of verbs or verb phrasese, so Nose(), Booster and so on, arent't good names for methods. They would make good names for readonly properties.

Fieldnames should be named using camelCasing casing.
Methodnames should be named using PascalCasing.

Scope

All your methods are public which isn't necessary at all. The only method which needs to be public is the overridden ToString.

Example code to blow up your code

VerticalRocket rocket = new VerticalRocket(2);
rocket.rocketPartsList = null;
Console.WriteLine(rocket.ToString());  

This shouldn't be possible ;-)

So watch the scope.

Flow

As you have the FuselageSections property, this is the only part of your code, which can change the output of the ToString() method. Therefor I would expect that inside the ToString() method the composition of the rocket would take place. I wouldn't call a method rocket() to actually build the rocket after changing the property.

Also, if I really would call the rocket() method after changing the property, a really strange rocket would be built, as you don't clear the rocketPartsList field.

So I see 3 possibilities

1. Rebuild the rocket if the property is changed (bad, as properties should return as fast as possible)
2. Rebuild the rocket inside the ToString() method
3. rename the rocket() method to something more meaningful like Rebuild(), leave it public and skip the property.

I would go with number 3, but a little bit different. I would rename rocket to Build(int) and define it as private and then introduce a new method public Rebuild(int) which is only calling Build().

Refactoring

As the methods Nose(), Booster() shouldn't be accessible from the outside, changing the scope to private should be done. But if they are private we can just skip them, as they only return a new List<String> representing the private fields.

These List<String> fields are a little over the head also. You don't need the List's as you don't use any of its methods. A String array would be a better fit.

The divider field should be a const.

You should use constructor chaining as well.

public VerticalRocket()
{
    rocket(FuselageSections);
}  

should be

public VerticalRocket():this(FuselageSections)
{}

After implementing this and also the points of ferada's answer we would get the following

class VerticalRocket
{
    private const int initialFuselageSections = 2;
    private const string divider = "+=*=*=*=*=*=*+";

    private List<string> rocketPartsList = new List<string>();

    private String[] upDesigns = 
    {
      @"|../\..../\..|"
    , @"|./\/\../\/\.|"
    , @"|/\/\/\/\/\/\|"
    };

    private String[] downDesigns = 
    {
      @"|\/\/\/\/\/\/|"
    , @"|.\/\/..\/\/.|"
    , @"|..\/....\/..|"
    };

    private String[] noseParts = 
    {
      @"      /\"
    , @"     /**\"
    , @"    //**\\"
    , @"   ///**\\\"
    , @"  ////**\\\\"
    , @" /////**\\\\\"
    };

    private String[] boosterParts = 
    {
      @"    //**\\"
    , @"   ///**\\\"
    , @"  ////**\\\\"
    , @" /////**\\\\\"
    };

    public VerticalRocket()
        : this(initialFuselageSections)
    { }

    public VerticalRocket(int sections)
    {
        Build(sections);
    }

    public List<string> GetFuselage(bool diamondShape)
    {
        List<string> fuselage = new List<string>();

        if (diamondShape)
        {
            fuselage.AddRange(upDesigns);
            fuselage.AddRange(downDesigns);
        }
        else
        {
            fuselage.AddRange(downDesigns);
            fuselage.AddRange(upDesigns);
        }
        return fuselage;
    }

    public void ReBuild(int sections)
    {
        Build(sections);
    }

    private void Build(int sections)
    {
        if (sections > 100 && sections <= 0)
        {
            throw new ArgumentOutOfRangeException("sections");
        }

        bool wantDiamond = sections % 2 == 0;

        rocketPartsList.Clear();
        rocketPartsList.AddRange(noseParts);
        rocketPartsList.Add(divider);

        for (int i = 0; i < sections; i++)
        {
            rocketPartsList.AddRange(GetFuselage(wantDiamond));
            wantDiamond = !wantDiamond;
            rocketPartsList.Add(divider);
        }

        rocketPartsList.AddRange(boosterParts);
    }

    public override string ToString()
    {
        return String.Join(Environment.NewLine, rocketPartsList);
    }

} 

You could also build the nose of the rocket by using only the first 2 items of the noseParts and add the boosterParts.

Answer 2

• I don't like that the FuselageSections silently discards out of bounds values, throwing an exception would be better.

• Maybe there are shorter ways, but Fuselage can use AddRange as well:

  if (diamondShape)
  {
      fuselage.AddRange(upDesigns);
      fuselage.AddRange(downDesigns);
  }
  else
  {
      fuselage.AddRange(downDesigns);
      fuselage.AddRange(upDesigns);
  }
  

• wantDiamond can just be

  bool wantDiamond = sections % 2 == 0;
  

  and that (foo ? true : false or the reverse) is something I'll always fix if I see it somewhere.

• ToString can just be

  return String.Join(Environment.NewLine, rocketPartsList);
  

  instead.

• Nose, Booster can all just create a copy with the List constructor, i.e.

  return new List<string>(BoosterParts);
  

  and so on.

Otherwise looks fine; if it was actually part of a larger program I'd move the parts itself into classes and have them output (ToString) themselves instead.