Question OOP, Relatively new

[Zamdrist]

I know, so shoot me, but I'm trying! Anyone want to take a gander on what I'm doing wrong in this code? Stuff starts in the public function PrepareNewRelease. From there the host is checked to see if its up (yes, I know its wrong, ignore that for now), then the file hosted on the site is validated for date modified, and further by the number of rows returned based on a pattern (loaded via a text file).

Problem: The rr variable cannot be assigned appropriately because I've got something screwed up somewhere, just not sure.

This is my attempt to be object oriented and modular so that I can reuse some of the functionality later. For example, getting the pattern file should be separate from using the pattern file to validate the host's file. Perhaps I've gotten too ambitious or too granular? Thanks...

Option Explicit On
Imports System.Net
Imports System.IO
Imports System.Text.RegularExpressions

Public Class NewRelease

    Private Const sSite As String = "http://www.diamondcomics.com"
    Private Const sNewReleaseLocationName As String = "http://www.diamondcomics.com/shipping/newreleases.txt"
    Private Const sContentType As String = "text/plain"
    Private Const sPatternFileLocationName As String = "\PatternFile.txt"

    Private MyResponse As New Response
    Private MyMatchCount As New MatchCount
    Private Pattern As New PatternFile
    Private rRegex As New Regex(Pattern.ReturnPattern, RegexOptions.IgnorePatternWhitespace Or RegexOptions.Multiline Or RegexOptions.Singleline)
    Private sURI As New Uri(sNewReleaseLocationName)

    Private sNewReleaseContents As String

    Private Class Response
        Public wRes As WebResponse

        Public Function GetHeader(ByVal sHeader As String) As String
            Return wRes.Headers.Get(sHeader)
        End Function

        Public Function GetResponseContents() As String
            Dim str As Stream = wRes.GetResponseStream
            Return New StreamReader(str).ReadToEnd
        End Function

        Public Sub New()
            Dim wReq As WebRequest
            wReq = WebRequest.Create(sNewReleaseLocationName)
            wRes = wReq.GetResponse
        End Sub
    End Class

    Private Class PatternFile
        Dim sr As New StreamReader(Replace(Application.ExecutablePath, "\CSDBNewReleases.EXE", "") & sPatternFileLocationName)

        Public Function ReturnPattern() As String
            Return sr.ReadToEnd
        End Function

    End Class

    Private Class NewReleaseMatches

        Public Function ReturnMatches(ByVal rx As Regex, ByVal res As Response) As MatchCollection
            Return rx.Matches(res.GetResponseContents)
        End Function

    End Class

    Private Class MatchCount
        Public NRM As New NewReleaseMatches
        Public rr As Regex

        Public Function GetCount(ByVal r As Response) As Integer
            rr = rRegex
            Return NRM.ReturnMatches(rr, r).Count
        End Function

    End Class

    Private Function bIsReacheable() As Boolean

        Return My.Computer.Network.IsAvailable

    End Function

    Private Function bCheckNewRelease(ByVal FileDate As Response) As Boolean

        If bIsReacheable() AndAlso Now >= CType(FileDate.GetHeader("Last-Modified"), Date) Then
            Return True
        Else
            Return False
        End If

    End Function

    Private Function bValidateNewRelease(ByVal ContentType As Response, ByVal MyMatchCount As MatchCount) As Boolean

        If sContentType = ContentType.GetHeader("Content-Type") AndAlso MyMatchCount.GetCount(MyResponse) > 0 Then
            Return True
        Else
            Return False
        End If

    End Function

    Public Function PrepareNewRelease() As Boolean

        If bCheckNewRelease(MyResponse) AndAlso bValidateNewRelease(MyResponse, MyMatchCount) Then
            Return True

        Else
            Return False
        End If

    End Function

End Class

 

[JohnH]

Reference to a non-shared member requires an object reference.

As I see it you have two options for rRegex;

• make it Shared to be able to refer to is from afar without instance reference
• move it to a narrower scope, ie to the class or method using it (it has only one consumer here)

 

[Zamdrist]

As I see it you have two options for rRegex;

• make it Shared to be able to refer to is from afar without instance reference
• move it to a narrower scope, ie to the class or method using it (it has only one consumer here)

Thank you John. I ended up moving Pattern and rRegex inside the MatchCount class, that seemed to resolve the issue.

More to the point though, like I said I'm fairly new to the OOP paradigm. Is this code plausible OOP code, or is it a mess? Presumably given the promise of OOP I should end up with functionality that stands alone as useful parts but also can work together to provide the best chance of reuse while not sacrificing clarity. Or at least that is how I see it.

I'm wide open to suggestions or pointers. Thank you!

    Private Class MatchCount
        Public Pattern As New PatternFile
        Public NRM As New NewReleaseMatches
        Public rRegex As New Regex(Pattern.ReturnPattern, RegexOptions.IgnorePatternWhitespace Or RegexOptions.Multiline Or RegexOptions.Singleline)

        Public Function GetCount(ByVal r As Response) As Integer

            Return NRM.ReturnMatches(rRegex, r).Count
        End Function

    End Class

 

[JohnH]

I wouldn't abstract that much, neither would I use class level variables to this extent. There is also two cases where you open a file/stream and not close it. Take a look at these suggestions for example, this code has same meaning as yours, but is a lot cleaner IMO:

NewRelease class is highly refactored, some has been reordered, for example IsReacheable is checked first, then date/plain is preliminary validation before matching the content. Notice also that you can call PrepareNewRelease multiple times and it goes out with a new request each time, with your code you had to create a new NewRelease instance to do this. If you have no further instance dependency in this class all three methods can be made Shared.

Public Class NewRelease

    Public Function PrepareNewRelease() As Boolean
        If Not IsReacheable() Then Return False
        Return ValidateNewRelease()
    End Function

    Private Function IsReacheable() As Boolean
        Return My.Computer.Network.IsAvailable
    End Function

    Private Function ValidateNewRelease() As Boolean
        Using res As New Response
            If res.IsNew AndAlso res.IsPlainText Then
                Return NewReleaseMatches.IsMatch(res.GetResponseContents)
            End If
        End Using
    End Function

End Class

Took the other two classes out as Friend to clarify the simple functionality of NewRelease class.

NewReleaseMatches class did only have one usage, same as Regex.IsMatch, this suffice if there are no other uses for the actual matches. ReadAllText leaves no file open.

Friend Class NewReleaseMatches

    Private Const patternFileLocationName As String = "PatternFile.txt"

    Public Shared Function IsMatch(ByVal contents As String) As Boolean
        Dim path As String = IO.Path.Combine(Application.StartupPath, patternFileLocationName)
        Dim pattern As String = IO.File.ReadAllText(path)
        Dim rRegex As New Regex(pattern, RegexOptions.IgnorePatternWhitespace Or RegexOptions.Multiline Or RegexOptions.Singleline)
        Return rRegex.IsMatch(contents)
    End Function

End Class

Response class is a bit more specialized (IsNew, IsPlainText), and implements IDisposable to ensure closing response. Notice also the reponse is HttpWebResponse objects, that has LastModified/ContentType properties.

    Friend Class Response
        Implements IDisposable

        Private Const sNewReleaseLocationName As String = "http://www.diamondcomics.com/shipping/newreleases.txt"
        Private wRes As HttpWebResponse

        Public ReadOnly Property IsNew() As Boolean
            Get
                Return Date.Now >= wRes.LastModified
            End Get
        End Property

        Public ReadOnly Property IsPlainText() As Boolean
            Get
                Return wRes.ContentType = Net.Mime.MediaTypeNames.Text.Plain
            End Get
        End Property

        Public Function GetResponseContents() As String
            Dim str As Stream = wRes.GetResponseStream
            Using reader As New StreamReader(str)
                Return reader.ReadToEnd
            End Using
        End Function

        Public Sub New()
            Dim wReq As WebRequest = WebRequest.Create(sNewReleaseLocationName)
            wRes = CType(wReq.GetResponse, HttpWebResponse)
        End Sub

        Private disposedValue As Boolean = False        ' To detect redundant calls

        ' IDisposable
        Protected Overridable Sub Dispose(ByVal disposing As Boolean)
            If Not Me.disposedValue Then
                If disposing Then
                    ' TODO: free other state (managed objects).
                    wRes.Close()
                End If

                ' TODO: free your own state (unmanaged objects).
                ' TODO: set large fields to null.
            End If
            Me.disposedValue = True
        End Sub

#Region " IDisposable Support "
        ' This code added by Visual Basic to correctly implement the disposable pattern.
        Public Sub Dispose() Implements IDisposable.Dispose
            ' Do not change this code.  Put cleanup code in Dispose(ByVal disposing As Boolean) above.
            Dispose(True)
            GC.SuppressFinalize(Me)
        End Sub
#End Region

    End Class

Not sure if the IsNew property is right, perhaps only the date part should be compared? Could the response file have a future date?

 

[Zamdrist]

Thank you John, thank you for looking over the code to the extent you did, and for your suggestions and thoughts. Being something of a newbie to OOP I'll have to look over this and play around with it in my project to make sure I understand it, and can answer your question.

Again, thank you!

 

[Zamdrist]

Ok, I can answer your question now

IsNew is perfect. I may though have to tweak the date evaluation/comparison. The file in question is normally and predictably released anew every Monday, or the following business day were Monday to land on a holiday, such as Memorial Day coming up next week. I knew this already but was leaving that piece for later.

The code I'm writing, and will write yet will run as a Windows Service to poll for a new, updated file, parse it and ultimately send it off to a database.

The pattern file parses the web file into a columnar tab delimited result sans the header text and section titles. Part of the validation process should be to return the number of matches, and go on the assumption all is well if the count is > 0.

Ideally I'd then send the same results on to a new class for processing into a database. Ideally I wouldn't have to go get the file again, you know.

I'm not sure what else to do with the Dispose Sub and the TODO items, other than as you've already done to close the Web Response. Perhaps close the StreamReader after I've captures the file contents?

---

So I modified the IsMatch function of the NewReleaseMatches class to return the result of the Regex MatchCollection's count being > 0.

Public Shared Function IsMatch(ByVal contents As String) As Boolean
        Dim path As String = IO.Path.Combine(Application.StartupPath, patternFileLocationName)
        Dim pattern As String = IO.File.ReadAllText(path)
        Dim rRegex As New Regex(pattern, RegexOptions.IgnorePatternWhitespace Or RegexOptions.Multiline Or RegexOptions.Singleline)
        Dim mColl As MatchCollection = rRegex.Matches(contents)
        'Return rRegex.IsMatch(contents)
        Return mColl.Count > 0
    End Function

Also, the IsReachable function should test the availability of the host in question, diamondcomics.com. I have some code to do this, but in order to use it I want to avoid creating a new instance of the Response class as I need to use both WebRequest and WebResponse to make the determination. How can I adjust the code to use the objects already created and used elsewhere in the code? Thanks.

 

[JohnH]

I'm not sure what else to do with the Dispose Sub and the TODO items, other than as you've already done to close the Web Response. Perhaps close the StreamReader after I've captures the file contents?

Everything from the disposedValue declaration is generated when adding the IDisposable interface, except the Close call that I added. There is nothing else to clean up in Response class. The StreamReader and the underlying response stream is disposed when the Using block ends.

IsReachable; after checking that network is available I'd Ping the host; How to: Determine if a Remote Computer is Available in Visual Basic

 

[Zamdrist]

Everything from the disposedValue declaration is generated when adding the IDisposable interface, except the Close call that I added. There is nothing else to clean up in Response class. The StreamReader and the underlying response stream is disposed when the Using block ends.

IsReachable; after checking that network is available I'd Ping the host; How to: Determine if a Remote Computer is Available in Visual Basic

IMHO ping is an unreliable means to check a host, and as you'll see diamondcomics.com does not accept ping requests. My plan is to use HttpWebResponse rather, to get a Status and StatusDescription of "OK".

In order to do that I need to either reuse the code's existing Request and Response objects or create new ones for this purpose. I'd like to reuse the existing one.

Again, thanks for the hand holding on the code, it was much more cleaner and readable!

 

[JohnH]

Yes and no, Ping will, if accepted, verify the availability of the host, but even still the document later requested may not be available. The code I posted for ValidateNewRelease already handles this, you see the Using statement is a combined effort to both Try-Catch the code block and auto-dispose the used object, so in case the request here is made and server is not available or the document is not available this webrequest will throw an Exception, which is thus handled by ValidateNewRelease and returns default False. Anything that goes on inside the Using statement is also handled by same Try-Catch. So making two requests, or checking the state of the initial one, has no benefit here. I'd say all scenarios are handled already. You could add a IsOK property and prepend "If res.IsOK AndAlso" before checking headers, but still if request falls over this will never be called, and exception is handled as explained nevertheless.

 

[Zamdrist]

Yes and no, Ping will, if accepted, verify the availability of the host, but even still the document later requested may not be available. The code I posted for ValidateNewRelease already handles this, you see the Using statement is a combined effort to both Try-Catch the code block and auto-dispose the used object, so in case the request here is made and server is not available or the document is not available this webrequest will throw an Exception, which is thus handled by ValidateNewRelease and returns default False. Anything that goes on inside the Using statement is also handled by same Try-Catch. So making two requests, or checking the state of the initial one, has no benefit here. I'd say all scenarios are handled already. You could add a IsOK property and prepend "If res.IsOK AndAlso" before checking headers, but still if request falls over this will never be called, and exception is handled as explained nevertheless.

Ok, makes sense I suppose. I guess I was trying to separate out the reasons for 'failure' so I could write to a log file what is going on. Did the import fail because the host is unavailable or did it fail because the contents of the file changed from its expected format or content?

 

[JohnH]

To do that you will have to explicitly Try-Catch the use of the Response object and check with the exception message. Of you can check many things (as is done already), but ultimately there is no way around try-catching socket operations, at times they may fail and will throw an exception.