HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Are these good examples of functions that do one thing only or am I getting a little carried away?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
awaythesearegettingexamplesonethatlittlegoodcarried

Problem

I'm reading Robert C. Martin's "Clean Code" and for the past few days I've been fiddling with its function writing guidelines. At first I thought the idea of functions being two/three lines long to be ludicrous, but I'm slowly getting the hang of it. The problem is, I'm pretty sure I'm overdoing it now.

These are the first lines of the module I decided to refactor using the advice from the book.

Public Sub ExportAvailabilityData()
  Dim aplicativo As Excel.Application
  Set aplicativo = CreateObject("Excel.Application")
  aplicativo.Visible = True

  Dim pastaDeTrabalho As Excel.Workbook
  Set pastaDeTrabalho = aplicativo.Workbooks.Add

  Do Until pastaDeTrabalho.Sheets.Count = DaysInMonth(Month(Date)) + 1
    pastaDeTrabalho.Sheets.Add
  Loop

  Dim day As Integer
  For day = 1 To DaysInMonth(Month(Date))
    pastaDeTrabalho.Sheets(day).Name = Format(day, "00") & "." & Format(Month(Date), "00")
  Next day

  pastaDeTrabalho.Sheets(DaysInMonth(Month(Date)) + 1).Name = "Summary " & UCase(MonthName(Month(Date)))
End Sub

Private Function DaysInMonth(ByVal month As Integer) As Integer
  DaysInMonth = Day(DateSerial(Year(Date), month + 1, 1) - 1)
End Function


And this is how it looks like today.

```
Public Sub ExportAvailabilityData()
Dim aplicativo As Excel.Application
Set aplicativo = CreateObject("Excel.Application")
aplicativo.Visible = True

Dim pastaDeTrabalho As Excel.Workbook
Set pastaDeTrabalho = aplicativo.Workbooks.Add

CreateSheets pastaDeTrabalho
NameSheets pastaDeTrabalho
End Sub

Private Sub CreateSheets(ByRef pastaDeTrabalho As Excel.Workbook)
CreateDailySheets pastaDeTrabalho, Month(Date)
CreateSummarySheet pastaDeTrabalho
End Sub

Private Sub CreateDailySheets(ByRef pastaDeTrabalho As Excel.Workbook, ByVal month As Integer)
Do Until pastaDeTrabalho.Sheets.Count = DaysInMonth(month)
pastaDeTrabalho.Sheets.Add
Loop
End Sub

Private Sub CreateSummarySheet(ByRef pastaDeTrabalho As Excel.Workbook)
pastaDeTrabalho.Shee

Solution

I think you have definitely taken it in the right direction. Excellent work!

I don't see any compelling reason to break AssembleDailySheetName() down any further.

My only complaint is that I don't really like the separation of adding and naming the sheets. I see both steps as being part of creating a sheet. I think it would be nice to set the name of the sheet immediately after adding it (in the same function). That will likely eliminate some code as well. I suspect Sheets.Add may return the new sheet. If that's the case, there would be no need to look up the sheet by index to name it.

Context

StackExchange Code Review Q#26682, answer score: 5

Revisions (0)

No revisions yet.