[AdamSpeight2008]

Quote

BC30909: 'ObsList' cannot expose type 'Observer' outside the project through class 'Observers'.

Wouldn't that suggest an issue with the accessibility (or the reach) of that class? What is the default if you don't specify one?

A little research (a quick google search) on BC30909 lead me to this MSDN Documentation on that error message

The page also had a link to a page containing of the access levels available in VB.net.

[lar3ry]

AdamSpeight2008, on 22 November 2012 - 12:10 AM, said:

Wouldn't that suggest an issue with the accessibility (or the reach) of that class? What is the default if you don't specify one?

Yes, it would (and it did), but I looked and looked and could not see the lack of a keyword in the Class. Nice to know it defaults to Private.

Thanks for the pointers!

Just fixed it, and tested it. Kinda got lost in the testing, and finished off the Observer class, and implemented a few more subs, fixed up the Form's Observers panel, and it all worked slick as can be. Great stuff!

I think I could get used to this OOP.

This post has been edited by lar3ry: 22 November 2012 - 12:20 AM

[CodingSup3rnatur@l-360]

EDIT: I apologize, I've gone a bit OTT again with the quantity of text... I've made the standalone points I made bold, and the explanations follow each point.

Good job, the Observer class is looking much better now. There were just a few things that occurred to me looking at that code as it is now.

1) I would make the fields in the Observer class read-only, so that they cannot be changed once set. You then have a totally immutable class, which has great benefits. Here are three benefits off the top of my head:

* Greatly simplifies multithreaded programming if you ever introduce extra threads.
* We can more easily reason about things that don't change, thus making the code easier to follow.
* Keeps code side effect free.

It's not a hard and fast rule like it is with structures, but immutability is often a desirable quality, even when using classes.

2) I noticed you are exposing the list of observers publicly to the outside world. I would firstly get rid of the set on the property, and make it read-only. Otherwise, other objects can reach in, and completely reassign the list to a brand new list. It is the Observers class's list, only that class should be able to reassign it.

So, the property becomes:

Public ReadOnly Property ObsList() As List(Of Observer)
    Get
        Return _obslist
    End Get
End Property

Alternatively, you could just make the setter private to make the property seem read only to external objects only:

Public Property ObsList() As List(Of Observer)
    Get
        Return _obslist
    End Get
    Private Set(Value As List(Of Observer))
        Me._obsList = Value
    End Set
End Property

Now, I still consider both those approaches to be a problem. External objects can still reach in, and mess with the list (they can add elements, remove elements, clear the entire list etc). This shouldn't be the case, as at that point, the Observers class isn't justifying its existence as it isn't providing any beneficial abstraction, as it stands. The operations you want to allow external objects to perform on the list should be exposed and limited through dedicated methods that you write (exactly as you have started with the add and delete stubs). Other objects should not be able to change the list directly.

At this point, I am not sure what to say, because I don't exactly know how your application is going to work. However, I will say that if you don't need to give external objects access to the entire list, get rid of that property all together.

If you do need to give callers access to the entire list, I'd give them a read only view of the collection. Some of the ways to do that is:

Private ReadOnly _obsList As New List(Of Observer)()

Public ReadOnly Property ObsList As IEnumerable(Of Observer)
    Get
        Return Me._obsList
    End Get
End Property

Private ReadOnly _obsList As New List(Of Observer)()

Public ReadOnly Property ObsList As IList(Of Observer)
    Get
        Return Me._obsList.AsReadOnly()
    End Get
End Property

or if you are using VB 11 (.NET 4.5):

Private ReadOnly _obsList As New List(Of Observer)()

Public ReadOnly Property ObsList As IEnumerable(Of Observer)
    Get
        For Each obs As Observer In me._obsList
            Yield Return obs
        Next obs
    End Get
End Property

The last two probably being the most foolproof.

Now, because each of those effectively provides a read only view of the underlying collection, callers cannot modify the collection directly. Further, as IEnumerable(Of T) and, to a lesser extent, IList<T>, are very general interfaces that many collections implement, you will be free to change the underlying collection from a list to another type of collection in the future if necessary, without breaking the calling code.

EDIT: I should mention however, that you do have to be careful not to modify the collection indirectly (by calling addObserver() for example) whilst iterating through the read only view of observers with a for each loop. If you do modify the collection whilst iterating through, an exception will be thrown.

If you need to modify the collection whilst you are iterating through it with a for each loop, you would need to take a copy of collection, and return that instead.

You could also implement IEnumerable(of Observer) in the Observers class, and given the name of the class, that could be the most logical, I suppose.

3) I also just wanted to mention was the way you are instantiating a StreamReader, and reading from a file in the constructor of the class. Feel free to ignore this, but the tester in me is slightly disturbed by that. The tester in me suggests you should perhaps move that file reading code into another class, inject an instance of that class into the constructor of the Observers class, and the call ReadObservers() or something similar on that instance, in order to get the list of Observers.

That way, it becomes much easier to mock out the file reading code in favor of an in memory collection of observers whilst testing, and the responsibilities are more evenly distributed across the code base. Remember, one class should ideally have one responsibility, and hence only one reason to change.

Alternatively, you could read the observers into a list first, and then pass that list into the Observers constructor, taking a copy of the list that was passed in, in order to prevent caller from being able to change the list.

You may even find (depending on what you have planned) that the Observers class actually adds no real value to the solution after all, and a simple standalone list of Observer instances will do. I would say that it is debatable what value that class is adding at it stands, from the point of view of an outsider looking in.

Whatever you do, I personally would tend to avoid directly reaching out to external resources in the constructors of classes, or indeed having direct external resource interaction anywhere in a class that shouldn't have to deal with it directly.

4) Be sure to call Dispose() on the StreamReader to free the file handle it has acquired. You can let compiler handle this for you using a using statement, as so:

  
Using sr As New StreamReader("Observers.txt")
     'TODO: Insert file reading code
End Using 'Dispose() is called automatically when we get here

This post has been edited by CodingSup3rnatur@l-360: 22 November 2012 - 07:55 AM

[lar3ry]

CodingSup3rnatur@l-360, I feel I should be the one to apologize. Last night, just before I posted my last reply, I did a lot of work on the two classes. In fact, I had started to reply, and then got sidetracked into actually working on it, and probably an hour later, got back and finished the reply, completely forgetting to post the result. Anyway, your comments (which are VERY helpful aand welcome), are based on the code I posted before, so here's what it is now:

Option Explicit On
Option Strict On
Imports System.IO

Public Class Observers

    Private _obslist As New List(Of Observer)

    Public Property ObsList() As List(Of Observer)
        Get
            Return _obslist
        End Get
        Set(ByVal ObsList As List(Of Observer))
            _obslist = ObsList
        End Set
    End Property

    Public Sub New()
        If File.Exists("Observers.txt") Then
            Dim sr As New StreamReader("Observers.txt")
            Dim s() As String
            Do
                s = Split(sr.ReadLine(), ",")
                Dim obs As New Observer(s(0), s(1), s(2), s(3), s(4), s(5), s(6))
                _obslist.Add(obs)
            Loop Until sr.Peek = -1
        End If
    End Sub

    Public Sub addObserver(ByVal Ob As Observer)
        _obslist.Add(Ob)
    End Sub

    Public Sub saveObservers(ByVal obslist As List(Of Observer))

    End Sub

End Class

Public Class Observer
    Private _name As String
    Private _obslat As Double
    Private _obslong As Double
    Private _obselev As Integer
    Private _utcoffset As Int16
    Private _obsdst As Boolean
    Private _obsemail As String

    Public Property Name As String
        Get
            Return _name
        End Get
        Set(ByVal Name As String)
            _name = Name
        End Set
    End Property

    Public Property ObsLat As Double
        Get
            Return _obslat
        End Get
        Set(ByVal ObsLat As Double)
            _obslat = ObsLat
        End Set
    End Property

    Public Property ObsLong As Double
        Get
            Return _obslong
        End Get
        Set(ByVal ObsLong As Double)
            _obslong = ObsLong
        End Set
    End Property

    Public Property UTCOffset As Int16
        Get
            Return _utcoffset
        End Get
        Set(ByVal UTCOffset As Int16)
            _utcoffset = UTCOffset
        End Set
    End Property

    Public Property ObsElev As Integer
        Get
            Return _obselev
        End Get
        Set(ByVal ObsElev As Integer)
            _obselev = ObsElev
        End Set
    End Property

    Public Property ObsDST As Boolean
        Get
            Return _obsdst
        End Get
        Set(ByVal ObsDST As Boolean)
            _obsdst = ObsDST
        End Set
    End Property

    Public Property ObsEmail As String
        Get
            Return _obsemail
        End Get
        Set(ByVal ObsEmail As String)
            _obsemail = ObsEmail
        End Set
    End Property

    Public Sub New(ByVal Name As String,
                   ByVal ObsLat As String,
                   ByVal ObsLong As String,
                   ByVal ObsElev As String,
                   ByVal UTCOffset As String,
                   ByVal ObsDST As String,
                   ByVal ObsEmail As String
                   )
        _name = Name.Trim
        _obslat = CDbl(ObsLat)
        _obslong = CDbl(ObsLong)
        _obselev = CInt(ObsElev)
        _utcoffset = CShort(UTCOffset)
        _obsdst = CBool(If(ObsDST.Trim = "Y", vbTrue, vbFalse))  's(5).Trim
        _obsemail = ObsEmail.Trim
    End Sub
End Class

In your comments, you mentioned that I should make the Observer class ReadOnly. I wasn't going to do that because I wanted the user to be able to edit the contents. Most of the fields are such that they would change only rarely (Lat. Long, Elev, UTCOffset, and DST), but EMail and Name could be changed more often.

I was thinking (often dangerous) that perhaps I could have an "Edit" form and a Button that would bring it up with the currently displayed Observer values, and after making changes, have it create a New Observer, add it to the Observers , delete the older version, and re-Get the list from the class (all actual changes being made in the class).

Click Edit
Make changes
Commit
Form sends a Remove(Observer) 'the one with the old values
Form sends an Add(Observer) ' New one with new values
Get(Obslist)


Am I way off base with that, or am I perhaps starting to get it?

Thanks again for your extensive guidance.

[lar3ry]

Quote

You may even find (depending on what you have planned) that the Observers class actually adds no real value to the solution after all, and a simple standalone list of Observer instances will do. I would say that it is debatable what value that class is adding at it stands, from the point of view of an outsider looking in.

Yes, I think you're right. I can have my list in the form itself, and do everything directly. The only thing useful that Observers does is to get some code out of the way, and a #region would do that.
I will Keep Observer, and make the properties ReadOnly.

BTW, I originally wrote this program in VB6, and it was a lot of fun. I have since had some ideas to speed it up, and some automated reporting of results to the Observers who supply an email address. It's about time I got back to it. The old one won't run on Windows 7.

[CodingSup3rnatur@l-360]

If the user needs to be able to edit the properties of the Observer class, then absolutely make them mutable. When I was talking about using ReadOnly, I really meant if you have a class that has fields/properties that genuinely don't need to change (as it seemed like you had with the original Observer class), enforce that explicitly with ReadOnly. If properties naturally need to be mutable, then there is absolutely no need to bend over backwards to avoid changing them, and really it would be pretty silly, and possibly detrimental to try. After all, that is what setters are for

In short, make things explicitly immutable only where it makes good design sense to do so.

Moving on, I would say it's fine to hold the list of observers in the form itself, but try not to let that draw you in to focusing on things from the perspective of the UI. Try focusing more on building code around the problem domain, using objects from said domain, independent of any UI. As soon as you start getting domain or application logic creeping into the UI, you should quickly think about moving things out of the UI.

Simplest possible example from what you have said could be the determining of whether to send an email to an observer or not. Rather than having the form check if the observer's ObsEmail property is empty or not, which would work, it would be better from an OOP point of view to encapsulate that decision into the Observer class itself, perhaps in a read only property called RequiresEmail.

Truly embracing the OOP paradigm requires you to think in terms of objects that exist in the problem domain, and encapsulate appropriate data AND behavior into those objects.

[lar3ry]

And I owe you many thanks again!

I get the feeling that I am a man with a new hammer, looking for something to hit with it.

[lucky3]

Go lar3ry!