VBA code tidy up

[JohnGow383]

Hi.
I'm new to VBA and my below code works but I'm sure there is a more efficient way of writing it and I'm keen to learn.
This code works and it basically copies a comment from a fixed merged cell in the first sheet (NOON Logs) to cells in another sheet (LOG Entries) in the same workbook. It's fired by a worksheet change in the first sheet. Each comment gets a prefixed consecutive number. The comments will populate a table of up to 8 cells in the 2nd sheet. So say there are two comments already, the code will see they are populated and copy and paste the comment into the 3rd cell and so on. When 8 are full a message box will appear to say maximum number of comments has been logged.
I have also started with it auto deleting the 2nd comment if the last 6 characters of the 2nd comment match the first and want to expand this. As you cn see the length of the code is getting longer and longer so I was wondering if there is a more elegant way of writing this? Here is the whole code from the Worksheet_change private sub. This calls the other copy child macros.

Dim DailyComment, ComRng1, ComRng2, ComRng3, ComRng4, ComRng5, ComRng6, ComRng7, ComRng8 As Range
    Dim Comment1, Comment2 As String
    
    Set ComRng1 = Sheets("LOG Entries").Range("T14")
    Set ComRng2 = Sheets("LOG Entries").Range("T15")
    Set ComRng3 = Sheets("LOG Entries").Range("T16")
    Set ComRng4 = Sheets("LOG Entries").Range("T18")
    Set ComRng5 = Sheets("LOG Entries").Range("T20")
    Set ComRng6 = Sheets("LOG Entries").Range("T22")
    Set ComRng7 = Sheets("LOG Entries").Range("T24")
    Set ComRng8 = Sheets("LOG Entries").Range("T26")
    Set DayCom = Range("H13")
    
    Comment1 = ComRng1.Value
    Comment2 = ComRng2.Value
       
    If Not Application.Intersect(Range("H13"), Target) Is Nothing Then 'Message box is trigger cell
    If Len(DayCom) > 6 And IsEmpty(ComRng1) = False And IsEmpty(ComRng2) = False And IsEmpty(ComRng3) = False And IsEmpty(ComRng4) = False And IsEmpty(ComRng5) = False And IsEmpty(ComRng6) = False And IsEmpty(ComRng7) = False And IsEmpty(ComRng8) = False Then MsgBox "Maximum Number of Comments Logged", vbExclamation, Title:="Comment Logging"
    If Len(DayCom) > 6 And IsEmpty(ComRng1) = False And IsEmpty(ComRng2) = False And IsEmpty(ComRng3) = False And IsEmpty(ComRng4) = False And IsEmpty(ComRng5) = False And IsEmpty(ComRng6) = False And IsEmpty(ComRng7) = False And IsEmpty(ComRng8) = True Then Call CopyDailyComment8
    If Len(DayCom) > 6 And IsEmpty(ComRng1) = False And IsEmpty(ComRng2) = False And IsEmpty(ComRng3) = False And IsEmpty(ComRng4) = False And IsEmpty(ComRng5) = False And IsEmpty(ComRng6) = False And IsEmpty(ComRng7) = True Then Call CopyDailyComment7
    If Len(DayCom) > 6 And IsEmpty(ComRng1) = False And IsEmpty(ComRng2) = False And IsEmpty(ComRng3) = False And IsEmpty(ComRng4) = False And IsEmpty(ComRng5) = False And IsEmpty(ComRng6) = True Then Call CopyDailyComment6
    If Len(DayCom) > 6 And IsEmpty(ComRng1) = False And IsEmpty(ComRng2) = False And IsEmpty(ComRng3) = False And IsEmpty(ComRng4) = False And IsEmpty(ComRng5) = True Then Call CopyDailyComment5
    If Len(DayCom) > 6 And IsEmpty(ComRng1) = False And IsEmpty(ComRng2) = False And IsEmpty(ComRng3) = False And IsEmpty(ComRng4) = True Then Call CopyDailyComment4
    If Len(DayCom) > 6 And IsEmpty(ComRng1) = False And IsEmpty(ComRng2) = False And IsEmpty(ComRng3) = True Then Call CopyDailyComment3
    If Len(DayCom) > 6 And IsEmpty(ComRng1) = False And IsEmpty(ComRng2) = True Then Call CopyDailyComment2
    If Len(DayCom) > 6 And IsEmpty(ComRng1) = True Then Call CopyDailyComment1
    
    If IsEmpty(ComRng1) = False And Right(Sheets("LOG Entries").Range("T14").Value, 6) = Right(Sheets("LOG Entries").Range("T15").Value, 6) Then Call ClearCom2 'If the same 2nd comment has been re-entered it will be deleted but leave the first.

Also, you can see I have made comments1 and 2 as string variables and assigned these to their respective ranges. The last line is used to check if comment 2 right 6 characters match comment 1 and then delete comment 2 if true. However I am having to use the whole range address to get this to work. When I use

If IsEmpty(ComRng1) = False And Right(Comment1, 6) = Right(Comment2, 6) Then Call ClearCom2

It doesn't seem to work.
As mentioned above, this all works and does what I need it to do, but I'd like to know if there is a better way of writing this.

Thanks

 

[JohnGow383]

Thanks dave

Most certainly because If you do not specify the workbook then the active workbook is used by default - not a problem if you only ever have one workbook open but that's not very likely so much safer to qualify to correct workbook.

Glad all resolved - good luck with your project

Dave

I am using that now. I also noticed when I did have another workbook open when changing values in that workbook I was getting a run time error displayed. Changing it to that has stopped that.
I am just working on creating undo buttons for my clear data buttons. Since you can't actually undo, I have it pasting to hidden cells so when undo is called it copies from there back. A workaround but it's working.

 

[dmt32]

I am using that now. I also noticed when I did have another workbook open when changing values in that workbook I was getting a run time error displayed. Changing it to that has stopped that.

You have discovered the reason to consider qualifying a worksheet to the workbook.

Another tip may find useful, where you have multiple variables of same data type that have a name just indexed with a suffix, then rather than do this

    Dim Comment1    As Range, Comment2  As Range, Comment3 As Range, Comment4 As Range
    Dim Comment5    As Range, Comment6  As Range, Comment7 As Range, Comment8 As Range

Consider declaring the variable as an array with required number of elements

Dim Comment(1 To 8) As Range

In this case your choice of name would need a little revision as Comment Object is a member of the Comments collection and using same name best avoided. In such cases, the normal workaround is to prefix the name with the variables data type in this case Range

     Dim rngComment(1 To 8) As Range
    
    Set rngComment(1) = ComRng.Cells(1, 1)
    Set rngComment(2) = ComRng.Cells(2, 1)
    Set rngComment(3) = ComRng.Cells(3, 1)
    Set rngComment(4) = ComRng.Cells(5, 1)
    Set rngComment(5) = ComRng.Cells(7, 1)
    Set rngComment(6) = ComRng.Cells(9, 1)
    Set rngComment(7) = ComRng.Cells(11, 1)
    Set rngComment(8) = ComRng.Cells(13, 1)

an advantage of doing this makes it easier to reference each array element in a loop making code much cleaner.

Dave

 

[JohnGow383]

You have discovered the reason to consider qualifying a worksheet to the workbook.

Another tip may find useful, where you have multiple variables of same data type that have a name just indexed with a suffix, then rather than do this

    Dim Comment1    As Range, Comment2  As Range, Comment3 As Range, Comment4 As Range
    Dim Comment5    As Range, Comment6  As Range, Comment7 As Range, Comment8 As Range

Consider declaring the variable as an array with required number of elements

Dim Comment(1 To 8) As Range

In this case your choice of name would need a little revision as Comment Object is a member of the Comments collection and using same name best avoided. In such cases, the normal workaround is to prefix the name with the variables data type in this case Range

     Dim rngComment(1 To 8) As Range
   
    Set rngComment(1) = ComRng.Cells(1, 1)
    Set rngComment(2) = ComRng.Cells(2, 1)
    Set rngComment(3) = ComRng.Cells(3, 1)
    Set rngComment(4) = ComRng.Cells(5, 1)
    Set rngComment(5) = ComRng.Cells(7, 1)
    Set rngComment(6) = ComRng.Cells(9, 1)
    Set rngComment(7) = ComRng.Cells(11, 1)
    Set rngComment(8) = ComRng.Cells(13, 1)

an advantage of doing this makes it easier to reference each array element in a loop making code much cleaner.

Dave

Thanks for the tip Dave. That is definetely cleaner. I will do that.