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

 

[6StringJazzer]

This can't be all the code because it is missing an End If. I assumed that was the only missing line.

Variable DayCom is not declared. I strongly recommend to everyone that they use Option Explicit and declare variables. Doing so prevents a lot of bugs and runtime errors.

This version of your uses more lines of code, but it tests each condition only once, which is a much better structure from the standpoint of boolean programming. I personally find it more readable.

I have left in the Call keyword but you don't need it. It is leftover for compatibility with older versions of VB.

Your two Dim statements don't do what you think they do. You must specify a data type for every variable. If you do not, it will be defaulted to a Variant data type. This is often OK but often not. It is possible that is what is causing the problem on your last line of code because Comment1 will be typed as Variant, but I'm not sure.

   Dim DailyComment As Range, ComRng1 As Range, ComRng2 As Range, ComRng3 As Range, _
       ComRng4 As Range, ComRng5 As Range, ComRng6 As Range, ComRng7 As Range, ComRng8 As Range
   Dim Comment1 As String, 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 Then
         If Not IsEmpty(ComRng1) Then
            If Not IsEmpty(ComRng2) Then
               If Not IsEmpty(ComRng3) Then
                  If Not IsEmpty(ComRng4) Then
                     If Not IsEmpty(ComRng5) Then
                        If Not IsEmpty(ComRng6) Then
                           If Not IsEmpty(ComRng7) Then
                              If Not IsEmpty(ComRng8) Then
                                 MsgBox "Maximum Number of Comments Logged", vbExclamation, Title:="Comment Logging"
                              Else
                                 Call CopyDailyComment8
                              End If
                           Else
                              Call CopyDailyComment7
                           End If
                        Else
                           Call CopyDailyComment6
                        End If
                     Else
                        Call CopyDailyComment5
                     End If
                  Else
                     Call CopyDailyComment4
                  End If
               Else
                  Call CopyDailyComment3
               End If
            Else
               Call CopyDailyComment2
            End If
         Else
            Call CopyDailyComment1
         End If
      
      ElseIf 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.
      End If
   
   End If ' missing

 

[JohnGow383]

Wow, thanks for taking the time to reply.
You're quite right, I didn't paste in the final End If.
Also, daycom was originally dailycomment and I changed it later on but forgot to change it at the declarations part. It was still working but must have been triggering when less than 6 characters were detected.
Thanks for the updated code, I would never have thought of doing it that way as it's not as logical or as easy to understand for me personally. I will test it later when I'm back near the computer. And thanks too about the not needing to use call.
I still don't understand why the last part of the code isn't working as I've declared comment 1 and 2 as strings and set them to the value of their respective ranges. When I had it using the named variables it was calling when the first comment was entered which was really weird but not when the 2nd comment matched the first.
Still a very steep learning curve but enjoying it. I really should book some kind of course to better understand it.
Thanks again for your help, I'll try it later and mark as solved once tested.

 

[6StringJazzer]

I still don't understand why the last part of the code isn't working

I don't either, just from reading the code. If I can find time I'll try to set up a test file but it would also help if I used the same data as you.

I am a professional software developer and started learning VBA about 15 years ago so don't fee discouraged if you don't everything figured out in one day

The way I wrote the logic is not necessarily "better"; it is logically the same. But I think most pro programmers would do it the way I did, for the main reason I mentioned--you only evaluate each condition once. That way if you have to change the code it is much easier. To my eye it is easier to understand, but that's personal taste.

 

[JohnGow383]

Thanks.

I don't either, just from reading the code. If I can find time I'll try to set up a test file but it would also help if I used the same data as you.

I am a professional software developer and started learning VBA about 15 years ago so don't fee discouraged if you don't everything figured out in one day

The way I wrote the logic is not necessarily "better"; it is logically the same. But I think most pro programmers would do it the way I did, for the main reason I mentioned--you only evaluate each condition once. That way if you have to change the code it is much easier. To my eye it is easier to understand, but that's personal taste.

I have just tested it and it mostly works. The only part that didn't work was if 2nd comment is same as 1st comment. Strangely, I noticed you used the ElseIf statement for that part. I changed this to just If and then it forced me to add another End If and now seems to be working.
Also what I don't understand, with the wrong variable declared (dailycomment) instead of daycom, it was still working. Ie not firing if under 6 chars. So it seems I'm able to Set daycom as a range without correctly declaring it as a range first.
The ranges for the comments are actually merged cells. The other question I have is when setting the ranges I am just using the first top left cell reference (which is what Excel uses). When I tried to set as the merged range ie T14:W15 it didn't work at all. These comments are cleared by another button, I used the recorder to build the code for the that and that uses the whole range to select. So it seems to be a bit inconsistent how to actually code the ranges.
Don't get me wrong, I do prefer your method, I guess I just did it my way because initially I had to think how I was going to achieve what I wanted to do. I'm assuming this is VBAs version of nesting Ifs?
Thanks again

 

[6StringJazzer]

If DayCom is undeclared, it will be typed as Variant, which can contain any data type. VBA should determine what type it has based on context, i.e., you can do this:

' Undeclared variable DayCom
DayCom = "A string" ' will now be a String type
DayCom = 5 ' now it becomes an Integer
DayCom = 123.45 ' now it's a Double
Set DayCom = Range("A1") ' now it's a Range

In computer science we call this weak typing--the variable can contain any type of data. It can be powerful but tricky and more often than not causes bugs so most people avoid this sort of thing as a poor practice.

So that's why your code worked, once you squared away the If.

Merged cells are a Terrible, Horrible, No Good, Very Bad thing. They cause more problems than they solve and most Excel experts advise avoiding them. There is almost always another way to do what you want without them. I'm not sure why the macro recorder code didn't work for deleting it, but that is typical of problems you encounter with merged cells.

 

[JohnGow383]

If DayCom is undeclared, it will be typed as Variant, which can contain any data type. VBA should determine what type it has based on context, i.e., you can do this:

' Undeclared variable DayCom
DayCom = "A string" ' will now be a String type
DayCom = 5 ' now it becomes an Integer
DayCom = 123.45 ' now it's a Double
Set DayCom = Range("A1") ' now it's a Range

In computer science we call this weak typing--the variable can contain any type of data. It can be powerful but tricky and more often than not causes bugs so most people avoid this sort of thing as a poor practice.

So that's why your code worked, once you squared away the If.

Merged cells are a Terrible, Horrible, No Good, Very Bad thing. They cause more problems than they solve and most Excel experts advise avoiding them. There is almost always another way to do what you want without them. I'm not sure why the macro recorder code didn't work for deleting it, but that is typical of problems you encounter with merged cells.

Oh I see. So are you saying that I don't need to use Dim As Range at the start since when I use `Set = Range` this is not only setting the variable as a range but also defining what the range is?

Dim DayCom As Range, ComRng1 As Range, ComRng2 As Range, ComRng3 As Range, _
       ComRng4 As Range, ComRng5 As Range, ComRng6 As Range, ComRng7 As Range, ComRng8 As Range ' Do I need to do this?
   Dim Comment1 As String, 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")

With regards to merged cells, the recorder would select say Range("T14:W15").Select and then clearcontents. I did not try changing this to just say T14. But for example, when I was trying to Set ComRng1 using

   Set ComRng1 = Sheets("LOG Entries").Range("T14:W15")

doesn't work.
A lot of the problems I have with VBA is the syntax, which I know will just take time and practice. It is very different from using Excel formulas to adjust data, which I find a lot easier to understand probably because I'm way more used to it and it kind of helps you build complex formula easily. I often know what I want to do but struggle with acheiving it. A lot of googling examples, asking on here (which has been a god send from people like you) and reverse engineering some recordings really helps.

 

[JohnGow383]

Also something very strange going on when I tried to add the next delete line. I added this:

 If Right(Sheets("LOG Entries").Range("T15").Value, 6) = Right(Sheets("LOG Entries").Range("T16").Value, 6) Then
           Call ClearCom3
End If

So it works perfectly when Comment 2 is the say as No 1. But doesn't work when comment 3 is the same as comment 2. I then added the above into another module as it's own sub for testing and when comment 3 is the same as 2 it calls ClearCom3. I am thinking it may be a latency issue? If it's taking some time to actually paste the comment between sheets is it possible that when it's checking for the duplicate comment, the comment has yet to be pasted? I wouldn't have thought so, but it doesn't make much sense to me

 

[6StringJazzer]

So are you saying that I don't need to use Dim As Range at the start since when I use `Set = Range` this is not only setting the variable as a range but also defining what the range is?

When you did this

Set DayCom = Range("H13")

then VBA said, "Hmm, this is undeclared so I'm going to make it Variant. Also, in this context, it is being used as a Range, so for now I will make it a Range object."
However, it's not a good practice, hence my link earlier to use Option Explicit.

So it works perfectly when Comment 2 is the say as No 1. But doesn't work when comment 3 is the same as comment 2. I then added the above into another module as it's own sub for testing and when comment 3 is the same as 2 it calls ClearCom3. I am thinking it may be a latency issue? If it's taking some time to actually paste the comment between sheets is it possible that when it's checking for the duplicate comment, the comment has yet to be pasted? I wouldn't have thought so, but it doesn't make much sense to me

I'm finding this description a bit confusing vis a vis the code. I would need to experiment with your actual file to diagnose this.

There is no latency issue. VBA a single-threaded synchronous language. If you do a paste, execution does not continue until the paste is done. The only exception is when you use VBA to send commands to an external application or system, like sending a URL to open in Internet Explorer or sending an HTTP command to a server.

 

[JohnGow383]

When you did this

Set DayCom = Range("H13")

then VBA said, "Hmm, this is undeclared so I'm going to make it Variant. Also, in this context, it is being used as a Range, so for now I will make it a Range object."
However, it's not a good practice, hence my link earlier to use Option Explicit.

I'm finding this description a bit confusing vis a vis the code. I would need to experiment with your actual file to diagnose this.

There is no latency issue. VBA a single-threaded synchronous language. If you do a paste, execution does not continue until the paste is done. The only exception is when you use VBA to send commands to an external application or system, like sending a URL to open in Internet Explorer or sending an HTTP command to a server.

Thanks. Yes I tried using Option Explicit but it broke other things, so removed it again. I have a good few other Worksheet_change in the same sheet (perhaps why it's not working as I thought it should) as well as calculation and it's probably littered with mishaps and undecalred variables. It's too complex to use XL2BB etc. I tried lots of different methods to get 3rd comment and it was firing when 2nd comment was added. It's not important, more of a bit of fun now so no worries if you don't have time.