Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have multiple forms, each of which would like to send message through the network, using my NetworkCommunicator class's SendMessage method.

The solution I described here works, however it is ugly, and bludgeons some of the main concepts of OOP paradigm. What I'm looking for (and I'm open to any suggestions) is basically to achieve the same goal but in a more elegant, and OOP conform solution. (maybe by events?)

I'm using more than these forms, but for the example these seemed to be enough.

//using statements
namespace examplespace
{
 public delegate void ImprovedSenderDelegate(string message);  

 public class LoginForm :Form
  {
   private  NetworkCommunicator ncom = new NetworkCommunicator();
   public Ncom
   {
    get { return ncom; }
   }

   private void Login()
   {
    MainForm mf = new MainForm();
    mf.Owner = this;
    //other things i do etc.
    this.Hide();
    mf.Show();
   }      
  }//endofLoginForm

 public class MainForm :Form
 {
  public ImprovedSenderDelegate ISD;

  private void Load ()
  {
   (this.Owner as LoginForm).NCom.SubscribeToSendMessage(this);
  }

  private void I_Want_To_send_A_Message(string message)
  {
   ISD(string);
  }
 }//endofMainForm

 public class NetworkCommunicator
  {
   public void SubscribeToSendMessage(object sender)
    {
     if (sender is MainForm)
     {

         (sender as MainForm).ISD += new ImprovedSenderDelegate(SendMessage);
     }
    }

   private void SendMessage(string message) 
    {/*magic*/}
  }/endofNetworkCommunicator
}
share|improve this question
    
Feel more complex than it should have been. Is it a two-way communication, or do forms send their message in one direction only? Also, I believe that the MainForm should temporarily show the LoginForm, and not what you have - a LoginForm permanently shows the MainForm. I would simplify things as much as possible first. –  Leonid Dec 30 '12 at 20:30
    
Forms send their messages to the server only (through one tcp connection) using delegates however each form can receive message from the server. I Think your idea about the MainForm vs Loginform is quite right, i don't know why i didn't do it that way. –  Cyberbird Dec 30 '12 at 21:19
    
Does the communication need to be truly asynchronous? The LoginForm does not need it. It should spin around and wait until the login succeeds. What about the other form? Give me an example of the type of communication that goes on. –  Leonid Dec 30 '12 at 23:17
    
Well, for the LoginForm it's not entirely necessary, however, other forms work in a manner that they need it. What the server does is, it connects to a device via serial port, and sends everything the device writes there, to the client, and writes everything to the device which the client sends using a basic protocol WRITE_DEVICE|DEVICE_ID|MESSAGE. So because of this, at least some of the client's forms have to work async. Because they have to show the message, the device sends while being able to send messages themselves. Also there could be multiple devices, which i handle on tabbed forms. –  Cyberbird Dec 31 '12 at 10:28
    
Is this a freeware/opensource project? –  C Sharper Dec 31 '12 at 11:08

1 Answer 1

up vote 1 down vote accepted

My suggestions:

If the NetworkCommunicator exists only once per application, you should create a singelton. Code sample:

    /// <summary>
    /// Provides a base class for singeltons.
    /// </summary>
    /// <typeparam name="T">The class that act as a singelton.</typeparam>
    public abstract class Singleton<T>
        where T : class
    {
        protected Singleton()
        {
        }

        /// <summary>
        /// Gets the singelton instance.
        /// </summary>
        public static T Instance
        {
            get { return Nested._instance; }
        }

        private sealed class Nested
        {
            /// <summary>
            /// Creates the nested instance class.
            /// </summary>
            /// 
            /// <remarks>
            /// Explicit static constructor to tell the compiler
            //  not to mark type as beforefieldinit.
            /// </remarks>
            static Nested()
            {
                ConstructorInfo constructor = typeof (T).GetConstructor(Type.EmptyTypes) ??
                                              typeof(T).GetConstructor(BindingFlags.NonPublic | BindingFlags.Instance, null, Type.EmptyTypes, null);

                if (constructor == null)
                    throw new InvalidOperationException("Class has no private nor public constructor");

                _instance = (T)constructor.Invoke(null);
            }

            internal static readonly T _instance;
        }
    }

If someone has any suggestion about my singelton implementation, let me know it..

The delegate makes the code more complex and violates the KISS OOP principle: Keep It Simple and Stupid. Make it as easy as possible first.

share|improve this answer
    
Thank you for the suggestion. I only need one instance per application, so it seems like a good idea. I am not familiar with singletons, so i started my reading here: msdn.microsoft.com/en-us/library/ff650316.aspx it seems like something i could implement. If i understand it correctly, with this pattern, i could directly use that instance, and i wouldn't need a delegate to send a message(i have to make it's sendmessage method public). however, for receiving the message part, I'm not entirely sure.I'll make some changes to the code along with Leonid's suggestion, and update the question. –  Cyberbird Dec 31 '12 at 12:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.