Making a report from payroll details

*** PS_HOURS.3_1
July |1 |2 |3 |4 |5 |6 |7
Name |Total|Total|Total|Total|Total|Total|Total
Abram, John |1.00 | | | |3.00 |3.00 |3.00
Agara, Hara |2.00 | | | |2.00 |2.00 |3.00
Alsi, Idria |2.00 | | | |2.00 |2.00 |2.00
Amon, Char |2.00 | | | |3.00 |3.00 |2.00
Base, Adron |3.00 | | | |3.00 |2.00 |3.00


Screenshot:

Data is entered into this spreadsheet manually:

• Job coaches write down a clients start time/stop time on a piece of paper

• This paper is then brought to the person who works in our central office, and she takes all the pieces of paper (probably about 8-9 pieces of paper for a week and makes a hand written master sheet which looks like above (minus the quarter hours), but is 55 rows deep and 31 columns wide. If for instance Hara Agara works on July 13th then that box is filled in with the amount of hours he worked for that day.

• I input her master sheet into this spreadsheet. I could easily have her do it, but at least the theory now is that the agency still needs to maintain paper copies.

I have the following functional macro based on the above. There's a list of 55 names, and a record of the hours they've worked for every day of the month (31 total columns) starting from cell C going to cell BK (skipping by 2's); cell D going to cell BJ (also skipping by 2's) is a record of the total amount of hours they've worked translated into quarter hour units (i.e. 2 hours worked = 8 hour quarter units). I use this for reporting purposes.

At the far right the total amount of hours a person worked is summed up, and is then reported on a summary page. Each month is given it's own worksheet - IE there's a record for June, July, August, with the name of the worksheet being its NameOfMonth.year.

This data is used to generate the following report:

`Report for John Abram

a b c d e f g h i j

There's a lot to chew on here, so I'll grab some low hanging fruit to get things rolling.

Declarations

Multiple declarations on the same line need to have the type specified for each variable, not just the last one. This will declare mName as an Integer, but the rest are actually being declared as the fall-back Variant: Dim dValue, mIndex, mName As Integer. If you declare them on the same line and want all Integer's, you need to declare them like this:

Dim dValue As Integer, mIndex As Integer, mName As Integer

Better would be to declare them on separate lines, and right before they are used for the first time. Not only does it help you easily identify unused and undeclared variables (see below), it makes the code more maintainable by grouping everything that might be related to a code change in roughly the same place (again see below). Finally (for the purpose of this discussion), it avoids the need to scroll or even glance to the top of the procedure to see how or if a variable is declared. Doing so makes it like reading something with a ton of footnotes (headnotes?) - it interrupts the normal top down reading of the procedure. Computers are good a remembering what a ton of variable are - humans much less so.

Also, you should add Option Explicit to the top of your modules. This would point out the fact that mValue is never declared in Sub copy() (and mIndex is never used), date_next_month, last_day_month, nb_days, i, and j are not declared in Sub updateDates(), etc., etc. This can avoid a host of hard to track down bugs as well as adding the processing overhead that comes from having them implicitly created as Variant's.

Dates

Parsing dates with code like dValue = Int(Right(Cells(7, 1), 2)) in Sub copy() is unnecessary - you can just use the build in Month() and Day() functions...

dValue = Day(CDate(target.Cells(7, 1)))
mValue = Month(CDate(target.Cells(7, 1)))

...although just grabbing the Date itself would be the direction I'd go.

You do the same thing in Sub updateDates() with n_mValue = Int(Left(Cells(2, 26), 2)) + 1, although in that routine you can skip a bunch of the date handling. The entire first half of the Sub basically just gets you a limit to make sure that your week stays within the month. You can actually just test for the month directly in the loop and skip all of the messing around with the date. If I'm reading it correctly, you can distill the whole thing down to this:

For i = 0 To 6
    dValue = CDate(Cells(7 + i, 1)) 
    If Month(dValue) = Month(dValue + 7) Then
        Cells(7 + i, 1) = dValue + 7
    Else
        Range(Cells(7 + i, 1), Cells(7 + i, 32)).ClearContents
    End If
Next i

Note that you don't have to use the loop to wipe individual cells, and that I removed the Format call. You should generally avoid treating dates as strings except when they are going to be displayed to the user. And... Excel has a perfectly good way to do that - just set the cell format to the appropriate type and let Excel do the heavy lifting.

In Sub newMonth(mValue) you manually determine the name of the month like this (twice):

mName1 = Switch(mValue = 1, "January", mValue = 2, "February", ...

You can just use the built in MonthName() function: mName1 = MonthName(mValue).

Refactoring opportunities

Note the case on Selection.copy in Sub copy(). You basically get one casing for each declaration everywhere it appears in your code, so you should try to avoid collisions. This would be a great place for a more descriptive name for the Sub - something to the effect of Public Sub CopyAndFormatSheet will let Selection have its .Copy back.

Sections like this:

Columns("A:A").ColumnWidth = 6
Columns("B:B").ColumnWidth = 8
Columns("C:C").ColumnWidth = 10

...can be converted into loops so you don't have a huge wall of statements. I'd probably extract the row and column sizing to its own Sub - something like this. Note that I sized the missing rows 6, 15, and 16 - you could easily just skip them in the loop or use some other similar method, but this should at least give the gist:

Private Sub SizeRowsAndColumns(sheet As Worksheet)
    Dim widths As Variant
    widths = Array(6, 8, 10, 10, 8, 8, 6, 6, 6, 10, 10, 10, 1, 1, 5, 5, 5, _
                   5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5)

    Dim i As Long
    For i = LBound(widths) To UBound(widths)
        sheet.Columns(i + 1).ColumnWidth = widths(i)
    Next

    For i = 5 To 19
        If i = 5 Then
            sheet.Rows(i).RowHeight = 55
        ElseIf i = 14 Then
            sheet.Rows(i).RowHeight = 70
        Else
            sheet.Rows(i).RowHeight = 50
        End If
    Next
End Sub

I'd also extract a Sub to do your page setup - you generally want routines that do one thing, and one thing only. I'd also remove all of the extraneous enabling and disabling of Application.PrintCommunication - you shoul

StackExchange Code Review Q#139563, answer score: 3