[TheAKB]

in the code below and sepcifically this line, i have been told that i am doing more than is needed i.e wrriten extra code when not needed cany anyone advise

Specific Line :

If (_pen.ClickedState And _pen.Clickable) Or _pen.Clickable = False Then 

whole sub routine

Protected Sub bttnWrite_Click(ByVal sender As Object, ByVal e As EventArgs) Handles bttnWrite.Click
        LoadPen()
        If (_pen.ClickedState And _pen.Clickable) Or _pen.Clickable = False Then
            _pen.Write(txtWords.Text.Count)
            lblPad.Text = lblPad.Text + " " + txtWords.Text
            lblPad.ForeColor = _pen.InkColour
            txtWords.Text = String.Empty
            SavePen()
            UpdateInkUsedLabel()
        Else
            MsgBox("Your Pen is not Clicked")
        End If
    End Sub

This post has been edited by TheAKB: 16 October 2012 - 05:05 AM

[lucky3]

It's hard (or impossible) to just guess what _pen object is, and how are it's ClickedState and Clickable properties handled. If setting one affects the other, and they are both simple boolean types, then there might be no need for such if statement. Can you post the Class for _pen object?

[TheAKB]

lucky3, on 16 October 2012 - 07:13 AM, said:

It's hard (or impossible) to just guess what _pen object is, and how are it's ClickedState and Clickable properties handled. If setting one affects the other, and they are both simple boolean types, then there might be no need for such if statement. Can you post the Class for _pen object?

Option Strict On '
Option Explicit On 'explicitly declare using Dim or Redim


Public Class Pen
    Private Const COLOUR_RED As String = "Red"
    Private Const COLOUR_BLUE As String = "Blue"
    Private Const COLOUR_GREEN As String = "Green"
    Private Const COLOUR_BLACK As String = "Black"
    Public Sub New()
        InkCapacity = 1000
        MaxFlow = 4
        Flow = 0
    End Sub

    Public Sub New(ByVal vMaterial As String, ByVal vInkColour As Drawing.Color, _
                   ByVal vInkCapacity As Double, ByVal vNibType As String, _
                   ByVal vLogo As String, ByVal vClickable As Boolean, _
                   ByVal vFlow As Int32)
        InkCapacity = vInkCapacity
        Material = vMaterial
        InkColour = vInkColour
        NibType = vNibType
        Logo = vLogo
        Clickable = vClickable
        Flow = vFlow
        MaxFlow = 4
    End Sub

    Public ClickedState As Boolean
#Region "Public Properties"
    ''' <summary>
    ''' declares of properties for public, private and functions
    ''' </summary>
    ''' <remarks></remarks>
    Private _material As String
    Public Property Material() As String
        Get
            Return _material
        End Get
        Set(ByVal value As String)
            _material = value
        End Set
    End Property

    Private _inkColour As Drawing.Color
    Public Property InkColour() As Drawing.Color
        Get
            Return _inkColour
        End Get
        Set(ByVal value As Drawing.Color)
            _inkColour = value
        End Set
    End Property

    Private _inkCapacity As Double

    Public Property InkCapacity() As Double
        Get
            Return _inkCapacity
        End Get
        Set(ByVal value As Double)
            _inkCapacity = value
        End Set
    End Property

    Private _nibType As String
    Public Property NibType() As String
        Get
            Return _nibType
        End Get
        Set(ByVal value As String)
            _nibType = value
        End Set
    End Property

    Private _logo As String
    Public Property Logo() As String
        Get
            Return _logo
        End Get
        Set(ByVal value As String)
            _logo = value
        End Set
    End Property


    Private _clickable As Boolean
    Public Property Clickable() As Boolean
        Get
            Return _clickable
        End Get
        Set(ByVal value As Boolean)
            _clickable = value
        End Set
    End Property



#End Region

    Private _inkused As Double
    Public ReadOnly Property InkUsed() As Double
        Get
            Return _inkused
        End Get

    End Property

#Region "Private Properties"


    Private _flow As Int32
    Protected Property Flow() As Int32
        Get
            Return _flow
        End Get
        Set(ByVal value As Int32)
            _flow = value
        End Set
    End Property

    Private _maxflow As Int32
    Protected Property MaxFlow() As Int32
        Get
            Return _maxflow
        End Get
        Set(ByVal value As Int32)
            _maxflow = value
        End Set
    End Property



#End Region

#Region "Functions"
    ''' <summary>
    ''' Write Function
    ''' maxflow * rate of flow * number of letters gives the amount of ink used. 
    ''' inkused/inkcapacity gives us the ratio for the remainging capity, * 100 gives us the percent.
    ''' </summary>
    ''' <param name="NumberOfLetters"></param>
    ''' <returns></returns>
    ''' <remarks></remarks>
    Public Function Write(ByVal NumberOfLetters As Int32) As Double
        Dim result As Double = 0.0



        _inkused = _inkused + (_maxflow * _flow * NumberOfLetters / 100)
        result = 100 * _inkused / _inkCapacity

        If _inkused > 100 Then
            _inkused = 100
        End If

        Return result
    End Function

    Public Sub ChangeColour()
        Select Case InkColour
            Case Drawing.Color.Red
                InkColour = Drawing.Color.Blue
            Case Drawing.Color.Blue
                InkColour = Drawing.Color.Green
            Case Drawing.Color.Green
                InkColour = Drawing.Color.Black
            Case Drawing.Color.Black
                InkColour = Drawing.Color.Red
        End Select
    End Sub

    Public Overridable Sub Click()
        If Clickable Then
            If ClickedState Then
                ClickedState = False
            Else
                ClickedState = True
            End If
        End If
    End Sub
#End Region





End Class

[lucky3]

Let me first say that it's strange design, when you can access and set ClickedState from outside of the class, while it is changed with Public Click() function, but only if Clickableis TRUE. When you instantiate Pen class, it has all Boolean values set to FALSE, unless you use constructor with vClickable parameter, and set it to TRUE, or do it "inline" with something like Dim _pen As New Pen With {.Clickable = True}.

Probably you wish to allow writing only if .ClickedState is TRUE only, no matter if _pen is Clickable. In that case only If _pen.ClickedState Then is needed. If you wish to check also for .Clickable, and allow writing if both are TRUE, then do it: If _pen.ClickedState And _pen.Clickable Then.

It really depends on which case you wish to allow _pen.Write the text.