0

Please take a look at the code below:

    Imports System.Data.SqlClient
Public Class Person
    Private id As String
    Private name As String

    Public Function check(ByVal personid As String) As Boolean
        'Do some checks on the person to see if it is ready for deletion
    End Function

    Public Shared Sub Delete()
        Dim v As New Vehicle
        Dim p As New Person
    End Sub
End Class

Public Class Vehicle
    Private vrm As String

    Public Function check(ByVal vehicleid As String) As Boolean
        'Do some checks on the vehicle to see if it is ready for deletion
    End Function

    Private Shared Sub Delete()
        Dim p As New Person
        Dim v As New Vehicle
        Dim objCommand As SqlCommand
        Dim objCon As SqlConnection
        Dim objDR As SqlDataReader
        Try
            Dim _ConString As String = "Data Source=IANSCOMPUTER;Initial Catalog=Test;Integrated Security=True;MultipleActiveResultSets=true"
            objCon = New SqlConnection(_ConString)
            objCommand = New SqlCommand("SELECT * FROM Person were startdate < dateadd(year,-6," & Now & ")")
            objDR = objCommand.ExecuteReader
            Do While objDR.Read
                If p.check(objDR("id")) And v.check(objDR("vehicleid")) Then
                    'Execute delete statement, which deletes the person and vehicle
                End If
            Loop
            objDR.Close()
            objCommand.Connection = objCon
            objCon.Open()
            objCommand.ExecuteNonQuery()
        Catch ex As Exception
            Throw
        Finally

        End Try
    End Sub

End Class

Notice that the shared function (Person.Delete) contains references to Person and Vehicle and uses instance variables in Person and Vehicle. Basically a check needs to be done on the Person and vehicle before the person and vehicle can be deleted.

Is it poor practice to reference Person and Vehicle from the Shared function? Is it poor practice to use instance variables in the Delete function. The Delete function will delete thousands of persons and vehicles daily.

Improve this question
6
  • Your code seems incomplete. The Person class doesn't seem to do anything. What is the purpose of making the Delete subs shared? Why are you catching an exception without handling it? Commented Mar 26, 2013 at 20:14
  • @Robert Harvey, thanks again. The Delete subs are shared because they iterate through thousands of records. On each iteration a Person object is created along with a vehicle object. I am wandering whether to create a Person and Vehicle object or whether just to put all the logic in the shared function. Commented Mar 26, 2013 at 20:21
  • Making them shared won't help the fact that you're iterating thousands of times; those objects will still get created every time you call the method. If you don't want to create all those SQL objects every time, create a helper class, make the objects private members, and instantiate them in the constructor. Commented Mar 26, 2013 at 20:26
  • @Robert Harvey, I do use an SQLHelper class in my live environment. I also separate the business logic and data logic in layers. The code above was just an attempt to explain my question, which I don't think I have done very well. Commented Mar 26, 2013 at 20:31
  • I wouldn't get too hung up on code coupling. If you have to get the person out of the vehicle before you crush the car, then your program is going to need to know that. Commented Mar 26, 2013 at 20:33

1 Answer 1

Reset to default
1

It's not necessarily a bad practice to make them shared, but if they are shared its a good practice to refactor them into their own classes. Shared instantly makes then global functions that, in my mind, should not contaminate the class signature.

So this would be better practice in my estimation:

Public Class Person
    Private id As String
    Private name As String
End Class

Public Class Vehicle
    Private vrm As String
End Class

Public Class PersonVanisher
    Public Shared Sub Delete()
        Dim v As New Vehicle
        Dim p As New Person
        ...
    End Sub

    Public Shared Function Check(personId As String) As Boolean
        ...
    End Function
End Class

Public Class VehicleCrusher
    Private Shared Sub Delete()
        Dim p As New Person
        Dim v As New Vehicle
        ...
    End Sub

    Public Shared Function Check(vehicleId As String) As Boolean
        ...
    End Function
End Class

People get into philosophical debates about this, but if it were my code I would not make anything shared. I would prefer to "New up" a VehicleCrusher when I need it and let .NET dispose everything as soon as I'm done.

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

16 Comments

Thanks. +1. Do you think it is poor practice to create objects on every loop in the Delete function i.e. on every loop a new Person and Vehicle object is created. I think I am overthinking it, personally.
Personally, i'd prefer not to have a class named VehicleManager. :P The name doesn't say much about an instance's role in things. If it's just there to delete vehicles, i'd call it CarCrusher. :) That still seems a bit odd, though, having a class that does nothing but delete objects of another class...
@cHao, +1 for CarCrusher. Do you agree with this answer accept for the name of the classes?
@w0051977: Just updated my comment. Not sure i like the idea of a VehicleManager in general. Even without the name, that's pretty much what it is. It seems kinda wacky, when your Vehicle and Person types already do that to a huge degree. (If they didn't, you wouldn't be able to just new one up at will and use it; you'd get one passed to you.)
OK, I agree 'Manager' is a bad name. I forgot I was on StackExchange. LOL. I'll update the names...
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.