2
\$\begingroup\$

This is in continuation from the project posted here: Making a report from payroll details part II

Basically, based on the user input in columns F, G, H, the macro generates a varying string in column I.

Eventually this spreadsheet is combined together in a mail merge in Microsoft Word (if anyone knows how to do that in excel please let me know). The goal is somehow to be able to rate the consumer: in terms of their progress that month, compared to other months, in specific categories of their success, and what the job coach is supposed to be doing.

It looks like this:

enter image description here

Option Explicit
Public Sub main()
    CheckSpreadsheet

End Sub
Private Sub CheckSpreadsheet()
'checks the spreadsheet
'based on the current row being analyzed, finds the previous row where their name occured
'sets the current row and the offset row

    Dim CheckName As String
    Dim CurrentRow As Long
    Dim OffsetRow As Long
    Dim j As Long
    Dim i As Long
    Dim IsThere As Boolean

    IsThere = False

    For i = 2 To Range("a1").End(xlDown).row
        CheckName = Cells(i, 1)
        CurrentRow = i

        If IsThere = False Then
            For j = i To 2 Step -1
                If Cells(j, 1) = CheckName Then
                    IsThere = True
                    OffsetRow = j
                End If
            Next j
        ElseIf IsThere = True Then
            BeginWriting CurrentRow, OffsetRow, CheckName
        Else
            MsgBox ("Some error 1")
        End If
    Next i
End Sub

Private Sub BeginWriting(CurrentRow As Long, OffsetRow As Long, ConsumerName As String)
'takes the current row, and the previous row which has the same string in column A
'starts putting together the output (eventually to take place in column 9)

    Dim output As String
    Dim cons As Consumer
    Dim check As Boolean
    Dim progress As Integer
    Dim skills As Dictionary
    Dim i As Long

    Dim v() As Variant

    Set cons = New Consumer
    Set skills = New Dictionary

    'initialize the consumer object
    cons.name = Cells(CurrentRow, 1)
    cons.SoftSkills = Cells(CurrentRow, 6)
    cons.JobSkills = Cells(CurrentRow, 7)
    cons.TimeSkills = Cells(CurrentRow, 8)

    'initialize output string
    output = cons.name & "'s service delivery employment plan for " & Cells(CurrentRow, 2) & " " & Cells(CurrentRow, 3) & " included implementation with soft skills, job skills, and time skills. "

    'find how consumer did this month; values associated with original row
    For i = 0 To 2
        skills(i) = Cells(CurrentRow, 6 + i)
        output = output & findSkill(i + 1, skills(i), cons.name)
    Next i

    'resets the current dictionary
    skills.RemoveAll

    'moving into progress stage...
    output = output & " In terms of progress this month,"

    'finds progress, based on a comparison between values in column 6,7,8 on the original row (this month) and offset row (last month)
    For i = 0 To 2
        progress = Cells(CurrentRow, 6 + i) - Cells(OffsetRow, 6 + i)
        skills.Add Cells(1, 6 + i), progress
        ConsumerName = skills.Keys()(i)
        output = output & FindProgress(ConsumerName, progress, cons.name)
    Next i

    progress = 0
    For i = 0 To 2
        progress = progress + (Cells(CurrentRow, 6 + i) - Cells(OffsetRow, 6 + i))
    Next i

    output = output & findIssues(skills, progress, cons.name)

    Cells(CurrentRow, 9) = output

End Sub
Public Function Analyze(skills As Dictionary, ConsumerName As String) As String
'performed some analysis

    Dim output As String
    Dim overall As Integer
    Dim individualProgress As String
    Dim individualTreatment As String
    Dim key As Long


    For Each key In skills
        overall = overall + skills(key)
    Next key

    'overall progress
    If overall > 0 Then
        output = " Overall, " & ConsumerName & " improved this month noticeably."
    ElseIf overall < 0 Then
        output = " Overall, " & ConsumerName & " declined this month noticebly."
    Else
        output = " Overall, there was no observable change in " & ConsumerName & " this month."
    End If

    'rate them each month
    For Each key In skills
        individualProgress = Switch(key = "Soft Skills", findSkill(1, skills(key), ConsumerName), key = "Job Skills", findSkill(2, skills(key), ConsumerName), key = "Time Skills", findSkill(3, skills(key), ConsumerName))
        output = output & individualProgress
    Next key

    'treatment for each month
    For Each key In skills
        individualTreatment = Switch(key = "Soft Skills", findSkill(4, 0, ConsumerName), key = "Job Skills", findSkill(5, 0, ConsumerName), key = "Time Skills", findSkill(6, 0, ConsumerName))
        output = output & individualTreatment
    Next key

''    'find issues
    For Each key In skills
        individualTreatment = Switch(key = "Soft Skills", findSkill(7, 0, ConsumerName), key = "Job Skills", findSkill(8, 0, ConsumerName), key = "Time Skills", findSkill(9, 0, ConsumerName))
        output = output & individualTreatment
    Next key

    If overall <> 0 Then
        output = output & findIssues(skills, overall, ConsumerName)
    ElseIf overall = 0 Then
        output = " There was no over"
    End If


    Analyze = output
End Function
Public Function findSkill(index As Integer, SkillLevel As Integer, ConsumerName As String) As String
'creates a series of strings which can be matched to values in columns F,G,H

        Dim SoftSkills(0 To 6), JobSkills(0 To 6), TimeSkills(0 To 6) As String
        Dim jobTreat(0 To 4), softTreat(0 To 2), timeTreat(0 To 2) As String
        Dim tempInt As Long


    If index = 1 Then
        SoftSkills(0) = ConsumerName & " did not interact at all, as required, with other people at the worksite. "
        SoftSkills(1) = ConsumerName & " interacted with other people, as required, minimally. "
        SoftSkills(2) = ConsumerName & " interacted with other people, as required, at a low level. "
        SoftSkills(3) = ConsumerName & " interacted with other people, as required, at a medium level. "
        SoftSkills(4) = ConsumerName & " interacted with other people, as required, at a high level. "
        SoftSkills(5) = ConsumerName & " interacted with other people, as required, in full competence. "
        SoftSkills(6) = ConsumerName & " mastered the soft skills required for the job. "
        findSkill = SoftSkills(SkillLevel)
    ElseIf index = 2 Then
        JobSkills(0) = ConsumerName & " did not do the job, as required, at all. "
        JobSkills(1) = ConsumerName & " did the job, as required, with minimal effectiveness. "
        JobSkills(2) = ConsumerName & " did the job, as required, with low effectiveness. "
        JobSkills(3) = ConsumerName & " did the job, as required, with medium effectiveness. "
        JobSkills(4) = ConsumerName & " did the job, as required, with high effectiveness. "
        JobSkills(5) = ConsumerName & " did the job, as required, with full effectiveness. "
        JobSkills(6) = ConsumerName & " mastered the skills required for the job. "
        findSkill = JobSkills(SkillLevel)
    ElseIf index = 3 Then
        TimeSkills(0) = ConsumerName & " did not arrive to or leave from work, as required. "
        TimeSkills(1) = ConsumerName & " arrived to or left from work with minimal consistency. "
        TimeSkills(2) = ConsumerName & " arrived to or left from work with full consistency. "
        TimeSkills(3) = ConsumerName & " arrived to or left from work with medium consistency. "
        TimeSkills(4) = ConsumerName & " arrived to or left from work with high consistency. "
        TimeSkills(5) = ConsumerName & " arrived to or left from work with full consistency. "
        TimeSkills(6) = ConsumerName & " mastered the time skills required for the job. "
        findSkill = TimeSkills(SkillLevel)
    ElseIf index = 4 Then
        jobTreat(0) = ("I worked to treat issues with job skills through assisting  " & ConsumerName & " with work technique, work approach, as well as work stamina. ")
        jobTreat(1) = ("I worked to treat issues with job skills through assisting  " & ConsumerName & " with work speed and prioritization of tasks. ")
        jobTreat(2) = ("I worked to treat issues with job skills through assisting  " & ConsumerName & " with the appropriate use of amount of materials for the tasks. ")
        jobTreat(3) = ("I worked to treat issues with job skills through assisting  " & ConsumerName & " with working within the team. ")
        jobTreat(4) = ("I worked to treat issues with job skills through assisting with " & ConsumerName & "'s relationship with a supervisor. ")
        tempInt = Round((4 * Rnd()), 0)
        findSkill = jobTreat(tempInt)
    ElseIf index = 5 Then
        softTreat(0) = ("I worked to treat issues with soft skills through assisting " & ConsumerName & " with remaining calm and working through problems with others. ")
        softTreat(1) = ("I worked to treat issues with soft skills through assisting " & ConsumerName & " with apperance to members of the community. ")
        softTreat(2) = ("I worked to treat issues with soft skills through assisting " & ConsumerName & " with speaking well with others. ")
        tempInt = Round((2 * Rnd()), 0)
        findSkill = softTreat(tempInt)
    ElseIf index = 6 Then
        timeTreat(0) = ("I worked to treat issues with time skills through assisting " & ConsumerName & " had access to and knew how to use scheduling technolgy (calendar, phone number, etc). ")
        timeTreat(1) = ("I worked to treat issues with time skills through waiting for " & ConsumerName & " at the bus/train stop while arriving and/or leaving. ")
        timeTreat(2) = ("I worked to treat issues with time skills through reminding " & ConsumerName & " about the work schedule as written. ")
        tempInt = Round((2 * Rnd()), 0)
        findSkill = timeTreat(tempInt)
    ElseIf index = 7 Then
         findSkill = "The issues were mostly related to  " & ConsumerName & "'s ability to successfully complete the work efficiently and on time. Going forward, I will need to pay closer attention to  " & ConsumerName & "'s ability to maintain a work rhythm through their shift. "
    ElseIf index = 8 Then
         findSkill = "The issues were mostly related to  " & ConsumerName & "'s ability to successfully complete the work without being distracted by other co-workers or residents of the community. There were some incidents where  " & ConsumerName & " did not optimally get along with other co-workers or community residents this month. Going forward, I will do my best to ensure that  " & ConsumerName & " has a work environment clear of potential quarrelsome colleagues or residents of the community. "
    ElseIf index = 9 Then
          findSkill = "The issues were mostly related to his conversational ability with me as his job coach.  " & ConsumerName & " is not very communicative with the I, which makes supervision harder to do. This requires me to build better ways to communicate with  " & ConsumerName & ", which will be primary challenge over the next month. "


    End If
End Function
Public Function FindProgress(key As String, ProgressLevel As Integer, ConsumerName As String) As String
    Dim prog As String


    If ProgressLevel > 0 Then
        If key = "Soft Skills" Then
            prog = " there was an noticable improvement this month in soft skills. " & ConsumerName & " was better at interacting with others at the work site and treating people respectfully than in previous months."
        ElseIf key = "Job Skills" Then
            prog = " There was an noticable improvement this month in job skills. " & ConsumerName & " was better at the job tasks - sweeping, cleaning, and collecting garbage or refuse - than in previous months."
        ElseIf key = "Time Skills" Then
            prog = " There was an noticable improvement this month in time skills." & ConsumerName & " was better with time management this month, and more able to be trusted with arriving/leaving on time."
        Else
            MsgBox ("some error in finding progress")
        End If
    ElseIf ProgressLevel < 0 Then
        If key = "Soft Skills" Then
            prog = " there was an noticable decline this month in soft skills. " & ConsumerName & " was worse with others at the worksite than in previous months."
        ElseIf key = "Job Skills" Then
            prog = "There was an noticable decline this month in job skills. " & ConsumerName & " was worse at the job tasks - sweeping, cleaning, and collecting garbage or refuse - than in previous months."
        ElseIf key = "Time Skills" Then
            prog = " There was an noticable decline this month in time skills." & ConsumerName & " was worse with time management - less able to be trusted with arriving/leaving on time - than in previous months."
        Else
            MsgBox ("some error in finding decline")
        End If
    Else
        prog = "There was no noticable change at all in " & ConsumerName & " with " & key & " this month. "
    End If

    FindProgress = prog

End Function

Public Function findIssues(skills As Dictionary, overall As Integer, ConsumerName As String) As String
'finds the issues with the consumer for the month
'takes the value in each column for the consumer for the month, and finds out which is the greatest cause of the change
'also compares it to overall change
    Dim issues As String
    Dim individualIssue As String
    Dim key As Variant

    'first report on any overall issues
    If overall > 0 Then
        issues = "Overall, because " & ConsumerName & " progressed this month, there were not issues to report, nor concerns about " & ConsumerName & " to do the work that is asked of him, nor were there any schedule issues to be concerned about regarding  " & ConsumerName & " for this month. "
        For Each key In skills

         If skills(key) / overall < 0.33 Then
            individualIssue = Switch(key = "Soft Skills", findSkill(7, 0, ConsumerName), key = "Job Skills", findSkill(8, 0, ConsumerName), key = "Time Skills", findSkill(9, 0, ConsumerName))
            issues = issues & individualIssue

         End If
         Next
    ElseIf overall < 0 Then
        issues = "Overall, because " & ConsumerName & " declined this month, there were issues to report on " & ConsumerName & ". "
        For Each key In skills
         If skills(key) / overall < 0.33 Then
            individualIssue = Switch(key = "Soft Skills", findSkill(7, 0, ConsumerName), key = "Job Skills", findSkill(8, 0, ConsumerName), key = "Time Skills", findSkill(9, 0, ConsumerName))
            issues = issues & individualIssue

         End If

    Next
    ElseIf overall = 0 Then
        issues = "Overall, there was no observable change in " & ConsumerName & " this month regarding the categories we rate him on. "
    Else
    End If
    findIssues = issues

End Function

Consumer Object

    Private cName As String
    Private output As String
    Private cProgress As String
    Private cJobSkills As String
    Private cTimeSkills As String
    Private cSoftSkills As String

Public Property Get name() As String
    name = cName
End Property
Public Property Let name(Value As String)
    cName = Value
End Property


Public Property Get SoftSkills() As String
    SoftSkills = cSoftSkills
End Property
Public Property Let SoftSkills(Value As String)
    cSoftSkills = Value
End Property
Public Property Get JobSkills() As String
    JobSkills = cJobSkills
End Property
Public Property Let JobSkills(Value As String)
    cJobSkills = Value
End Property
Public Property Get TimeSkills() As String
    TimeSkills = cTimeSkills
End Property
Public Property Let TimeSkills(Value As String)
    cTimeSkills = Value
End Property
Public Property Get progress() As String
    progress = cProgress
End Property
Public Property Let progress(Value As String)
    cProgress = Value
End Property

Skills Object

Private cSkillName As String

Private cSkillValue As Integer
Public Property Get SkillName() As String
    SkillName = cSkillName
End Property
Public Property Let SkillName(Value As String)
    cSkillName = Value
End Property
Public Property Get SkillValue() As Integer
    SkillValue = cSkillValue
End Property
Public Property Let SkillValue(Value As Integer)
    cSkillValue = Value
End Property
\$\endgroup\$
3
\$\begingroup\$

You've done a good job encapsulating private variables with properties there, and that's awesome - remember that Option Explicit only applies to the module it's specified in though, so it should be in all modules - even class modules!

There should be more vertical whitespace between module members - at least one empty line. The VBE doesn't care, but the human eye does. Compare this:

Private cSkillName As String

Private cSkillValue As Integer
Public Property Get SkillName() As String
    SkillName = cSkillName
End Property
Public Property Let SkillName(Value As String)
    cSkillName = Value
End Property
Public Property Get SkillValue() As Integer
    SkillValue = cSkillValue
End Property
Public Property Let SkillValue(Value As Integer)
    cSkillValue = Value
End Property

To this:

Option Explicit

Private cSkillName As String
Private cSkillValue As Integer

Public Property Get SkillName() As String
    SkillName = cSkillName
End Property

Public Property Let SkillName(Value As String)
    cSkillName = Value
End Property

Public Property Get SkillValue() As Integer
    SkillValue = cSkillValue
End Property

Public Property Let SkillValue(Value As Integer)
    cSkillValue = Value
End Property

Anyway, that's just a minor point. It seems that Skill class is really nothing more than a KeyValuePair, which the Dictionary handles already - in fact I can't seem to find a single place where you actually use that Skill class; looks like the class module can be removed from the project.

Naming is hard. Naming in a case-insensitive language such as VBA is even harder! Supposing the class is actually needed somewhere, this is how I would have implemented it:

Option Explicit
Public Enum SkillRating
    NotApplicable
    Minimal
    Low
    Medium
    High
    Full
End Enum

Private Type TSkill
    Name As String
    Rating As SkillRating
End Type

Private this As TSkill

Public Property Get Name() As String
    Name = this.Name
End Property

Public Property Let Name(ByVal value As String)
    this.Name = value
End Property

Public Property Get Rating() As SkillRating
    Rating = this.Rating
End Property

Public Property Let Rating(ByVal value As SkillRating)
    this.Rating = value
End Property

The Public Enum SkillRating defines named constants for values 0-6; its mere presence documents the meaning of the rating values being manipulated.

The Private Type TRating defines a structure that has the same members as the class' public members - that way I only ever have one private field in a class module (which I name this by my own convention), and because the members are scoped to a private type I don't need to use any obscure prefixes; this.Name makes it crystal-clear that I'm referring to the encapsulated Name value.

I love that you're consistently naming the Property Let parameter value. Note that by not specifying how you're passing the parameter, VBA defaults to ByRef, which isn't necessary - you should be passing these parameters ByVal.

Technicality: an object is an instance of a class module; it only ever exists at runtime. It's incorrect to say Consumer or Skills are objects when you're referring to the class module itself.

You're not using the Consumer class much either: the only Consumer object ever instantiated is in the BeginWriting procedure, and it's only ever used to hold a bunch of related values: you never pass the reference around to another method - you don't really need that class at all.

Regrouping the related values under a type was a good decision though. But this would have been sufficient:

Public Type TConsumer
    Name As String
    Output As String
    Progress As String
    JobSkills As SkillRating
    TimeSkills As SkillRating
    SoftSkills As SkillRating
End Type

So instead of Set cons = New Consumer and you could do Dim consumer As TConsumer and use it directly, exactly the way you have it already.


Your code is mixing up two very different things: code, and data. All these strings shouldn't be hard-coded in a procedure.

You could have a hidden worksheet that contains a ListObject/table for each set of strings you need. For example, tblJobSkills could look like this:

an Excel table containing the JobSkills strings for each rating value

It seems rather inefficient to populate all values when all you need is one:

JobSkills(0) = ConsumerName & " did not do the job, as required, at all. "
JobSkills(1) = ConsumerName & " did the job, as required, with minimal effectiveness. "
JobSkills(2) = ConsumerName & " did the job, as required, with low effectiveness. "
JobSkills(3) = ConsumerName & " did the job, as required, with medium effectiveness. "
JobSkills(4) = ConsumerName & " did the job, as required, with high effectiveness. "
JobSkills(5) = ConsumerName & " did the job, as required, with full effectiveness. "
JobSkills(6) = ConsumerName & " mastered the skills required for the job. "
findSkill = JobSkills(SkillLevel)

Assuming there's a hidden worksheet named Data with a tblJobSkills table, findSkill could have been doing this instead:

Dim result As String
result = Data.ListObjects("tblJobSkills").ListRows(SkillLevel + 1).Range(ColumnIndex:=2)
findSkill = Replace(result, "%NAME%, ConsumerName)

This approach also has the advantage of being easily localizable (just add another Value column to the data, to hold the translated strings!), if requirements ever come to that - in actual code, only the ColumnIndex:=2 would need to change then.

I won't be refactoring your whole code, but that should give you an idea of how I would implement something like it.

\$\endgroup\$
  • \$\begingroup\$ Hi, Firstly, thank you for your comments. I am seriously trusting stackoverflow with my code. The reason I made the skills class is because the name of the skill (Job Skills, Time Skills, Soft Skills) is the key while the number associated with each skill is the value; as I understood dictionaries, the key needs to be a number while the value is variant. As far as enum rather than making an object, would this be its own class module? If not, how would I actually go about declaring it and referencing it? \$\endgroup\$ – bdpolinsky Aug 30 '16 at 19:28
  • \$\begingroup\$ @bdpolinsky where are you using the class though? It seems the class is defined, yes, ..but never used. \$\endgroup\$ – Mathieu Guindon Aug 30 '16 at 19:34
  • \$\begingroup\$ For the skills class, I use it in FindSkill and FindIssue, mostly to cycle through. Basically I need a way to temporarily store which skill is associated with which value; specifically the change from month to month in the value. So for instance if in April hara gets a 3 for time skills, and in may a 4 for time skills, the 'skills' object will have Key: Time Skills Value: 1 (this is for the +1 change from april to may) So then I can easily say "hara did X in May for time skills. This was a +1 improvement in time skills." \$\endgroup\$ – bdpolinsky Aug 30 '16 at 19:43
  • 2
    \$\begingroup\$ Good call....skills is a dictionary. \$\endgroup\$ – bdpolinsky Aug 30 '16 at 19:48
  • 1
    \$\begingroup\$ @bdpolinsky - Dictionary has a Variant key and a Variant value - that's why you need a Variant to iterate over its keys in a For Each loop. The only thing the key requires is a unique hash. \$\endgroup\$ – Comintern Aug 31 '16 at 1:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.