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.

From times to times I stumble over the following situation:

I have got a class with a property that's only used if another property has a particular value, for instance:

public enum enum_ConnectionType
{
    Local,
    Server
}

public class Session
{
    private enum_ConnectionType _connectionType;
    private string _serverName; // It only makes sense if _connectionType == enum_ConnectionType.Server

    public Session(enum_ConnectionType connectionType, string serverName)
    {
        _connectionType = connectionType;
        _serverName = serverName;
    }
}

There's something that does not feel right to me. What is your common approach on this kind of situations?

EDIT

I think both Jeff and Leonid answers are valid depending on the situation.

share|improve this question
    
What language are you using? –  MECU Jan 8 '13 at 17:33
    
C#, I just updated the tags, but I believe this is a common situation for most of the oop languages. –  Eduardo Brites Jan 8 '13 at 17:35
    
I'd just change the line _serverName = serverName; to be _serverName = _connectionType == enum_ConnectionType.Server ? serverName : null; . So an instance of Session is internally consistent. –  Jesse C. Slicer Jan 8 '13 at 17:49
add comment

3 Answers

up vote 13 down vote accepted

I would do this using an interface and polymorphism to represent this:

public interface ISession
{
    void DoSomething();

    // Add your properties / method definitions
}

public class ServerSession : ISession
{
     private readonly string _serverName;

     public ServerSession(string serverName)
     {
         _serverName = serverName;
     }

     public void DoSomething()
     {
         // Some Code
     }

     // Implement interface contracts
}

public class OtherSession : ISession
{

     public void DoSomething()
     {
         // Some Code
     }

     // Implement interface contracts
}

Then you would use it something like this:

public DoSomethingWithSession(ISession session)
{
     session.DoSomething();
}

foo.DoSomethingWithSession(new ServerSession());

If there is commonality between ISession implementations, you could base class that out, then inherit from that.

share|improve this answer
    
This was my initial thought, thanks for posting. –  Eduardo Brites Jan 8 '13 at 17:50
4  
Did you really use “base class” as a verb? :-) –  svick Jan 8 '13 at 18:32
    
lol...guess I did :) –  Jeff Vanzella Jan 8 '13 at 18:48
3  
This is not over-engineering. It's paying your technical debt upfront. Session class, as it is, doesn't have any public behavior. You either put doSomething()s on it, or getSomething(). If you go doSomething route your Session class is peppered with if(connectionType==LOCAL) conditionals. If you go getSomething route, it is worse; everywhere Session class is used is polluted with the said conditionals. Moreover, each time you forget the conditional you get a NullReferenceException or what's its name. –  abuzittin gillifirca Jan 9 '13 at 7:39
1  
@abuzittingillifirca I disagree that its not overengineered. The problems you are worrying about in your solution may never become problems because the class may never become as complex as you're anticipating. The other answer to this question is perfectly acceptable if there are only minor and easily implemented differences between a local and remote connection. –  Andy Jan 9 '13 at 19:52
show 4 more comments

There is a different way, supplemented by documentation. One can say that it goes against OOP, one can also say that it keeps things simple and favors composition over inheritance. Here serverName = null is a convention for local connection. This approach condenses the entire state to a single string value.

This is not the only way. Sometimes this is a bad approach, sometimes a good one - depends on how big your architecture is and what your use cases are.

public class Session
{
    private readonly string serverName;

    /// Document me!    
    public Session() : this(null)
    {
    }

    /// Document me!
    public Session(string serverName)
    {
        // Assert serverName is not null or whitespace
        this.serverName = serverName;
    }

    /// Document me!    
    public bool IsLocalConnection
    {
        // Make sure that it is not whitespace
        return this.ServerName == null;
    }

    /// Document me!
    public string ServerName
    {
        get
        {
            return this.serverName;
        }
    }    
}
share|improve this answer
    
Why not just have two CreateSession methods, one which takes a string the other which takes nothing? Seems like the Session itself is the only thing that should care if its local or not. Otherwise I think this is the best answer. –  Andy Jan 8 '13 at 22:29
    
@Andy, you are right. I took your feedback and simplified my code, albeit in a slightly different way. –  Leonid Jan 8 '13 at 23:26
add comment

From a memory perspective I wouldn't worry. A few extra machine words in your class is unlikely to make a difference, and allocators out there tend to pad upwards anyway.

From an OO perspective, you can always make a subclass to add the field.

share|improve this answer
add comment

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.