Are globals still bad?

Question

I ran across some code like this today; having a flow control flag that is visible to the whole class. My gut says this is the wrong way to handle this need for flow control (it's almost like an old time global flag in C).

(The example is in VB.Net but similar things could be done in C#.)

Public Class myClass

    Private bMyFlag As Boolean ''// <------

    private sub New
        bMyFlag = false
    end sub

    private sub mySub1
        bMyFlag = true
        mySub3()
        mySub4()
        mySubN()
    end sub

    private sub mySub2
        bMyFlag = false
        mySub3()
        mySub4()
        mySubN()
    end sub

    private sub mySub3
        if bMyFlag then
            DoSomething()
        else
            DoSomethingElse()
        end if
    end sub

    private sub mySub4
        if not bMyFlag then
            DoSomethingDifferent()
        else
            DoSomethingReallyDifferent()
        end if
    end sub

    private sub mySubN
        if not bMyFlag then
            DoNotPassGo()
        else
            DoNotCollect200Dollars()
        end if
    end sub
end class

It's obvious to me that it was grafted on after the fact. I'm planning on reworking the code so that bMyFlag is a parameter passed to mySub3, mySub4, and mySuvN. However, before I do so:

Is there a valid reason for having a flow control flag that is global to the class?

Or if not, why is this considered bad practice?

Answer 1

In this case, bMyFlag seems to be introducing unnecessary state which could complicate development (debugging, etc.). Some of your subroutines modify its value, and others perform different operations based on its current value. This seems to introduce unnecessary complexity that could be reduced with some refactoring. Why not just pass a boolean value to mySub1 and mySub2, and then have those subroutines call your other functions with that value as a parameter?

Answer 2

I dont think thats really a "global". Its just a member level field. That is perfectly acceptable in most cases (but not really for flow control - it looks like what you really need there is some class redesign).

Answer 3

This is not a global (i.e. application wide) variable, it is instance level (i.e. scoped to the class) There is nothing wrong with that, so long is that is what is intended.

So when that flag is true, if it is true for the whole instance, that is where it should be. If it is some generic thing that is used in completely different ways each time, then it should not be declared there.

Answer 4

There are very often times when it's valid to have the pattern you refer to. After all, objects are data + operations, and in your example bMyFlag is just some object data (state). If the other methods don't use bMyFlag, and only mySub3, mySub4, mySubN do, then you could make it a parameter to them. Otherwise, it's reasonable to leave it as is, and bMyFlag is a valid instance member of the class if it encapsulates some useful state of the instance.

Answer 5

Having some internal variable to store state is ok, but only (in my opinion) when you want your object to expose an interface with state. In this case it's used by private methods, and that's just bad design to me.

Answer 6

If your example code reflects the real code I'd say the use of the flag member is a bad practice. Mysub3, mysub4 etc are really doing two different things so they should be different methods. Hence no need for a flag at all.

Answer 7

It's hard to tell from a contrived example, but you may want to consider the introduction of a strategy pattern to deal with the differing behavior based on the state of the object. In this way, you'd implement the different behviors as interchangeable strategies. These strategies would be swapped in/out based on the state of the object. Each method would simply invoke the current strategy for it's behavior. Of course, I'd only do this if the strategies are complex or numerous.

For something reasonably simply you could also use inheritance, e.g., I have two states: Read-only and Editable. Locking an object takes an Editable object and returns a Read-only version of the object. In reality, there is a single object, but you have editable and readonly wrappers around it. The difference in behavior is built into the wrapper (the readonly object throws an exception if you try to change something).

If it's very simple and not pervasive, I don't see any real problem with keeping it the way it is. Eventually the if/else stuff will get complex and ugly, then you can refactor to something cleaner as I've described above.

Update based on your comments:

It really sounds like you've got a problem that is solvable with inheritance. You want a LoggableObject. This LoggableObject has two different variants: ScreenLoggable and FileLoggable. Define an interface for ILoggable and an abstract base class containing any common code. Then define a ScreenLoggable class that writes to the screen, it should inherit from your base class and contain the variant of the code that writes to the screen. Define a FileLoggable class that writes to a file. It also should inherit from your base class, but contain the variant that writes to a file. Use a factory to create the type of ILoggable that you need based on your flag. Use that according to the interface and it will do the right thing.

Answer 8

It's not global but it's still messy. If it's only used a couple of times then maybe its ok to leave for simplicity, but anything more than 2 or 3 times & you should be looking to extract out the different functionality into other classes.

If you want to keep all the code local to this class you can nest the classes within it. Some thing like the following (sorry if the vb syntax is wrong, it's been a while)

Public Class myClass

Private bMyobj As iThing ' <------

Private Sub New()
    bMyobj = null
End Sub

Private Sub mySub1()
    bMyobj = New Thing2()
    mySub3()
    mySub4()
    mySubN()
End Sub

Private Sub mySub2()
    bMyobj = New Thing1()
    mySub3()
    mySub4()
    mySubN()
End Sub

Private Sub mySub3()
    bMyObj.DoSomething()
End Sub

Private Sub mySub4()
    bMyObj.DoSomethingDifferent()
End Sub

Private Sub mySubN()
    bMyObj.PassGo()
End Sub


 Public Interface iThing
     Sub DoSomething()
     Sub DoSomethingDifferent()
     Sub PassGo()
 End Interface

 Public Class Thing1
     Implements iThing

     Public Sub DoSomething()
         ' Implementation 1
     End Sub
     Public Sub DoSomethingDifferent()
         ' Implementation 1
     End Sub
     Public Sub PassGo()
         ' Don't do it
     End Sub
 End Class
 Public Class Thing2
     Implements iThing
     Public Sub DoSomething()
         ' Implementation 2
     End Sub
     Public Sub DoSomethingDifferent()
         ' Implementation 2
     End Sub
     Public Sub PassGo()
         ' Don't collect 200 dollars
     End Sub
 End Class

End Class