-1
\$\begingroup\$

As an old dog (age 73) learning new (Excel VBA) tricks, I am reasonably happy with putting together the code below. But I think it could be cleaner. How would you have coded it?

Private Sub Workbook_Open()  
    Dim lastRow As Long     'last row with data  
    Dim thisDate As Double  'start timestamp  
    thisDate = Now()  
With Sheets("Pressure Log")  
    lastRow = .Range("B" & .Rows.Count).End(xlUp).Row 'populate next row with date/time  
    .Range("B" & lastRow + 1 & ":G" & lastRow + 1).Borders.LineStyle = xlContinuous  
    .Range("B" & lastRow).Offset(1) = Format(thisDate, "dddd")  
    .Range("B" & lastRow).Offset(1, 1) = Format(thisDate, "mm/dd/yyyy")  
    .Range("B" & lastRow).Offset(1, 2) = Format(thisDate, "hh:mm AM/PM")  
    .Range("B" & lastRow).Offset(1, 3).Select 'position for data  
End With  
End Sub
\$\endgroup\$

closed as off-topic by πάντα ῥεῖ, Sᴀᴍ Onᴇᴌᴀ, Hosch250, alecxe, Toby Speight Dec 12 '18 at 8:43

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – πάντα ῥεῖ, Sᴀᴍ Onᴇᴌᴀ, Hosch250, alecxe
If this question can be reworded to fit the rules in the help center, please edit the question.

1
\$\begingroup\$

Properly formatting and indenting code is always a good start.

Using Option Explicit at the top of every module is a must. You may already do this, just thought I would mention it.

You declare thisDate as a Double but you use it as a Date. Declare it as a Date.

Make your life a little easier and set a range to the start of your new row instead of calling a calculated range. Example below:

Private Sub Workbook_Open()  
    Dim lastRow As Long     'last row with data  
    Dim thisDate As Date  'start timestamp  
    Dim entryRange as Range
    thisDate = Now()  
    With Sheets("Pressure Log")  
        lastRow = .Range("B" & .Rows.Count).End(xlUp).Row 'populate next row with date/time  
        Set entryRange = .Range("B" & lastRow+1) ` There are other ways of doing this too.   
    End With  
    entryRange.resize(1, 6).Borders.LineStyle = xlContinuous  ' Yes, could do this in a With block as well.
    entryRange.Value = Format(thisDate, "dddd")  
    entryRange.Offset(, 1).Value = Format(thisDate, "mm/dd/yyyy")  
    entryRange.Offset(, 2) = Format(thisDate, "hh:mm AM/PM")  
    entryRange.Offset(, 3).Select 'position for data  
End Sub
\$\endgroup\$
  • \$\begingroup\$ thank you AJD "Properly formatting and indenting code is always a good start ." Agreed. My script code is indented but I have trouble with this forum's code block. \$\endgroup\$ – grNadpa Dec 11 '18 at 21:51
  • \$\begingroup\$ thank you @AJD "Properly formatting and indenting code is always a good start ." Agreed. My script code is indented but I have trouble with this forum's code block. (as is trying to line break in this comment) "Using Option Explicit at the top of every module is a must. " I understood the concept, but thought that Option Explicit was once for the entire workbook so I did not know where to put it. Now I know. Thank you. "You declare thisDate as a Double but you use it as a Date. Declare it as a Date." I thought Date was a method, not a type as well. Will make the change \$\endgroup\$ – grNadpa Dec 11 '18 at 22:00
  • \$\begingroup\$ " Set entryRange = .Range("B" & lastRow+1) ` There are other ways of doing this too." I gather you recommend this as the better alternative. What might be some of the other ways? \$\endgroup\$ – grNadpa Dec 11 '18 at 22:03
  • \$\begingroup\$ @grNadpa: Set entryRange = .Range("B" & lastRow).Offset(1,0) is an easy alternative. You could also use .Cells(lastRow+1,2) (not preferable because Cells is relative to a range and which cells are selected can be confused if you change the base range). In some cases, just comes down to personal preferences. \$\endgroup\$ – AJD Dec 12 '18 at 6:20
0
\$\begingroup\$

Another With statement would make the code easier to read.

With Sheets("Pressure Log")
    lastRow = .Range("B" & .Rows.Count).End(xlUp).Row    'populate next row with date/time
    .Range("B" & lastRow + 1 & ":G" & lastRow + 1).Borders.LineStyle = xlContinuous
    With .Range("B" & lastRow)
        .Offset(1) = Format(thisDate, "dddd")
        .Offset(1, 1) = Format(thisDate, "mm/dd/yyyy")
        .Offset(1, 2) = Format(thisDate, "hh:mm AM/PM")
        .Offset(1, 3).Select                      'position for data
    End With
End With

It is better to format entire columns and rows. These reduces the file size by combining multiple css rules.

There are many way to skin this cat. For variety I set a range variable ( newRow ) to the next empty row. This allows me to uses ranges relative to the new row.

Dim newRow As Range
With Worksheets("Pressure Log")
    Set newRow = .Range("B" & .Rows.Count).End(xlUp).Offset(1).EntireRow
    newRow.Range("B1:G1").Borders.LineStyle = xlContinuous
    newRow.Range("B1:D1").Value = Now
    .Columns("C").NumberFormat = "ddd"
    .Columns("D").NumberFormat = "mm/dd/yyyy"
    .Columns("E").NumberFormat = "hh:mm AM/PM"
    .Columns("C:G").AutoFit
    newRow.Columns("E").Select
End With

With this setup we can copy all the formats from the row above except NumberFormats by using Range().FillDown.

 newRow.FillDown

Alternately, we could copy all the formats from the row above like this:

newRow.Offset(-1).Copy
newRow.PasteSpecial Paste:=xlPasteFormats
Application.CutCopyMode = False

Your code is pretty solid but if you truly want to learn Excel VBA I would start by watching this series: Excel VBA Introduction.

You should also use Rubberduck VBA. It is a free add-in with many features to help you write better code. The code formatter alone is invaluable.

Last but not least start answering questions on StackOverflow. This will give you exposure to questions that you would never think to ask and solutions that you didn't know where possible.

\$\endgroup\$
  • 1
    \$\begingroup\$ Advice on the last suggestion ("Last but not least start answering questions on StackOverflow.") - while providing good exposure it will also open you up to criticism, especially as you learn how to provide a good answer. A thick skin and not taking anything as personal helps weather the early attempts! \$\endgroup\$ – AJD Dec 12 '18 at 6:22
  • \$\begingroup\$ @AJD I agree. I have my share of callouses. But I improved more in the 2 years of SO than I did in the previous 6 years. \$\endgroup\$ – TinMan Dec 12 '18 at 6:52
  • \$\begingroup\$ @TinMan This is precisely the thorough comment I was hoping for. Thank you. As far as answering questions, I think that may be a bit premature. But I certainly intend to make the effort to read the responses to other questions in this forum \$\endgroup\$ – grNadpa Dec 12 '18 at 14:58
  • \$\begingroup\$ @grNadpa, Since you're new to Code Review: It's customary to "thank" someone for a contribution that answers your question by clicking the checkmark to the left of the Answer. This also lets others with a similar question know when something helped/worked. Later, when you've accumulated reputation (site points) you'll also be able to upvote questions, comments and proposed answers you find useful/helpful. Enjoy! \$\endgroup\$ – Rey Juna Dec 12 '18 at 19:55
  • \$\begingroup\$ @grNadpa I'm glad that it helps. \$\endgroup\$ – TinMan Dec 12 '18 at 20:13

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