80
\$\begingroup\$

The question is probably quite simple, but I would like to hear what drawbacks we will have with our code.

So, we had a simple implementation and interface for it:

public interface ISettings
{
    bool IsOnline { get; set; }
}

public class Settings : ISettings
{
    public bool IsOnline { get; set; }
}

The property shows the application state and used in many conditions. The interface in our case is required for implementing settings for different platforms.

My colleague insists on implementing an additional property:

public interface ISettings
{
    bool IsOnline { get; set; }
    bool IsOffline { get; set; }
}

public class Settings : ISettings
{
    public bool IsOnline { get; set; }
    public bool IsOffline
    {
        get { return !IsOnline; }
        set { IsOnline = !value; }
    }
}

Is it a good practice to have such additional properties just for convenient code-reading?

\$\endgroup\$
  • 34
    \$\begingroup\$ It is sheer waste of effort maintaining two properties and invitation to future bugs when you forget to set either of the properties in tandem. \$\endgroup\$ – Pradeep Oct 16 '13 at 13:05
  • 2
    \$\begingroup\$ I don't think I'd call this "mutually exclusive", at least not from the interface's point of view. \$\endgroup\$ – Jeroen Oct 16 '13 at 13:49
  • 4
    \$\begingroup\$ I'd avoid "negative" names as much as possible, since they often lead to double negations which can cause confusion. \$\endgroup\$ – Bakuriu Oct 16 '13 at 16:04
  • 3
    \$\begingroup\$ You should not have two read-write properties that describe the same aspect of an object. I would do what @palacsint suggests in his answer. If it's deemed necessary, you can always add read-only properties like IsOnline that simply check the status property (or use extension methods). \$\endgroup\$ – Mike Strobel Oct 16 '13 at 16:08
  • 2
    \$\begingroup\$ Side note, what the heck is up with voting here?? Keep it up, community! \$\endgroup\$ – Mathieu Guindon Oct 16 '13 at 22:27
107
\$\begingroup\$

I think that having both properties is actually somewhat harmful. If there is just single property, then it's clear what it means: settings can be either online or not.

But when you have two properties (and don't know their implementation), then you start to wonder: is there some third case? Can both properties be true, or both false? Are the two properties actually related, or is it just confusing naming?

Good documentation would answer all those questions, but it's always better if something is clear from the start and doesn't have to be explained.

\$\endgroup\$
  • 8
    \$\begingroup\$ +1 for pointing out the possibility of a third case. If the next person to read your code even spends a second wondering about this, this code ought to be re-written! \$\endgroup\$ – shivsky Oct 16 '13 at 14:06
  • 1
    \$\begingroup\$ This is a very good point! The obvious answer to the original question is (in my own mind) that you should absolutely not have both IsOnline and IsOffline. However, I had no real motivation for that other than that it just feels very wrong. \$\endgroup\$ – Boise Oct 19 '13 at 7:36
  • \$\begingroup\$ if IsOffLine did not have a setter would help mitigate the potential confusion. Then follow-up with a tool-tip such as "Alias for !IsOnLine" even more so. \$\endgroup\$ – radarbob Feb 21 '14 at 2:35
135
\$\begingroup\$

Just a quick note: a Status (or ConnectionStatus) enum with ONLINE and OFFLINE values would be more readable here.

\$\endgroup\$
  • 18
    \$\begingroup\$ +1 This allows extension of additional Statuses such as Error states or transitory states like "Connecting" \$\endgroup\$ – deepee1 Oct 16 '13 at 14:40
  • 11
    \$\begingroup\$ It also allows more specific read-only properties to be implemented easily if someone insists on having them, e.g., public bool IsConnected { get { return this.Status == ConnectionStatus.Connected; } }. Under no circumstances would I create two separate read-write properties that serve no purpose other than to invert each other. \$\endgroup\$ – Mike Strobel Oct 16 '13 at 16:01
  • 1
    \$\begingroup\$ This this this. An enum with two values is not a Boolean and should be distinguished as such. \$\endgroup\$ – Jon Purdy Oct 17 '13 at 2:17
  • 2
    \$\begingroup\$ I would say rather: A boolean is not an enum with two values and should not be implemented as one. \$\endgroup\$ – jwg Oct 17 '13 at 9:51
  • 1
    \$\begingroup\$ @JonPurdy, I understood what you meant. But in my opinion you are wrong, a boolean is not an enum with two values. Extending an enum to have a third value makes sense, extending a bool to have a third value is famously a ridiculous blunder. Adding or multiplying two enums to make a third is nonsensical, whereas adding or multiplying two booleans is fundamental. if ... else is not a special case of switch ... case. \$\endgroup\$ – jwg Oct 20 '13 at 6:05
16
\$\begingroup\$

If you implement both fields you pretty much have to guarantee for the rest of your life that they will always be consistent, i.e. that IsOnline==!IsOffline is always valid, at any point in time, forever.

Can you?

What if someone extends your class and decides to "extend" the logic, too.

public class MySettings : Settings
{
    public bool IsOnline { get; set; }
    public bool IsOffline { get; set; }
}

Isn't MySettings now a subtype of Settings and can be used wherever settings can?

What if a few years down the road someone decides that they need a third state: "going online is triggered but not yet confirmed" and instead of properly reengineering the code just decides to tweak it such that IsOffline==IsOnline==false for a short time?

What if your code is used in a multithreaded environment? If one thread sets IsOnline to some value and another thread concurrently reads IsOffline? Can you guarantee atomicity, hence thread-safety forever?

I think if something represents one bit of information it should be one bit in the code.

\$\endgroup\$
  • 1
    \$\begingroup\$ Don't make the properties virtual then? \$\endgroup\$ – jwg Oct 17 '13 at 14:43
  • \$\begingroup\$ I think inheritance is not a problem, because the properties are not virtual. So the two properties from your derived class are unrelated to the original ones (except that they have the same names). \$\endgroup\$ – svick Oct 18 '13 at 16:43
12
\$\begingroup\$

I would avoid the additional property. It's entirely unnecessary.

The benefits of including it would be to improve readability (ever so slightly) as suggested by ChrisWue's answer. Other than that it poses no advantages that would outweigh the confusion it could cause as suggested by svick (a real consideration to make if it's going to be a headache for future developers).

The whole idea of a Boolean is that it be either true or false (it already caters for both scenarios). You're undermining the purpose of a Boolean merely by creating a negated version of one.

\$\endgroup\$
6
\$\begingroup\$

If "offline" is really just a "not online" in terms of business logic here - most probably is - then the method is superfluous.

However, it is not uncommon to create similarly looking methods that from the looks of do not do anything at first glance, but serve as a placeholder to better indicate a test - and be a single point of extension should an underlying logic change later.

For example, let's say we have a method isAdminUser(), and we know what "admin users do not have avatars", so instead of:

if (!isAdminUser()) {
   paintAvatar();
}

... you do:

bool hasAvatar(){
   return !isAdminUser();
}

// ...

if (hasAvatar()) {
   paintAvatar();
}

You later have the option to extend the logic of hasAvatar() - maybe avatar became optional, or admin users on some condition can have avatar?

Now that above example is so trivial, let's get back to isOnline/isOffline. If isOffline is only "is not online", then fine. However, if what it actually means is that you can either have an "authenticated user" or an "anonymous", nobody will argue that you can easily have two methods isAuthenticated() and isAnonymous(), because you can later break down isAuthenticated() into multiple methods, while isAnonymous() will continue to serve it's purpose where code specifically focuses on anonymous access blocks.

\$\endgroup\$
5
\$\begingroup\$

Yes, it can be beneficial to add a property like this:

  1. Testing for a positive outcome in a condition usually makes it a little bit easier to read especially if the condition is a bit more complex. E.g.

    if (!IsOnline || ForceReset)
    {
        TryReconnect();
    }
    

    vs

    if (IsOffline || ForceReset)
    {
        TryReconnect();
    }
    

    Highly depends on who reads the code though and how the spec (if it exists) was written. If the spec says: Try to reconnect if the system is offline or a reset was forced then the second version is easier to connect to the spec than the first one.

  2. If you have some UI binding (for WPF for example) then having the second property can be valuable. Usually things like disabling a user control is done by setting the IsEnabled property to false. Now imagine you need to have a button enabled if the system is offline. Much easier to bind if the IsOffline property is available.

That being said: It adds additional code which needs to be maintained and unit tested (instead of two states you now have to test four states to make sure it behaves as expected) so don't go blindly around adding a negated version of very boolean property to your models. I'd do it when testing for both the negated and non-negated version is common.

\$\endgroup\$
  • 4
    \$\begingroup\$ When it comes to binding in WPF, it's fairly easy to create an IValueConverter that negates the value. But I guess having both properties could make sense in view model. \$\endgroup\$ – svick Oct 16 '13 at 8:24
  • 1
    \$\begingroup\$ Having both properties only makes sense to me if they are both read-only and are simply an interpretation of some state that is governed by another property (e.g., "ConnectionStatus"). Even having one be read-only and one read-write is odd, because they are functionally equivalent apart from the result inversion, and so the choice of which is writable is arbitrary. And, as @svick says, databinding is trivial if your framework supports simple value converters. \$\endgroup\$ – Mike Strobel Oct 16 '13 at 16:14
  • \$\begingroup\$ It get more annoying when you already have a converter because you bind to a non-boolean property like Visibility. There are workarounds but they always feel a bit clumsy. \$\endgroup\$ – ChrisWue Oct 16 '13 at 20:43
  • \$\begingroup\$ +1 to @Mark, -1 to this answer. This is definitely a WTF. \$\endgroup\$ – BlueRaja - Danny Pflughoeft Oct 16 '13 at 22:21
5
\$\begingroup\$

As @palacsint mentioned I would create a Status property, but also I would create read-only boolean getters:

public interface ISettings
{
    Status Status { get; set; }
    bool IsOnline { get; }
    bool IsOffline { get; }
}

public class Settings : ISettings
{
    public Status Status { get; set; }
    public bool IsOnline { get { return Status == Status.Online; } }
    public bool IsOffline { get { return Status == Status.Offline; } }
}

Having a property like Status makes your code more extendible. Also having one Status with one Setter will eliminate the thread-safety issue you would have with two writeable IsOnline IsOffline properties.

\$\endgroup\$
4
\$\begingroup\$

I agree with the other answers to this question, particularly palacsint's, but feel they do not focus on what I consider the most important point: you are talking about an interface.

For an interface, it is crucial that what you define be clear and unambiguous--if you provide two properties, then a consumer of the INTERFACE can not be sure that they are mutually exclusive. Even if you document that they SHOULD an implementation could easily either ignore that or have a bug such that it is not the case.

A particular class might have a justification for having two properties and given that one could be private and implemented as a negatation of the other, one might say no harm no foul.

But an interface is expected to have more than one implementation, and all an implementaion is required to do is implement the required structures, not do so in a particular manner. Introducing this ambiguity does not improve an interface.

\$\endgroup\$
3
\$\begingroup\$

You could sort of do it with the MixIn pattern. It will be a method instead of a property, but hopefully that isn't the end of the world for you. Add this static class somewhere in the same namespace as your ISettings interface:

public static class ISettingsExt
{
    public static bool IsOffline(this ISettings obj)
    {
        return !obj.IsOnline;
    }

    public static void IsOffline(this ISettings obj, bool isOffline)
    {
        obj.IsOnline = !isOffline;
    }
}

Then, any class that implements the ISettings interface will be able to invert the logic of "IsOnline" without personally having to implement it.

\$\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.