3
\$\begingroup\$

I am looking for some different approaches to this following OOP scenario.

 Dim pb As Object = Nothing

 If radCars.Checked = True Then

     Dim productBase As ProductsBase(Of CarColumns)
     productBase = New Cars(fileLocation)

     pb = productBase

  ElseIf radTrucks.Checked = True Then

     Dim productBase As ProductsBase(Of TruckColumns)
     productBase = New Truck(fileLocation)

     pb = productBase

  End If

  pb.Parse()
  pb.AddRows()
  ....

Having to declare the object above and losing intelisense seems off. The base class if using generics because the Truck and Car classes use different column classes for the parsing and adding of columns in the db.

Base Class:

Public MustInherit Class ProductsBase(Of T)

    Sub New(filePathString As String)
        FilePath = filePathString
    End Sub

    MustOverride Sub Parse()
    MustOverride Sub DeleteRows()
    MustOverride Sub AddRows(ByVal rows As IEnumerable(Of T), pageNumber As Integer)

    Property ParseResult As ProductLookupContainer(Of T)

End Class

Car Class:

Public Class Cars
    Inherits ProductsBase(Of CarColumns)

    Sub New(filePath As String)
        MyBase.New(filePath)
    End Sub

    Public Overrides Sub AddRows(rows As IEnumerable(Of CarColumns), pageNumber As Integer)
        DatabaseHelper.AddItemsToDB(Of CarProduct)(ConnectionString, rows, pageNumber)
    End Sub

    Public Overrides Sub DeleteRows()
        DatabaseHelper.DeleteRows(ConnectionString, "CarProducts")
    End Sub

    Public Overrides Sub Parse()

        Dim pf As New ParseFile(FilePath)
        Dim result = New ProductLookupContainer(Of CarColumns)
        result = pf.ParseCars

       'Set object
       ParseResult = result

    End Sub

End Class
\$\endgroup\$

migrated from stackoverflow.com Sep 10 '12 at 16:43

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ I'm not a VB.Net guy, but instead of object, why don't you make pb a ProductsBase? \$\endgroup\$ – Jeff Vanzella Sep 10 '12 at 20:39
  • \$\begingroup\$ That would work perfectly, but because of the generics involved with the Add and Parse it fails. Dim foo as ProductsBase(Of ?) Setting the productbase in the if will break the cast. \$\endgroup\$ – Ryan Sep 11 '12 at 13:51
  • \$\begingroup\$ ahh, so the (of [classname]) is a generic :) Like I said, not a VB.Net guy \$\endgroup\$ – Jeff Vanzella Sep 11 '12 at 15:58
3
\$\begingroup\$

I would agree that defining pb as an object is a code smell and should be eliminated.

Either in addition or instead of having Cars and Trucks inherit from ProductsBase(of T) it should implement an interface.

Suggested Interface:

Public Interface IProducts

    Sub DeleteRows()
    Sub Parse()

End Interface

Then you would declare pb as IProducts. Add any other public properties that will be used by all the derived classes of ProductsBase.

I'd also suggest creating a class or modules that contains factory methods to generate the new objects.

Suggested Factory Class:

Public Class ProductsFactory

    Public Shared Function CreateTruck(fileLocation as string) As IProducts
         Return New Truck(fileLocation)
    End Function

    Public Shared Function CreateCar(fileLocation as string) As IProducts
         Return New Car(fileLocation)
    End Function

End Class

The final code would look like this:

  Dim pb As IProducts 

  If radCars.Checked = True Then   
     pb = ProductsFactory.CreateCar(fileLocation)
  ElseIf radTrucks.Checked = True Then
     pb = ProductsFactory.CreateTruck(fileLocation)
  End If

  pb.Parse()
  pb.AddRows()
\$\endgroup\$
  • \$\begingroup\$ Why don't you move the if statement inside the factory class? Then you could have 1 Create method that returns an IProducts. \$\endgroup\$ – Jeff Vanzella Sep 11 '12 at 2:41
  • \$\begingroup\$ I agree. Set the Tag property of each checkbox to the implementation name and call ProductsFactory.Create(selectedCheckbox.Tag) \$\endgroup\$ – jgauffin Sep 11 '12 at 5:20
  • \$\begingroup\$ I would agree with the use of selectedcheckbox.tag in most cases and moving the if statement into a single create function. I was mostly being lazy. I didn't want to write out the extra code required to do so. Just make sure the value stored in .Tag isn't a string. Using a string to tell a factory method what to create is almost always a bad idea. \$\endgroup\$ – JColeson Sep 11 '12 at 23:22

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.