0
\$\begingroup\$

I am creating an application just for practicing c# as well as new programming ways. So i came across the idea that i create an application that stores information about "soldiers" ex. Names, ranks, titles, etc... but the trick here is that i want the soldier title to be automatically upgraded when it's rank increases.

I have created a small application to demonstrate the idea.

Soldier Class

public class Soldier
{
    public string Name { get; private set; }
    public int Rank { get; private set; }
    public string Title { get; private set; }

    public Soldier(string name)
    {
        Name = name;
        RefreshTitle();
    }

    public void Rankup(int rank = -1)
    {
        if (-1 != rank)
            Rank = rank;
        else
            Rank++;

        RefreshTitle();
    }

    public void RefreshTitle()
    {
        Title = SoldierRankLibrary.GetSoldierTitle(Rank);
    }
}

Soldier Rank Library

public class SoldierRankLibrary
{
    private static readonly Dictionary<int, string> RankDictionary = new Dictionary<int, string>
                                                          {
                                                              {0, "Novice"},
                                                              {1, "Second Lieutenant"},
                                                              {2, "First Lieutenant"},
                                                              {3, "Captain"},
                                                              {4, "Major"},
                                                              {5, "Lieutenant Colonel"},
                                                              {6, "Colonel"}
                                                              //etc...
                                                          };

    public static string GetSoldierTitle(int requiredRank)
    {
        return RankDictionary.FirstOrDefault(rank => rank.Key == requiredRank).Value;
    }
}

Using the code

static void Main()
{
    //Create new soldier...
    var soldier = new Soldier("Daniel");

    //Print soldier info...
    PrintSoldierInfo(soldier);

    //Upgrade soldier...
    soldier.Rankup();

    //Print soldier info...
    PrintSoldierInfo(soldier);

    Console.Read();
}

static void PrintSoldierInfo(Soldier soldier)
{
    Console.WriteLine("****************************************");
    Console.WriteLine("Name: {0}", soldier.Name);
    Console.Write("Rank: ");
    switch (soldier.Rank)
    {
        case 0:
            Console.WriteLine("novice");
            break;
        case 1:
            Console.WriteLine("*");
            break;
        case 2:
            Console.WriteLine("**");
            break;
        case 3:
            Console.WriteLine("***");
            break;
        case 4:
            Console.WriteLine("****");
            break;
        case 5:
            Console.WriteLine("*****");
            break;
        case 6:
            Console.WriteLine("******");
            break;
        case 7:
            Console.WriteLine("*******");
            break;
    }
    Console.WriteLine("Title: {0}", soldier.Title);
    Console.WriteLine("****************************************");
    Console.WriteLine();
}

Conclusion

I am really waiting for your answers and i hope you give me detailed information about what i miss, what i can do better, how could i improve this code.

\$\endgroup\$
  • \$\begingroup\$ Based upon the code provided, you are avoiding the benefits of polymorphism by using enums and switches and string literals. According to Fowler (and his disciples) these can be classified as 'code smells'. Consider polymorphism and OO design. \$\endgroup\$ – Gayot Fow Jul 15 '13 at 17:46
  • \$\begingroup\$ While technically not relevant, novice is not a military rank :-) Pick your country from the list: Wikipedia: List of comparative military ranks \$\endgroup\$ – Lstor Jul 16 '13 at 12:41
  • \$\begingroup\$ @Lstor : hehehe i know i was just creating a question :D \$\endgroup\$ – RuneS Jul 16 '13 at 22:25
3
\$\begingroup\$

Get rid of SoldierRankLibrary

  • GetSoldierRank() belongs in the Soldier class. That's what Rank { get; set; } is for.
  • the Dictionary will be replaced by an enum (see below). An enum does not need be inside of any class. You can use boolean operators on enums ("<", "==", ...)

Use Enum instead of Dictionary

  • Your dictionary is mimicking an Enum. In other words and Enum inherently associates a number with a name. And in general using Enum is preferred over strings.

  • Make separate OfficerRank and EnlistedRank enums if that works better.

  • I like @Nik's answer with "unknown". But I'd make it "0" instead of "-1". The natural default value of an enum variable is zero.

  • If the underlying int values matter elsewhere in code just make sure you put the ranks in the desired order.

Soldier Class

  • Rank and Title appear to be the same thing. Do you really need to refer to the soldier's rank by its underlying (integer) value?

  • Explicitly set variables in the constructor. We all know default values for .NET types, but you want to convey intent and meaning.

  • Exhaustively set the entire state in the constructor. Again, intent and meaning.

  • Rankup() - change name to Promote(). Don't pass in an integer, pass in something that has meaning. We're going to utilize Rank enum so this fills the bill.

  • RefreshTitle() - obsolete. Rank/Title is the same thing. The Rank enum keeps us from having to deal with a string and an int.

  • Property mis-use. It makes no sense to have a private Title set and a public RefreshTitle(). Think of a Property as a method. A lot of existing code goes away simply by using the public get properties.

  • Override ToString(). Now PrintSoldierInfo() disappears.

New Code

public enum Rank {undefined, BuckPrivate, Sergeant, Lieutenant, Captain}

public class Soldier {
    public string   Name { get; protected set; }
    public Rank     Rank { get; protected set; }

    public Soldier (string name) {
        Name = name ?? "No Name";       // null coalescing.
        Rank = Rank.undefined;   // maybe private if enlisted or lieutenant if officer 
    }

    public void Promote (Rank newRank) {
        if ((int)this.Rank < (int)newRank ) this.Rank = newRank;
        // we don't allow Demotion in this method. 
    }

   public override string ToString() {
       Stringbuilder me = new Stringbuilder();  // need to import System.Text to use Stringbuilder

// C# string formatting avoids clumsy string concatination; clearer and less error prone
       me.AppendLine("Name: {0}  Rank: {1}", Name,Rank); 

       return me.ToString();  // Stringbuilder must be converted to a string.
   }
}

Using Soldier

Soldier grunt = new Soldier("Sad Sack");
grunt.Promote(Rank.Sergeant);
Console.WriteLine (grunt); // ToString() is automagically called.
\$\endgroup\$
  • \$\begingroup\$ This is very nice... There are some errors though so i edited your answer please accept the edit \$\endgroup\$ – RuneS Jul 15 '13 at 18:21
  • \$\begingroup\$ 1. Dictionary<int, string> instead of an enum can make sense if the strings should be human readable, you can't have "Second Lieutenant" in an enum. 2. You can't use private as an enum value like that, because it's a keyword. You should use the normal .Net naming conventions and write Private. Or you could use @private. \$\endgroup\$ – svick Jul 16 '13 at 11:55
  • \$\begingroup\$ The issues that enum solves and/or prevents here trumps the need to extract proper grammar from enum values. Proper grammar is a secondary issue. Then indeed a Dictionary<Rank, string> (vice Dictionary<int, string>) would do well for extracting Second Lieutenant (2 words) from Rank.SecondLieutenant. But for Heaven's sake get away from obtuse use of integers and express things in terms of the problem domain. \$\endgroup\$ – radarbob Jul 16 '13 at 12:25
1
\$\begingroup\$

You can change Rank type to some enum.

public enum Rank
{
    Unknown = -1,
    Novice = 0,
    SecondLieutenant = 1,
    ....
}

public static class SoldierRankLibrary
{
    private static readonly Dictionary<int, string> RankDictionary = new Dictionary<Rank, string>
                                                      {
                                                          {Rank.Novice, "Novice"},
                                                          ....
                                                      };

     public static string DisplayName(this Rank rank)
     {
         if (RankDictionary.ContainsKey(rank))
         {
             return RankDictionary[rank];
         }
         return rank.ToString();
     }
}

public class Soldier
{
     public string Name { get; private set; }
     public Rank Rank { get; private set; }
     public string Title { get { return Rank.DisplayName(); } }

     public Soldier(string name, Rank rank = Rank.Unknown)
     {
         Name = name;
         Rank = rank;
     }

     public void Rankup()
     {
         Rank++;
     }
}
\$\endgroup\$
  • \$\begingroup\$ Very nice well detailed explanation thank you very much Nik \$\endgroup\$ – RuneS Jul 15 '13 at 18:21

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.