Program structure to be checked according to OOP

Question

I am building my first application based on the OOP model. The application basically responsible for sending XML Soap requests. Model contains couple classes - Server.cs which contain servers IPs, Query.cs for XML soap texts, Message.cs which will contain list of queries put to specific message to be send, and Sender.cs to take Messages to send requests.

Please check whether my OOP way of preparing that application is good enough and what do you think how I related classes and dealing with object - as well as generic class used in this project.

Is this a good way of programming? Note that this is not finished yet I want to take your suggestion under consideration if I am using OOP well enough.

class Program
{
 static void Main(string[] args)
 {
 //Queries belongs to message
 var Query1 = new Query() { ID = 1, Message = "First XML query"};
 var Query2 = new Query() { ID = 2, Message = "Second XML query" };
 //Message contains queries
 Message msg1 = new Message();
 Utilities<Query>.AddToList(Query1);
 Utilities<Query>.AddToList(Query2);
 Utilities<Query>.RevertList();
 Utilities<Query>.RemoveFromList(Query1);
 //Message contains servier ip
 msg1.Server.ServerIP = "192.168.0.1";
//all below is just for testing - here will be Sender execute method realased instead of below:
 foreach (Query item in Utilities<Query>.GetExistingList())
 {
 Console.WriteLine("Query ID : " + item.ID.ToString());
 Console.WriteLine("Query Message : " + item.Message.ToString());
 Console.WriteLine("Server IP : " + msg1.Server.ServerIP.ToString());
 }
 Console.ReadLine();
 }
}
public class Sender
{
 private Message Message;
 private List<Message> _messageList;
 public void GetCompleteSendInfromation()
 {
 Console.WriteLine("Server IP : " + Message.Server.ServerIP);
 Console.ReadLine();
 }
 public void ExecuteSOAP()
 {
 // TODO: Execite SOAP requests loops based on messages which contains xml soap queries
 }
}
public class Message
{
 public Server _server;
 public Server Server
 {
 get { return _server; }
 set { _server = value; }
 }
 public Message()
 {
 Utilities<Query>.CreateNewList();
 Server = new Server(this);
 }
 public void GetServerIP()
 {
 Console.WriteLine(_server.ServerIP);
 Console.ReadLine();
 }
 public override string ToString()
 {
 return "Message contains: " + Utilities<Query>.List.Count.ToString()
 + " queries and 1 Server IP address";
 }
}
public class Query
{
 private int _id;
 public int ID
 {
 get { return _id; }
 set { _id = value; }
 }
 private string _message;
 public string Message
 {
 get { return _message; }
 set { _message = value; }
 }
 //TODO text xml query will be taken from files
public string GetXMLmessage()
{
 return "xmlstring";
}
}
public class Server
{
 private string _serverIP;
 private Message message;
 public string ServerIP
 {
 get { return _serverIP; }
 set { _serverIP = value; }
 }
 public Server(Message obj)
 {
 message = obj;
 }
}
public class Utilities<T>
{
 private static List<T> _list;
 public static List<T> List { get; set; }
 public static void CreateNewList()
 {
 _list = new List<T>();
 }
 public static List<T> GetExistingList()
 {
 return _list;
 }
 public static void AddToList(T value)
 {
 _list.Add(value);
 }
 public static void RemoveFromList(T value)
 {
 _list.Remove(value);
 }
 public static void RevertList()
 {
 _list.Reverse();
 }
}

I had got also other version but I wanted to have static List helper class Utilities, which is why I dropped this below one. Is it better OOP than the above version?

class Program
{
 static void Main(string[] args)
 {
 //Queries belongs to message
 var Query1 = new Query() { ID = 1, Message = "First XML query"};
 var Query2 = new Query() { ID = 2, Message = "Second XML query" };
 //Message contains queries
 Message msg1 = new Message();
 msg1.AddQueryToList(Query1);
 msg1.AddQueryToList(Query2);
 //Message contains servier ip
 msg1.Server.ServerIP = "192.168.0.1";
 foreach (Query item in msg1.QueryList)
 {
 Console.WriteLine("Query ID : " + item.ID.ToString());
 Console.WriteLine("Query Message : " + item.Message.ToString());
 Console.WriteLine("Server IP : " + msg1.Server.ServerIP.ToString());
 }
 Console.ReadLine();
 }
}
public class Sender
{
 private Message Message;
 private List<Message> _messageList;
 public void GetCompleteSendInfromation()
 {
 Console.WriteLine("Server IP : " + Message.Server.ServerIP);
 Console.ReadLine();
 }
 public void ExecuteSOAP()
 {
 // TODO: Execite SOAP request based on message contains xml soap queries
 }
}
public class Message
{
 private List<Query> _queryList;
 public List<Query> QueryList
 {
 get { return _queryList; }
 private set { _queryList = value; }
 }
 public Server _server;
 public Server Server
 {
 get { return _server; }
 set { _server = value; }
 }
 public Message()
 {
 _queryList = new List<Query>();
 Server = new Server(this);
 }
 public void AddQueryToList(Query query)
 {
 _queryList.Add(query);
 }
 public void GetServerIP()
 {
 Console.WriteLine(_server.ServerIP);
 Console.ReadLine();
 }
 public override string ToString()
 {
 return "Message contains: " + _queryList.Count.ToString()
 + " queries and 1 Server IP address";
 }
}
public class Query
{
 private int _id;
 public int ID
 {
 get { return _id; }
 set { _id = value; }
 }
 private string _message;
 public string Message
 {
 get { return _message; }
 set { _message = value; }
 }
 public string GetXMLmessage()
 {
 return "xmlstring";
 }
}
public class Server
{
 private string _serverIP;
 private Message message;
 public string ServerIP
 {
 get { return _serverIP; }
 set { _serverIP = value; }
 }
 public Server(Message obj)
 {
 message = obj;
 }
}

• Paritosh: I looked into second version, if server is part of the message why message instance is passed into server instance? I dont see any usage over there
• user3811374: I need to create new Server instance to be apple to create object (IP) for message otherwise this will not work: msg1.Server.ServerIP = "192.168.0.1"
• Paritosh: if you have to assign the serverIp , pass that in constructor of message and assign there using server instance
• user3811374: maybe i dont get it i already passing: public Message() { _queryList = new List<Query>(); Server = new Server(this); }
• Paritosh: added the answer with creating instance of server inside the message and it's properties

Answer 1

Utilities<Query>.AddToList(Query1);
Utilities<Query>.AddToList(Query2);
Utilities<Query>.RevertList();
Utilities<Query>.RemoveFromList(Query1);

Please, don't do this. Why have a global list of queries that can be accessed from anywhere? This is what Wikipedia has to say about global variables (your list technically isn't a global variable, but it behaves exactly like one):

They are usually considered bad practice precisely because of their non-locality: a global variable can potentially be modified from anywhere, and any part of the program may depend on it. A global variable therefore has an unlimited potential for creating mutual dependencies, and adding mutual dependencies increases complexity.

Instead, just use a local List<T> in your method and pass it as a parameter or store it in a field of your object.

I had got also other version but I wanted to have static List helper class Utilities, which is why I dropped this below one. Is it better OOP than the above version?

Yes, this code is definitely better. Why would you want to have the list static like this? That's pretty much asking for bugs.

---

public Server _server;
public Server Server
{
 get { return _server; }
 set { _server = value; }
}

You don't need to be this verbose, just use an auto-property:

public Server Server { get; set; }

---

"Message contains: " + Utilities<Query>.List.Count.ToString()
 + " queries and 1 Server IP address"

You don't need that ToString() there, that's called automatically. Also I think it's better to use string.Format() in cases like this (it makes especially dealing with punctuation and spaces much clearer):

string.Format(
 "Message contains: {0} queries and 1 Server IP address", Utilities<Query>.List.Count);

• user3811374: ok ill keep first version without static feature. One remark i meant here: private Server _server; public Server Server { get { return _server; } set { _server = value; } } to encapsulate. Why would you prefer only propertie without private field for it because i want to keep privte field and propertie which is accessor for it.
• svick: @user3811374 There is no advantage in having visible private field for a trivial property. It doesn't improve encapsulation or anything like that.

Answer 2

Server is a part of Message so server initialization is part of message itself.

so updated solution:

public class Message
{
 private Server _server;
 public Message(string ipAddress)
 {
 _server = new Server(ipAddress);
 }
 public void GetServerIp()
 {
 Console.WriteLine(_server.ServerIP);
 Console.ReadLine();
 }
 public override string ToString()
 {
 return "Message contains: " + Utilities<Query>.List.Count.ToString()
 + " queries and 1 Server IP address";
 }
}
public class Server
{
 private readonly string _serverIp;
 public string ServerIp
 {
 get { return _serverIp; }
 }
 public Server(string ipAddress)
 {
 _serverIp = ipAddress;
 }
}

so by doing so u will restrict access to server properties directly.

Answer 3

so complete code would be like below what do you think right now? little summarize:

Sender.cs class can have many messages thats why it contains : List

Message.cs class can have many Queries thats why List and contain one Server

Question in this case: is it correct implemented and also for rest classes i am asking especially regarding correlation between classes?

Please check also class diagram if its built correctly (i mean arrows here) because this is my first class diagram like this) : find it here: https://www.dropbox.com/s/lydp8yi8ma2jj65/1.png

And the complete code:

 class Program
{
 static void Main(string[] args)
 {
 //--Queries belongs to message--
 var Query1 = new Query() { ID = 1, Message = "First XML query"};
 var Query2 = new Query() { ID = 2, Message = "Second XML query" };
 //--Message contains queries--
 Message msg1 = new Message();
 msg1.AddQueryToList(Query1);
 msg1.AddQueryToList(Query2);
 Server ser1 = new Server("192.168.0.1");
 //--Assign Server to Message--
 msg1.Server = ser1;
 foreach (Query item in msg1.QueryList)
 {
 Console.WriteLine("Query ID : " + item.ID.ToString());
 Console.WriteLine("Query Message : " + item.Message.ToString());
 Console.WriteLine("Server IP : " + msg1.Server.ServerIP.ToString());
 }
 Console.ReadLine();
 }
}
 public class Sender
{
 //--Sender could contain Message object or many
 private List<Message> _messageList;
 private List<Message> MessageList
 {
 get { return _messageList; }
 set { _messageList = value; }
 }
 public void GetCompleteSendInfromation()
 {
 //Console.WriteLine("Server IP : " + Message.Server.ServerIP);
 Console.ReadLine();
 }
 public void ExecuteSOAP()
 {
 // TODO: Execite SOAP request based on message contains xml soap queries
 }
}
public class Message
{
 private List<Query> _queryList;
 public List<Query> QueryList
 {
 get { return _queryList; }
 private set { _queryList = value; }
 }
 //--Server is part of a Message--
 public Server _server;
 public Server Server
 {
 get { return _server; }
 set { _server = value; }
 }
 public Message()
 {
 _queryList = new List<Query>();
 }
 public void AddQueryToList(Query query)
 {
 _queryList.Add(query);
 }
 public void GetServerIP()
 {
 Console.WriteLine(_server.ServerIP);
 Console.ReadLine();
 }
 /// <summary>
 /// Method overiding virtual method Equals() from class System.Object.
 /// For parameter it takes any object (System.Object)
 /// </summary>
 /// <param name="obj"></param>
 /// <returns></returns>
 public override bool Equals(object obj)
 {
 //--check whether objest is not empty--
 if (obj == null) return false;
 //--check whether object can be cast to Contact class
 Message m = obj as Message;
 if ((System.Object)m == null) return false;
 //--check contain of each field--
 return (QueryList == m.QueryList) && (Server == m.Server);
 }
 /// <summary>
 /// Method checks equality of values between objects. Same method as previus but 
 /// taking Message object.
 /// </summary>
 /// <param name="m"></param>
 /// <returns></returns>
 public bool Equals(Message m)
 {
 //--check whether object is not empty--
 if (m == null) return false;
 //check contain of each field
 return (QueryList == m.QueryList) && (Server == m.Server);
 }
 public override int GetHashCode()
 {
 return 1;
 }
 public override string ToString()
 {
 return "Message contains: " + _queryList.Count.ToString()
 + " queries and 1 Server IP address";
 }
}
 public class Query
{
 private int _id;
 public int ID
 {
 get { return _id; }
 set { _id = value; }
 }
 private string _message;
 public string Message
 {
 get { return _message; }
 set { _message = value; }
 }
 public string GetXMLmessage()
 {
 return "xmlstring";
 }
 /// <summary>
 /// Method overiding virtual method Equals() from class System.Object.
 /// For parameter it takes any object (System.Object)
 /// </summary>
 /// <param name="obj"></param>
 /// <returns></returns>
 public override bool Equals(object obj)
 {
 //--check whether objest is not empty--
 if (obj == null) return false;
 //--check whether object can be cast to Contact class
 Query s = obj as Query;
 if ((System.Object)s == null) return false;
 //--check contain of each field--
 return (ID == s.ID) && (ID == s.ID);
 }
 /// <summary>
 /// Method checks equality of values between objects. Same method as previus but 
 /// taking Query object.
 /// </summary>
 /// <param name="m"></param>
 /// <returns></returns>
 public bool Equals(Query q)
 {
 //--check whether object is not empty--
 if (q == null) return false;
 //check contain of each field
 return (ID == q.ID) && (Message == q.Message);
 }
 public override int GetHashCode()
 {
 return 1;
 }
 public override string ToString()
 {
 return "Query ID: " + ID.ToString();
 }
}
 public class Server
{
 private string _serverIP;
 public string ServerIP
 {
 get { return _serverIP; }
 set { _serverIP = value; }
 }
 public Server(string ipAddress)
 {
 this._serverIP = ipAddress; 
 }
 /// <summary>
 /// Method overiding virtual method Equals() from class System.Object.
 /// For parameter it takes any object (System.Object)
 /// </summary>
 /// <param name="obj"></param>
 /// <returns></returns>
 public override bool Equals(object obj)
 {
 //--check whether objest is not empty--
 if (obj == null) return false;
 //--check whether object can be cast to Contact class
 Server s = obj as Server;
 if ((System.Object)s == null) return false;
 //--check contain of each field--
 return (ServerIP == s.ServerIP);
 }
 /// <summary>
 /// Method checks equality of values between objects. Same method as previus but 
 /// taking Server object.
 /// </summary>
 /// <param name="m"></param>
 /// <returns></returns>
 public bool Equals(Server s)
 {
 //--check whether object is not empty--
 if (s == null) return false;
 //check contain of each field
 return (ServerIP == s.ServerIP);
 }
 public override int GetHashCode()
 {
 return 1;
 }
 public override string ToString()
 {
 return "Message IP: " + ServerIP.ToString();
 }
}

• user3811374: what do you think about?
• user3811374: any final feedback?