2
\$\begingroup\$

I have an IPublisherFactory which publishes instances of, IPublishers. I'm feeding it a bunch of overloaded methods from this gigantic class, that I'm basically using for organization of the implementations.

My concern is that, while it's nice to have the intellisense provided implementations on the client side, maintaining a giant "God" class is sort of obnoxious, but I don't see any alternatives to having a bunch of overload "CreatePublisher" methods. The God class isn't fully implemented, but I did implement one of the UDP and clipboard types to show how I'm planning on continuing along. Basically, if I wanted a new implementations, I'd add an overload in the IPublisherFactory, as well as the PubSub class. Any suggestions would be appreciated. :)

namespace KeyCommunications
{
public static class IPublisherFactory
{
    public static IPublisher CreateIPublisher(PubSub.IPC.OverNetwork.UDP udpParams)
    {
        return udpParams.Publisher;
    }

    public static IPublisher CreateIPublisher(PubSub.IPC.LocalMachine.ClipboardPublisher cb)
    {
        return cb;
    }
}

public class PubSub
{
    public static class IPC 
    {
        public static class OverNetwork
        {
            public class TCP
            {
            }
            public class UDP
            {
                public IPublisher Publisher { get; set; }
                public UDP(DotNetUDPPublisher publisher)
                {
                    Publisher = publisher;
                }
            }
        }

        public static class LocalMachine
        {
            public class ClipboardPublisher : IPublisher
            {
                public ILogger Logger { get; set; }

                public void SendObject<T>(object instanceOfTypeToPublish) where T : new()
                {
                    try
                    {
                        // Getting a default topic to publish to.
                        string defaultTopic = typeof(T).AssemblyQualifiedName;

                        // Call overloaded method with defaultTopic
                        SendObject<T>(instanceOfTypeToPublish, defaultTopic);
                    }
                    catch (Exception e)
                    {
                        Logger.Error(e.Message);
                    }
                }

                public void SendObject<T>(object objectToPublish, List<Type> subTopicTypes) where T : new()
                {
                    try
                    {
                        foreach (var type in subTopicTypes)
                        {
                            // Getting a default topic to publish to.
                            string instanceTopic = typeof(T).AssemblyQualifiedName;

                            // Getting subtopic type  
                            string subtopicType = type.AssemblyQualifiedName;

                            // Appending the default topic, and subtopic.
                            string topicToPublish = instanceTopic + subtopicType;

                            // Call overloaded method with defaultTopic
                            SendObject<T>(objectToPublish, topicToPublish);
                        }
                    }
                    catch (Exception e)
                    {
                        Logger.Error(e.Message);
                    }
                }

                public void SendObject<T>(object objectToPublish, string topicToPublish) where T : new()
                {
                    try
                    {
                        // Creating instance of type inside the method.
                        T type = (T)objectToPublish;

                        // Convert the paramaterless ctor instance as xml string.
                        string objectAsXmlString = type.SerializeObject<T>();

                        // Send xml string over the .Net Socket 
                        SendText(objectAsXmlString);

                        // Log end of call message.
                        Logger.Verbose("End of UDPPublisher SendObject<T> method call");
                    }
                    catch (Exception e)
                    {
                        Logger.Error(e.Message);
                    }
                }

                public void SendText(string textToSend)
                {
                    try
                    {
                        Clipboard.SetDataObject(textToSend);
                    }
                    catch (Exception e)
                    {
                        Logger.Error("Exception message was " + e.Message + ". Stack trace was " + e.StackTrace);
                    }
                }
            }
        }
    }
}
}
\$\endgroup\$
  • \$\begingroup\$ Do you expect these methods to be called with actual object parameters? Otherwise doesn't seem to make sense to make them generic but take object. \$\endgroup\$ – 404 Sep 21 '16 at 18:24
1
\$\begingroup\$
public static IPublisher CreateIPublisher(PubSub.IPC.OverNetwork.UDP udpParams)
{
    return udpParams.Publisher;
}

A factory is for creating new objects which I cannot find in your code. You just return a property of the object you pass to the method.

The naming is also incorrect. It should be called just CreatePublisher without the I for the interface.


public void SendObject<T>(object objectToPublish, string topicToPublish) where T : new()
{
    try
    {
        // Creating instance of type inside the method.
        T type = (T)objectToPublish;

        // Convert the paramaterless ctor instance as xml string.
        string objectAsXmlString = type.SerializeObject<T>();

        // Send xml string over the .Net Socket 
        SendText(objectAsXmlString);

        // Log end of call message.
        Logger.Verbose("End of UDPPublisher SendObject<T> method call");
    }
    catch (Exception e)
    {
        Logger.Error(e.Message);
    }
}

you write here // Creating instance of type inside the method. but this is not what you think.

You are just casting object to T. You already have to pass an instance. Why do you set the new() constraint if you don't make use of it like new T() which it is for?

You also use generics incorrectly.

The signature of this method should be

 public void SendObject<T>(T objectToPublish, string topicToPublish)

so that you don't have to cast it. You also don't create a new T so you don't need the constraint either. Maybe a different, maybe some interface but definitely not this one.

But even if the generics were corrent I cannot find any line where you actaully use any property of T being some interface or abstract class. An object after all would probably be just fine. I think this will work too:

public void SendObject(object objectToPublish, string topicToPublish)

You shouldn't use nested classes unless really necessary. In your case namespaces would be better.

\$\endgroup\$

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.