Check xml serialization/deserialization of list

[dwigley]

Below is code for drawing arrows onto a Windows form and saving them into an XML file, which can be used to open and reload the arrows. To use it you will need a Button1, Form1, MenuStrip1 (with open and save on it), SaveFileDialog1, and OpenfileDialog1. Click button each time to get cross to draw an arrow. Draw as many arrows as you want. Point at ends of arrows to get SizeAll cursor to move arrows around and resize them. To save the arrows, choose save from menu. You can save the file of arrows as an XML file (serialization) anywhere you choose, like on your desktop. After saving the file, you can later use the file to reload the arrows (deserialization) that you saved. The arrows should reappear exactly how you drew them, and you will be able to move them around again. You might run into some message boxes reminding you to save your work, and things like that.

Am I doing this serialization correctly? It all seems to work.

The next problem I have is to try to do the same thing with text boxes. I'm able to make moveable and resizable text boxes, but I'll need to make a list of textboxes that will fit with the list of arrows here so that I can serialize them all to save them in an XML file like I have done here with the arrows. I'm trying to make sure that I'm on the right track and not doing anything now that will stump me in the future.

Thank you.

Option Strict On
Option Explicit On
Imports System.IO
Imports System.Xml.Serialization

Public Class Form1

    Private myPen As Pen = New Pen(Drawing.Color.Black) With {.Width = 2,
        .CustomEndCap = New Drawing2D.AdjustableArrowCap(6, 6, False)}
    Private drawStartArrow As Boolean
    Private listStartCount As Integer
    Private listEndCount As Integer
    Private Arrow As New ReasonArrows
    Private chkArrow As New ReasonArrows
    Private listArrows As New List(Of ReasonArrows)
    Private saveWork As Boolean = True

    Private Sub Form1_Load(sender As Object, e As EventArgs) Handles MyBase.Load
        Height = 800
        Width = 1000
    End Sub

    Private Sub Form1_MouseDown(sender As Object, e As MouseEventArgs) Handles Me.MouseDown
        Me.DoubleBuffered() = True
        If e.Button = MouseButtons.Left And Cursor = Cursors.Cross Then
            Arrow.StartPoint = e.Location
        ElseIf e.Button = MouseButtons.Left And Cursor = Cursors.SizeAll Then
            For checkPoint As Integer = 0 To listArrows.Count - 1
                If Math.Abs(e.X - listArrows(checkPoint).StartPoint.X) < 5 And
                   Math.Abs(e.Y - listArrows(checkPoint).StartPoint.Y) < 5 Then
                    Cursor.Position = PointToScreen(listArrows(checkPoint).StartPoint)
                    listArrows(checkPoint).StartPoint = New Point(e.X, e.Y)
                    listStartCount = checkPoint
                    drawStartArrow = True
                ElseIf Math.Abs(e.X - listArrows(checkPoint).EndPoint.X) < 5 And
                Math.Abs(e.Y - listArrows(checkPoint).EndPoint.Y) < 5 Then
                    Cursor.Position = PointToScreen(listArrows(checkPoint).EndPoint)
                    listArrows(checkPoint).EndPoint = New Point(e.X, e.Y)
                    listEndCount = checkPoint
                    drawStartArrow = False
                End If
            Next
        End If
    End Sub

    Private Sub Form1_MouseMove(sender As Object, e As MouseEventArgs) Handles Me.MouseMove
        Me.DoubleBuffered() = True
        If e.Button = MouseButtons.Left And Cursor = Cursors.Cross Then
            Arrow.EndPoint = e.Location
            Me.Invalidate()
        ElseIf e.Button = MouseButtons.Left And Cursor = Cursors.SizeAll And drawStartArrow = True Then
            listArrows(listStartCount).StartPoint = e.Location
            Invalidate()
        ElseIf e.Button = MouseButtons.Left And Cursor = Cursors.SizeAll And drawStartArrow = False Then
            listArrows(listEndCount).EndPoint = e.Location
            Invalidate()
        ElseIf Cursor = Cursors.Arrow And Not e.Button = MouseButtons.Left Then
            For checkPoint As Integer = 0 To listArrows.Count - 1
                If Math.Abs(e.X - listArrows(checkPoint).StartPoint.X) < 5 And
                    Math.Abs(e.Y - listArrows(checkPoint).StartPoint.Y) < 5 Then
                    chkArrow.StartPoint = listArrows(checkPoint).StartPoint
                    Cursor = Cursors.SizeAll
                ElseIf Math.Abs(e.X - listArrows(checkPoint).EndPoint.X) < 5 And
                    Math.Abs(e.Y - listArrows(checkPoint).EndPoint.Y) < 5 Then
                    chkArrow.EndPoint = listArrows(checkPoint).EndPoint
                    Cursor = Cursors.SizeAll
                End If
            Next
        ElseIf Cursor = Cursors.SizeAll And Not e.Button = MouseButtons.Left Then
            If (e.X >= chkArrow.StartPoint.X + 5 Or e.X <= chkArrow.StartPoint.X - 5 Or
            e.Y >= chkArrow.StartPoint.Y + 5 Or e.Y <= chkArrow.StartPoint.Y - 5) And
            (e.X >= chkArrow.EndPoint.X + 5 Or e.X <= chkArrow.EndPoint.X - 5 Or
            e.Y >= chkArrow.EndPoint.Y + 5 Or e.Y <= chkArrow.EndPoint.Y - 5) Then
                Cursor = Cursors.Arrow
            End If
        End If
    End Sub

    Private Sub Form1_Paint(sender As Object, e As PaintEventArgs) Handles Me.Paint
        e.Graphics.SmoothingMode = Drawing2D.SmoothingMode.AntiAlias
        If Cursor = Cursors.Cross Then
            e.Graphics.DrawLine(myPen, Arrow.StartPoint, Arrow.EndPoint)
        End If
        For Each ArrowToDraw In listArrows
            e.Graphics.DrawLine(myPen, ArrowToDraw.StartPoint, ArrowToDraw.EndPoint)
        Next
    End Sub

    Private Sub Form1_MouseUp(sender As Object, e As MouseEventArgs) Handles Me.MouseUp
        If Cursor = Cursors.Cross And e.Button = MouseButtons.Left Then
            listArrows.Add(New ReasonArrows With {.StartPoint = Arrow.StartPoint, .EndPoint = Arrow.EndPoint})
            Cursor = Cursors.Arrow
            saveWork = False
        End If
    End Sub

    Private Sub Button1_Click(sender As Object, e As EventArgs) Handles Button1.Click
        Cursor = Cursors.Cross
    End Sub

    Private Sub SaveAsToolStripMenuItem_Click(sender As Object, e As EventArgs) Handles SaveAsToolStripMenuItem.Click
        With SaveFileDialog1
            .InitialDirectory = "C:\"
            .Title = "Specify Destination Filename"
            .Filter = "XML Files (*.xml)|*.xml|Text Files (*.txt)|*.txt"
            .FilterIndex = 1
            .FileName = "Your File Name"
            .OverwritePrompt = True
            .CreatePrompt = True
            If .ShowDialog() = Windows.Forms.DialogResult.OK Then
                Dim objStreamWriter As New StreamWriter(SaveFileDialog1.FileName)
                Dim xmlSerialize As New XmlSerializer(listArrows.GetType) 'Determine what object types are present
                xmlSerialize.Serialize(objStreamWriter, listArrows)
                objStreamWriter.Close()
                .Dispose()
                saveWork = True
            End If
        End With
    End Sub

    Private Sub OpenToolStripMenuItem_Click(sender As Object, e As EventArgs) Handles OpenToolStripMenuItem.Click
        If saveWork = False Then
            MessageBox.Show("Just a reminder -- you haven't SAVED your current work yet.")
        End If
        With OpenFileDialog1
            .InitialDirectory = "C:\"
            .Title = "Select a File"
            .FileName = "Select a File"
            .Filter = "XML Files (*.xml)|*.xml|Text Files (*.txt)|*.txt"
            .FilterIndex = 1
            If .ShowDialog() = Windows.Forms.DialogResult.OK Then
                listArrows.Clear()
                Dim objStreamReader As New StreamReader(OpenFileDialog1.FileName) 'Read File
                Dim serializer As New XmlSerializer(GetType(List(Of ReasonArrows)))
                listArrows = CType(serializer.Deserialize(objStreamReader), List(Of ReasonArrows))
                Me.Invalidate()
                objStreamReader.Close()
                .Dispose()
            End If
        End With
    End Sub
End Class

Public Class ReasonArrows
    Public Property StartPoint As Point
    Public Property EndPoint As Point
End Class

[.paul.]

.Dispose()???

Change With ... etc

to Using ... etc

[Sitten Spynne]

Hmm. I'm going to go top to bottom and say what I think. A lot of it is negative, that doesn't mean I think it's bad code!

The Pen at the top may or may not be a good idea. They're IDisposable, which means you normally call Dispose() ASAP. Pens are system resources and Windows only gives you so many. BUT: since it's just one and you're using the same one over and over again, I could see an argument for caching it. "Mostly harmless".

I don't like your variable names. This will be a few points.

First, be consistent in capitalization. It's sloppy and confusing when you mix them. "Arrow" should be "arror" to match your convention.

Next: "ReasonArrows" is plural, as in "many arrows". So why is "Arrow" a "ReasonArrows"? "Arrow" is "one arrow", how can it also be "many arrows"? This kind of mismatch causes cognitive dissonance in a task that's already really hard. What is "chkArrow"? Why is it missing its vowels? I don't think "listArrows" needs to have 'list' in it if we could say "a plural name means plural objects". Having something called "arrows" would imply it's a list!

Why do you set Me.DoubleBuffered multiple times? It's normally set once. There's no effect if you set it to the same value again. Set it in the Load handler.

I think your code could benefit from some "intent-revealing variables", as Code Complete called them. Or maybe it was Clean Code. What I mean is it's hard to look at "Math.Abs(e.X - listArrows(checkPoint.StartPoint.X)" as a reader and figure out what it means. It would be easier if:

Dim startDistanceX = Math.Abs(...)
Dim startDistanceY = Math.Abs(...)
If startDistance < 5 AndAlso startDistanceY < 5 Then
    ...

It often turns out this is a ton easier to debug and maintain.

I'm thinking by the end of MouseDown that "Arrow" should be called "currentArrow".

Good work calling Invalidate() instead of Refresh(). It's much nicer.

Consider some helper functions for gigantic blocks of conditionals. Imagine if the end of MouseMove looked like:

ElseIf Cursor = Cursors.SizeAll AndAlso...
    If HasMousePassedThreshold(e.X, e.Y) Then
        Cursor = Cursors.Arrow
    End If
End If

I'd write that with intent-revealing variables, too:

Function HasMousePassedThreshold(x As Integer, y As Integer) As Boolean
    Dim startDistanceX = Math.Abs(e.X - chkArrow.StartPoint.X)
    Dim startDistanceY = ...
    ...

    Return 5 <= StartDistanceX OrElse 5 <= startDistanceY ...
End Function

I can't figure out what 'saveWork' does yet, and I'm past MouseUp.

I don't really like that you're using the Cursor property as a state variable. I get now things like, "If it's a cross, I'm in 'start' mode. If it's an arrow I'm in 'drawing' mode." I think having an enum-backed 'currentMode' variable or even a set of Booleans like 'isDrawing' helps keep things logical, and means you can offload a lot of work to:

Sub UpdateCursor()
    Select Case mode
        Case DrawMode.Start:
            Cursor = Cursors.Cross
        ...

.paul. is right: most of your IDisposables like the StreamReader should be managed with a Using statement:

Using reader As New StreamReader(OpenFileDialog1.FileName)
    ...
End Using

Using statements automatically call Dispose() when the "End Using" is reached, even if an exception is thrown. Note that you were careful to dispose the FileOpenDialog, which is rarely done in practice, but you didn't dispose the StreamReader. Failure to dispose a FileOpenDialog is impolite. Failure to dispose a StreamReader means "the file may be locked and inaccessible until the system reboots."

I finally got to "ReasonArrows". It represents one arrow. Name it "ReasonArrow"!

Despite there being a lot of things here, most of them are very minor suggestions. The only one I think you HAVE to fix is you should make sure to use Using statements for your filesystem objects. Everything else can be ignored with little consequence.

Well, that and the naming. Naming is important. But it doesn't affect your program in ways that are as easy to notice as "whoops the file is locked".

[dwigley]

Ok, thank you. I'll look into that, .paul.

[dwigley]

Sitten Spynne, thank you for taking the time to give me all the good ideas. I agree with what you say about my names, and I know I need to do better. What's happening is that I'm not sure what I'm doing, and when I write a name, I might not even know what the heck it's for until I figure it out later. The names end up being very idiosyncratic. Looks like "Code Complete" is a book I need to read.

The "saveWork" is for sending the user a message reminding her that she hasn't saved her work when she goes to load something. There's probably a better way to do that.

There is a '.Dispose()' on the reader. At least I put it under '.objStreamReader.Close()'. Maybe it shouldn't go there?

'ReasonArrows' is the name of a class of objects. The class is an abstract object to be filled with individuals. So I made 'ReasonArrows' plural. One arrow will be a member of this class of 'ReasonArrows', like this one chair I'm sitting in belongs to the class of chairs. So that's why I said 'arrow' (singular) as 'ReasonArrows' (plural). One arrow is made each time. Each arrow belongs to the class of 'ReasonArrows'. And 'listArrows' stands for a list of a number of individual arrows that belong to the class of 'ReasonArrows'. That's how I was conceptualizing all this.

You have pointed out a good number of things that I need to look into. I can't understand it all, but I will eventually, I hope.

Thanks for the great help.

[.paul.]

Sitten Spynne, thank you for taking the time to give me all the good ideas. I agree with what you say about my names, and I know I need to do better. What's happening is that I'm not sure what I'm doing, and when I write a name, I might not even know what the heck it's for until I figure it out later. The names end up being very idiosyncratic. Looks like "Code Complete" is a book I need to read.

The "saveWork" is for sending the user a message reminding her that she hasn't saved her work when she goes to load something. There's probably a better way to do that.

There is a '.Dispose()' on the reader. At least I put it under '.objStreamReader.Close()'. Maybe it shouldn't go there?

'ReasonArrows' is the name of a class of objects. The class is an abstract object to be filled with individuals. So I made 'ReasonArrows' plural. One arrow will be a member of this class of 'ReasonArrows', like this one chair I'm sitting in belongs to the class of chairs. So that's why I said 'arrow' (singular) as 'ReasonArrows' (plural). One arrow is made each time. Each arrow belongs to the class of 'ReasonArrows'. And 'listArrows' stands for a list of a number of individual arrows that belong to the class of 'ReasonArrows'. That's how I was conceptualizing all this.

You have pointed out a good number of things that I need to look into. I can't understand it all, but I will eventually, I hope.

Thanks for the great help.

That Dispose() is called on the With object, not the reader.
Your class would be ReasonArrow (as singular), and your list of that class object(as a multiple) would be ReasonArrows

[Sitten Spynne]

'ReasonArrows' is the name of a class of objects. The class is an abstract object to be filled with individuals. So I made 'ReasonArrows' plural. One arrow will be a member of this class of 'ReasonArrows', like this one chair I'm sitting in belongs to the class of chairs. So that's why I said 'arrow' (singular) as 'ReasonArrows' (plural). One arrow is made each time. Each arrow belongs to the class of 'ReasonArrows'. And 'listArrows' stands for a list of a number of individual arrows that belong to the class of 'ReasonArrows'. That's how I was conceptualizing all this.

Really think about this if you're arguing for the way it is.

When I look at an apple tree, I see "apples". That's because an apple tree is a thing that has many apples.

Now suppose I'm picking the fruit from this tree and putting it in the basket. I hold the basket so you can't see what's in it and ask you if you know what's in it. "Apples", I say, even though there's one inside. It still makes sense, because a basket can hold many things so the fact that it is temporarily holding one doesn't change that it holds "apples", plural. Even when there are zero, an "apple basket" holds "apples".

Now I pick an apple from a tree and say "this is apples". Does... that make sense? It's one apple. Singular. I sound silly. You don't say, "Pick me an apples", do you? Or "I have one apples in my right hand, and two apples in my left." No, when we talk about the object "apple", we use a singular when we're talking about one specific one and plurals if we have more than one.

So if "a reason arrow" is a singular thing, it is "ReasonArrow". If you have "a collection of 0 or more ReasonArrow objects", that can logically be called "ReasonArrows" even if it has 0 or 1 object inside.

It's very common to have this pattern:

Dim apples() As AppleBasket
...

For Each apple As Apple In apples
...

"For each apple", singular item. "In apples", a plural set of apples. Now imagine trying to write this with ReasonArrows as it sits:

Dim reasonArrows As ReasonArrows
...

For Each reasonArrow As ReasonArrows In reasonArrows

Uhh... if 'reasonArrow' is a ReasonArrows how is it also IN 'reasonArrows'? This is a situation you just want to avoid.

[dwigley]

I was explaining how I was thinking about coding using my intuitions as a native speaker of English. It would be a mistake, or even absurd, to try to argue that the VB language must capture English, because VB is a special formalized language. One can't expect VB to translate English. But VB follows natural language, of course, and we should take advantage of that whenever we can, so we need to use things like good names. One way to find out what the naming conventions are for VB is to consult the experts in the language, like the experts here on this VB Forum.

So the code for the class I made was this:

Public Class ReasonArrows
    Public Property StartPoint As Point
    Public Property EndPoint As Point
End Class

I wanted to make a "Class" in VB, and it's natural to think in English of a plural name of a class of objects. It's the class of arrows. But this goes against the naming convention in VB. From what experts have said here in this forum, and on the Microsoft page dealing with naming conventions, the name of a "class" should be a singular noun. I should have wrote:

Public Class ReasonArrow

My code for making an arrow that belongs to the class was this:

Private Arrow As New ReasonArrows

But this breaks VB convention because of the wrong class name. Instead:

Private Arrow As New ReasonArrow

My code for making a list was this:

Private listArrows As New List(Of ReasonArrows)

It should have been this:

Private listArrows As New List(Of ReasonArrow)

Spynne suggested eliminating the "list" part of the name, because the plural name itself would indicate it's a list. Then it would be:

Private Arrows As New List(Of ReasonArrow)

Then the "For Each" code would be:

For Each Arrow As ReasonArrow In Arrows

This would avoid the following "For Each" code that would result from my mistaken class name:

For Each Arrow As ReasonArrows In Arrows

I wish the rest of my coding problems were so simple!

[Sitten Spynne]

Names are one of the hardest things to get right. It's why I'm very picky about them.

True story:

I recently had to pick a name for a feature in our application. "Alert" would've been the right word. But we actually already have a feature that uses the word. "Message" would have been a good word. But we already have plans to use "Message" to mean something else entirely. "Notification" is right out, because on iOS and Android that has a very specific meaning to the system. We went back and forth for a long time about the name, and I was frustrated because I was busy prototyping and was already using 3 different names.

So I made up a new word, and that's the name of the feature. It's a secret.

[.paul.]

I was explaining how I was thinking about coding using my intuitions as a native speaker of English. It would be a mistake, or even absurd, to try to argue that the VB language must capture English, because VB is a special formalized language. One can't expect VB to translate English. But VB follows natural language, of course, and we should take advantage of that whenever we can, so we need to use things like good names. One way to find out what the naming conventions are for VB is to consult the experts in the language, like the experts here on this VB Forum.

So the code for the class I made was this:

Public Class ReasonArrows
    Public Property StartPoint As Point
    Public Property EndPoint As Point
End Class

I wanted to make a "Class" in VB, and it's natural to think in English of a plural name of a class of objects. It's the class of arrows. But this goes against the naming convention in VB. From what experts have said here in this forum, and on the Microsoft page dealing with naming conventions, the name of a "class" should be a singular noun. I should have wrote:

Public Class ReasonArrow

My code for making an arrow that belongs to the class was this:

Private Arrow As New ReasonArrows

But this breaks VB convention because of the wrong class name. Instead:

Private Arrow As New ReasonArrow

My code for making a list was this:

Private listArrows As New List(Of ReasonArrows)

It should have been this:

Private listArrows As New List(Of ReasonArrow)

Spynne suggested eliminating the "list" part of the name, because the plural name itself would indicate it's a list. Then it would be:

Private Arrows As New List(Of ReasonArrow)

Then the "For Each" code would be:

For Each Arrow As ReasonArrow In Arrows

This would avoid the following "For Each" code that would result from my mistaken class name:

For Each Arrow As ReasonArrows In Arrows

I wish the rest of my coding problems were so simple!

It's a step in the right direction. Learning and adopting good habits is never a bad thing, but most programmers i know aren't as obsessive as Sitten Spynne

[dwigley]

What!? You made up a new word? And it's a secret!? You just threw out the whole purpose of having conventions for good names! I'm sticking with my names!

I'm just joking.

So I'm guessing that your new name may be a combination of familiar words so people can have some understanding of what it is, like MessageAlert or NewsFlash, or something like that. But perhaps the new feature is so new that it can handle a new word. I can see that what names to use can depend on the particular context within which one is working and who will be reading the code. You might even have to make up an entirely new word.

[Sitten Spynne]

Well the problem is it turns out there's a furniture company with the same name, so it can't be the customer-facing name for the feature. But I just needed a dang word to use in the code that wouldn't get confused with umpteen different words. The people who were having the initial argument can keep having their fight over what they should call it in customer-facing documentation.

[dwigley]

Ah, context is everything. Keep it secret.