VB.NET - Misusing instance variables?

Question

Please take a look at the code below:

    Public Class A
    Public person1 As Person
End Class

Public Class B
    Inherits A

    Public Function CheckGender() As Boolean
        If person1._Gender = "M" Then
            CheckGender = True
        End If
    End Function

End Class

Public Class C
    Inherits B
    Public Function CheckAge() As Boolean
        If person1._Age > 30 Then
            CheckAge = True
        End If
    End Function

End Class

Public Class D
    Inherits C

    Public Sub SomeMethod()
        Dim list As List(Of Person) = New List(Of Person)
        Dim p1 As New Person("Ian", "M", 31)
        Dim p2 As New Person("Susan", "F", 20)
        Dim p3 As New Person("Mark", "M", 22)
        list.Add(p1)
        list.Add(p2)
        list.Add(p3)

        For Each Person As Person In list
            person1 = Person
            If CheckAge() And CheckGender() Then
                'Do something
            End If
        Next
    End Sub

    Public Shared Sub Main()
        Dim d As New D
        d.SomeMethod()
    End Sub

End Class

Public Class Person
    Public _Name As String
    Public _Gender As String
    Public _Age As String
    Public Sub New(ByVal Name As String, ByVal Gender As String, ByVal Age As Integer)
        _Name = Name
        _Gender = Gender
        _Age = Age
    End Sub
End Class

c.SomeMethod loops through three persons and does two checks: b.CheckGender and c.CheckAge. CheckGender and CheckAge use an instance variable from the superclass A.

The code in the live environment loops through 100,000 records daily in a database and deletes those where CheckGender and CheckAge are both true. Is it a bad design choice to use instance variables in this scenario? I was always taught to use local variables. I would expect the Person object to be passed to CheckGender and CheckAge on each loop. Or does it really not matter?

Please note that the above code is a hypothetical example. CheckGender and CheckAge are complex functions in the actual application.

stackh34p · Accepted Answer · 2013-04-03 20:26:14Z

2

As long as CheckGender and CheckAge are not accessing any private, protected or internal member of the classes in hierarchy, and are public functions, and their logic is the same for any instance, being A, B, or C, it is a better design to have these methods in another class. Make them static if possible. You can have them accept the most general implementation of your classes (A for instance) that allows checking either the age or gender. Taken from your code, you can even pass the Person property instead of using any of the A, B and C classes.

Use of inheritance in the above case and such logic is permitted though, as long as you need to do any or all of the following:

Conform to a specific interface or base class, that A, B and C classes have to implement/extend, and that interface or base class provides the CheckGender and CheckAge methods. This can be the only solution if you pass your objects to 3rd party API, that accepts the base class/interface as an argument and internally calls the check methods.

Here is example in C#:

public static class CheckUtil
{
    public static bool CheckAgeOfPerson(Person p)
    {
        return p.Age > 30;
    }
    public static bool CheckAgeOfObject(A obj)
    {
        // NOTE: obj.person1 must be accessible - by being either public or internal.
        // If this class is in another assembly, internal is not useful
        return CheckAgeOfPerson(obj.person1);
    }
}

A objA = ...;
B objB = ...;
C objC = ...;

CheckUtil.CheckAgeOfObject(objA);
CheckUtil.CheckAgeOfObject(objB);
CheckUtil.CheckAgeOfObject(objC);

CheckUtil.CheckAgeOfPerson(objA.person1);
CheckUtil.CheckAgeOfPerson(objB.person1);
CheckUtil.CheckAgeOfPerson(objC.person1);

Provide specific implementation to the classes. If you have to some logic in CheckAge for instances of A, but either completely different validation for instances of B, or a combination of the existing and some new logic in C, then inheritance is your friend. Still, if that is the case, I'd prefer exposing the CheckGender and CheckAge to interface and call them via the interface. That way, inheritance is valid, but not mandatory, as long the interface is satisfied.

here is an example in C#:

public interface IGenderCheckable
{
    bool CheckGender();
}

public interface IAgeCheckable
{
    bool CheckAge();
}

public class A : IAgeCheckable, IGenderCheckable
{
    public virtual bool CheckGender()
    {
        return this.person1.Gender.Equals("M");
    }

    public virtual bool CheckAge()
    {
        return this.person1.Age > 30;
    }
}

public class B : A
{
     public override bool CheckAge()
     {
          // combine custom logic with new logic
          return this.person1.Age < 0 || base.CheckAge();
     }
}

For complex scenarios, a combination of both approaches might be also used (of course for far more complex cases than age and gender checks):

public class A : IAgeCheckable, IGenderCheckable
{
    ...
}

public static class CheckUtil
{
    public static bool CheckAge(IAgeCheckable obj)
    {
        return obj.CheckAge();
    }
    public static bool CheckGender(IGenderCheckable)
    {
        return obj.CheckGender();
    }
}

About usage of instance variables vs local variables - there is a drawback in performance of using instance variables in .NET especially when they are value types. Use of local member that is int _someIntMember for example gets translated to this._someIntMember - which in turn calls the heap to get the this object, and then accesses its _someIntMember member. Having the member as a local variable, puts its value in the stack, and reads it from there without the unnecessary roundtrip trough the heap. Moreover, the stack is faster than the heap.

However, I cannot say whether too much heap usage is an abuse of it, neither a misuse of local variables when they are used too much. This depends on the performance needed, and the complexity of code. Sometimes local variables make the code more-readable, but if too many, you could easily forget what each one was (and that can be more serious issue than the negligent performance gain). So it is a matter of style and necessity.

edited Apr 3, 2013 at 20:26

answered Apr 3, 2013 at 19:44

stackh34p

9,02913 gold badges70 silver badges112 bronze badges

Sign up to request clarification or add additional context in comments.

15 Comments

w0051977 Over a year ago

Thanks. +1. I will have a think about this. Can you answer the original question though. Is it poor practice to use instance variables in a loop or does it not really matter? Please don't think I am suggesting that you have not answered my question (leave your original answer in tact).

w0051977 Over a year ago

You are the only answerer to this question. I don't understand your last comment. Sorry.

stackh34p Over a year ago

sorry, I thought instead about that one: stackoverflow.com/questions/15772573/…. I am too tired today I guess...

w0051977 Over a year ago

sorry I don't know what you mean. Do you believe it is poor practice? Thanks again.

stackh34p Over a year ago

It is performance-related, but certainly not poor. The cons of too many locals is potential confusion over minor performance improvement - I mean the improvement in performance may the lesser good of having readable and maintainable code.

|

Jim Wooley · Accepted Answer · 2013-04-03 21:03:11Z

2

In case you're interested in "fixing" your code to make Person a Property rather than a field, change the implementation of Class A as follows:

Public Class A
    Public Property Person1 As Person

   Public Overridable Function ComputeAge() As Integer
       Return Person1.Age
   End Function

End Class

The benefit here is you have the ability to add additional abstractions over getting and setting this property down the road if you need to. The compiler will generate a hidden private backing field for the auto property. You would still access the Person1 from any implementing classes:

Public Class B
   Inherits A

   Public Overrides Function ComputeAge() As Integer
       Return MyBase.Person1.Age.AddYears(1)
   End Function
End Class

answered Apr 3, 2013 at 21:03

Jim Wooley

10.4k1 gold badge27 silver badges44 bronze badges

2 Comments

stackh34p Over a year ago

+1. Properties are both good and bad though. The good is that they usually get inlined by the compiler as part of optimization - and that creates the locals automatically. The bad is, if the property is not your code, and you are not sure what it actually does. Even the getter of a property might execute unexpected logic in its implementation that will make you wonder where is the bottleneck (like NHibernate entity proxy of lazy property). As long it is not an issue and there is not risk for it to be, its a good practice.

w0051977 Over a year ago

@Jim Wooley, thanks. Do you think person should be an instance variable of A or do you think it should be passed as a parameter to CheckAge and CheckGender. Or does it not really matter? +1.

Collectives™ on Stack Overflow

VB.NET - Misusing instance variables?

2 Answers 2

15 Comments

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

15 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related