Is this good code practice?

PIEBALDconsult

Hmmm... just about. :thumbsup: I'm not much for needless local variables, so I wouldn't use the fullName in the ToString and the _invokeAction in Main. I also thought you'd still want to do the menu and have the same Dictionary as before, but use the _personManager.DumpAllToConsole rather than the anonymous method. Also usually += is used with events, not "regular" delegates. (And it still looks like your generic parameters got eaten by the forum -- check the Encode "<" (and other HTML) characters when pasting option.)

venomation

Thanks for the reply I left the Person class as it is but changed Program:

  class Program
{
    delegate void DictionaryDel();

    static Dictionary<string, DictionaryDel> \_action;
    static PersonManager \_personManager;

    static void Main()
    {
        
        \_personManager = new PersonManager();

        LoadActions();

       
        \_action\["1"\]();


        Console.Read();

    }

    static void LoadActions()
    {
        \_action = new Dictionary<string, DictionaryDel>();
        \_action.Add("1", \_personManager.DumpAllToConsole);
    }
}

modified on Sunday, May 2, 2010 8:01 AM

PIEBALDconsult · modified on Sunday, May 2, 2010 8:01 AM

:thumbsup: Good call on writing LoadActions -- now can you make a class to encapsulate the menu options? Menu menu = new Menu ( _personManager ) ;

venomation

This is the code that I have created so far, your comments are very helpful!

class Program
{

    static void Main()
    {
        PersonManager personManager = new PersonManager();
        Menu mainMenu = new Menu(personManager);
        mainMenu.LoadActions();
        mainMenu.Display();
       
        Console.Read();

    }
}

   class Person
{
    public Person(string forname, string surname, string title)
    {
        ForName = forname;
        SurName = surname;
        Title = title;
    }

    public string ForName { get; set; }

    public string SurName { get; set; }

    public string Title { get; set; }

    public override string ToString()
    {
        string fullName = string.Format("Name :{0} {1}\\nTitle {2}", ForName, SurName, Title);
        return fullName;
    }

}

class PersonManager
{
    private readonly List \_personList;

    public PersonManager()
    {
        //add a few people as a test
        \_personList = new List(10)
                          {
                              new Person("James", "x", "Prog"),
                              new Person("Claire", "y", "Illu")
                          };

    }

    public void DumpAllToConsole()
    {
        //Prints the details of every person
        foreach (Person p in \_personList)
        {
            Console.WriteLine(p.ToString());
        }
    }
}

class Menu
{
private readonly PersonManager _personManager;
private delegate void DictionaryDel();
private Dictionary _action;

    public Menu(PersonManager personManager)
    {
        \_personManager = personManager;

    }

    void GetInput(string input)
    {
        if (\_action.Count == 0) return;
        if (!\_action.ContainsKey(input)) return;

        \_action\[input\]();
    }



    public void LoadActions()
    {
        \_action = new Dictionary
                      {
                          {"1", \_personManager.DumpAllToConsole}
                      };
    }

    public void Display()
    {
        Console.WriteLine("Press 1 to display all students or 2 t

PIEBALDconsult

:thumbsup: (Especially for the automatic properties (or whatever they're called).) Not much else: Maybe GetInput should be named DoAction instead? I would likely call the LoadActions from the constructor. I would have a while loop in Display. You may also consider creating an interface from the PersonManager so the Menu can be used with other types of manager. Come to think of it, maybe the Manager should be generic -- Manager<Person>.

venomation

Thanks again, I have implemented your advise and I can see that it is a much more manageable way that what it originally was, even though there is allot more work involved it does pay off! :laugh:

class Program
{

    static void Main()
    {
        Manager<Person> personManager = new Manager<Person>();

        //adding two people for a test purpose
        personManager.Add(new Person("James","x","Soft"));
        personManager.Add(new Person("Claire", "y", "Illu"));

        Menu mainMenu = new Menu(personManager);
        mainMenu.Display();
       


    }
}

class Manager<T>
{
private readonly List<T> _managedList;

    public Manager()
    {
        \_managedList = new List<T>(10);
    }

    public void DumpAllToConsole()
    {
        foreach (T p in \_managedList)
        {
            Console.WriteLine(p + "\\n");
            
        }
    }

    public void Add(T input)
    {
        if (input.Equals(null)) return;

        \_managedList.Add(input);

    }

    public void Remove(T item)
    {
       if (!\_managedList.Contains(item)) return;
        \_managedList.Remove(item);
    }

    public List<T> GetCopyOfList()
    {
        List<T> list = new List<T>(\_managedList);
        return list;
    }
 
}

class Menu
{
private readonly Manager<Person> _personManager;
private delegate void DictionaryDel();
private Dictionary<string, DictionaryDel> _action;
private bool _running = true;
public Menu(Manager<Person> personManager)
{
_personManager = personManager;
LoadActions();

    }

    void DoAction(string input)
    {
        if (\_action.Count == 0) return;
        if (!\_action.ContainsKey(input)) return;

        \_action\[input\]();
    }



    void LoadActions()
    {
        \_action = new Dictionary<string, DictionaryDel>
                      {
                          {"1", \_personManager.DumpAllToConsole},
                          {"2",(() => \_running = false)}

    };
    //I personally think using the lambda here is ok...anyone care to tell me otherwise? :P

    }

    public void Dis

PIEBALDconsult

:thumbsup: It's turning out better than I thought. In Add, I would use a simple if ( input == null ) (this isn't Java). Yeah, the lambda seems good there. GetCopyOfList is a good idea, but look into list.AsReadOnly() to see if it might be what you want there. I would still argue for an Interface that the Menu class requires -- that way it can (theoretically) be used with different types of Manager. So far I see no need to make Menu be generic, but you may consider it.

thebuzzwright wrote:

"orthogonality"

That's one of the (few) things I remember from college. :thumbsup:

thebuzzwright wrote:

even though there is allot more work involved it does pay off

It certainly can. It may not in this case, but having experience in how to do this sort of abstraction should pay off when it's really needed later.

venomation

Thanks again "PIEBALDconsult" you have helped me allot ! I don't feel the need to show the code as its just minor changes but: 1 - I added a IMenu interface which can "Display" so that I can derive new menu's for other situations. 2 - Menu inherits IMenu and I replaced " Menu mainMenu = new Menu(personManager);" with " IMenu mainMenu = new Menu(personManager);" so that I can use different menu's. 3 - I decided to switch the List with an IList so that I can use the "AsReadOnly()" method as I don't need to modify the contents. 4 - For the "Add" method: "if (input != null) ManagedList.Add(input);" looked better for me. Final thoughts: I could apply the strategy pattern to IMenu so that it encapsulates its behaviour if needed, but this project was to show manageable code, and I think that would make it more complicated and unnecessary :laugh: Thanks again!

PIEBALDconsult

:thumbsup:

Alaric_

Just out of curiosity - Where do you attend and what level of student are you currently?

"I need build Skynet. Plz send code"

venomation

Alaric_ wrote:

Where do you attend and what level of student are you currently?

I am a college (UK) student on a final year of a BTEC National Diploma in Computing. University this year to study computing hopefully :-D Why the curiousness ? :confused: