Clean up button click

R

rguy84

Board Regular
Joined
May 18, 2011
Messages
112
I want to see if this button click can be less messy. Basically:
if checkbox is checked {
if some values appear on Past Contacts {
msgbox
}else add form values to sheets Contact and past contact
}else add values to contacts

Code: 
Private Sub btnAdd_Click()
    Dim who, conInitiate, conLastName, conFirstName As String
    Dim conCIO, conEmail, conPhone, conSummary, conType As String
    Dim conDate, conStartTime, conEndTime As Date
    Dim lastRow, lastRowPC As Long
    Dim cWs, PCWs As Worksheet
 
    who = Me.cmbWho.Value
    conDate = Me.txtDate.Value
    conInitiate = Me.cmbInitiate.Value
    conLastName = Me.txtLastName.Value
    conFirstName = Me.txtFirstName.Value
    conCIO = Me.cmbCIO.Value
    conEmail = Me.txtEmail.Value
    conPhone = Me.txtPhone.Value
    conSummary = Me.txtSummary.Value
    conType = Me.cmbType.Value
    conStartTime = Me.txtStart.Value
    conEndTime = Me.txtEndTime.Value
    Set PCWs = Worksheets("PastContacts")
    Set cWs = Worksheets("Contact")
 
    ' last row of Contact WS
    lastRow = cWs.Range("A" & Rows.Count).End(xlUp).Offset(1, 0).Row
    lastRowPC = PCWs.Range("a" & Rows.Count).End(xlUp).Offset(1, 0).Row
 
    If Me.cboxFreq.Value = True Then
        If WorksheetFunction.CountIf(PCWs.Range("A2:A" & lastRow), conLastName) > 0 Then
            If WorksheetFunction.CountIf(PCWs.Range("B2:b" & lastRow), conFirstName) > 0 Then
                MsgBox "The name " & conFirstName & " " & conLastName & "is already on the list.", vbOKOnly
            End If
        End If
        cWs.Range("A" & lastRow).Value = who
        cWs.Range("B" & lastRow).Value = Format(conDate, "mm/dd/yyyy")
        cWs.Range("C" & lastRow).Value = conInitiate
        cWs.Range("D" & lastRow).Value = conLastName
        cWs.Range("E" & lastRow).Value = conFirstName
        cWs.Range("F" & lastRow).Value = conCIO
        cWs.Range("G" & lastRow).Value = conEmail
        cWs.Range("H" & lastRow).Value = conPhone
        cWs.Range("I" & lastRow).Value = conSummary
        cWs.Range("J" & lastRow).Value = conType
        cWs.Range("K" & lastRow).Value = Format(conStartTime, "hh:mm")
        cWs.Range("L" & lastRow).Value = Format(conEndTime, "hh:mm")
        cWs.Range("M" & lastRow).FormulaR1C1 = Format("=CalcTime(RC11,RC12)", "hh:mm")
 
        PCWs.Range("A" & lastRowPC).Value = conLastName
        PCWs.Range("B" & lastRowPC).Value = conFirstName
        PCWs.Range("C" & lastRowPC).Value = conCIO
        PCWs.Range("D" & lastRowPC).Value = conEmail
        PCWs.Range("E" & lastRowPC).Value = conPhone
        Call SortAndRemoveDupes
        clearCtrl Me
        Me.Hide
    Else 'checkbox not checked
        cWs.Range("A" & lastRow).Value = who
        cWs.Range("B" & lastRow).Value = Format(conDate, "mm/dd/yyyy")
        cWs.Range("C" & lastRow).Value = conInitiate
        cWs.Range("D" & lastRow).Value = conLastName
        cWs.Range("E" & lastRow).Value = conFirstName
        cWs.Range("F" & lastRow).Value = conCIO
        cWs.Range("G" & lastRow).Value = conEmail
        cWs.Range("H" & lastRow).Value = conPhone
        cWs.Range("I" & lastRow).Value = conSummary
        cWs.Range("J" & lastRow).Value = conType
        cWs.Range("K" & lastRow).Value = Format(conStartTime, "hh:mm")
        cWs.Range("L" & lastRow).Value = Format(conEndTime, "hh:mm")
        cWs.Range("M" & lastRow).FormulaR1C1 = Format("=CalcTime(RC11,RC12)", "hh:mm")
 
        clearCtrl Me
        Me.Hide
    End If
End Sub

Thanks
 

Excel Facts

Pivot Table Drill Down
Double-click any number in a pivot table to create a new report showing all detail rows that make up that number
Sort by date Sort by votes
Break it up into a couple functions/subs, thought your going to run into some var issues since you have so many. You either got to redefine them in every sub or pass them all. Both very sloppy coding.

I would set up an array for all your strings and then one for all your numbers. Enumerate using the names of the fields and use the emuneration as the index of the arrays. Then you just pass your arrays to the subs. Much tidier.
 
Upvote 0
Yeah, the multiple var declares for the same thing is driving me nuts. Can you give me an example of what you mean. I don't quite follow
 
Upvote 0
Appears to be some repetition there:
Code: 
Private Sub btnAdd_Click()
   Dim who, conInitiate, conLastName, conFirstName As String
   Dim conCIO, conEmail, conPhone, conSummary, conType As String
   Dim conDate, conStartTime, conEndTime As Date
   Dim lastRow, lastRowPC As Long
   Dim cWs, PCWs As Worksheet

   who = Me.cmbWho.Value
   conDate = Me.txtDate.Value
   conInitiate = Me.cmbInitiate.Value
   conLastName = Me.txtLastName.Value
   conFirstName = Me.txtFirstName.Value
   conCIO = Me.cmbCIO.Value
   conEmail = Me.txtEmail.Value
   conPhone = Me.txtPhone.Value
   conSummary = Me.txtSummary.Value
   conType = Me.cmbType.Value
   conStartTime = Me.txtStart.Value
   conEndTime = Me.txtEndTime.Value
   Set PCWs = Worksheets("PastContacts")
   Set cWs = Worksheets("Contact")

   ' last row of Contact WS
   lastRow = cWs.Range("A" & Rows.Count).End(xlUp).Offset(1, 0).Row
   lastRowPC = PCWs.Range("a" & Rows.Count).End(xlUp).Offset(1, 0).Row

   cWs.Range("A" & lastRow).Value = who
   cWs.Range("B" & lastRow).Value = Format(conDate, "mm/dd/yyyy")
   cWs.Range("C" & lastRow).Value = conInitiate
   cWs.Range("D" & lastRow).Value = conLastName
   cWs.Range("E" & lastRow).Value = conFirstName
   cWs.Range("F" & lastRow).Value = conCIO
   cWs.Range("G" & lastRow).Value = conEmail
   cWs.Range("H" & lastRow).Value = conPhone
   cWs.Range("I" & lastRow).Value = conSummary
   cWs.Range("J" & lastRow).Value = conType
   cWs.Range("K" & lastRow).Value = Format(conStartTime, "hh:mm")
   cWs.Range("L" & lastRow).Value = Format(conEndTime, "hh:mm")
   cWs.Range("M" & lastRow).FormulaR1C1 = Format("=CalcTime(RC11,RC12)", "hh:mm")

   If Me.cboxFreq.Value = True Then
      If WorksheetFunction.CountIf(PCWs.Range("A2:A" & lastRow), conLastName) > 0 Then
         If WorksheetFunction.CountIf(PCWs.Range("B2:b" & lastRow), conFirstName) > 0 Then
            MsgBox "The name " & conFirstName & " " & conLastName & "is already on the list.", vbOKOnly
         End If
      End If

      PCWs.Range("A" & lastRowPC).Value = conLastName
      PCWs.Range("B" & lastRowPC).Value = conFirstName
      PCWs.Range("C" & lastRowPC).Value = conCIO
      PCWs.Range("D" & lastRowPC).Value = conEmail
      PCWs.Range("E" & lastRowPC).Value = conPhone
      Call SortAndRemoveDupes
   End If
   clearCtrl Me
   Me.Hide
End Sub
 
Upvote 0
FYI, when you write this:
Code: 
Dim conCIO, conEmail, conPhone, conSummary, conType As String
only conType is declared as String, all the others are variants. You need:
Code: 
Dim conCIO As String, conEmail As String, conPhone As String, conSummary As String, conType As String
if they are all Strings
 
Upvote 0
Since ur just pasting and doing no calculations, name everything as Varients, this will simplifiy it.

Code: 
'index for string vars
enum arrIndex
 who = 1
 conDate = 2
 conLastName = 3
  etc...
 conEndDate = 12  (i think its the 12th)
end enum

Then with you say for all your vars

dim arrVar(arrIndex.who to arrIndex.conEndDate) as Varient

Then you do
arrVar(arrIndex.who) = Me.cmbWho.Value
arrVar(arrIndex.conDate) = MetxtDate.Value
etc.

Then when you make a function call, you just pass arrVar() and it holds everything.

If you need to loop through the array,

dim eIndex as arrIndex
for eIndex = who to conEndDate

next eIndex
 
Upvote 0
Unless you actually need to manipulate them - and maybe not even then - there's no need to copy the form fields into local variables before copying them out to the worksheet: just copy the form fields directly into the worksheet.

So instead of:-
Code: 
   conCIO = Me.cmbCIO.Value
   conEmail = Me.txtEmail.Value
[COLOR=green][COLOR=#000000]   conPhone = Me.txtPhone.Value
   conSummary = Me.txtSummary.Value
   conType = Me.cmbType.Value[/COLOR]
' some code cut[/COLOR]
   cWs.Range("F" & lastRow).Value = conCIO
   cWs.Range("G" & lastRow).Value = conEmail
   cWs.Range("H" & lastRow).Value = conPhone
   cWs.Range("I" & lastRow).Value = conSummary
   cWs.Range("J" & lastRow).Value = conType
just do this:-
Code: 
   cWs.Range("F" & lastRow).Value = Me.cmbCIO.Value
   cWs.Range("G" & lastRow).Value = Me.txtEmail.Value
[COLOR=green][COLOR=#000000]   cWs.Range("H" & lastRow).Value = Me.txtPhone.Value
   cWs.Range("I" & lastRow).Value = Me.txtSummary.Value
   cWs.Range("J" & lastRow).Value = Me.cmbType.Value[/COLOR]
[/COLOR]
 
Upvote 0
Rory, Yes I know there is repetition there. I didnt try your code, but, doesn't it add to contacts no matter what? If the person is already on the past contacts list, cboxFreq is true and thhe person clicks submit, I don't want the form to submit.

Thanks for the variable help, I figured since they were on the same line, theyy all looked at the same AS
 
Upvote 0
You must log in or register to reply here.

Forum statistics

Threads
1,224,578
Messages
6,179,652
Members
452,934
Latest member
mm1t1

Share this page

We've detected that you are using an adblocker.

We have a great community of people providing Excel help here, but the hosting costs are enormous. You can help keep this site running by allowing ads on MrExcel.com.
Allow Ads at MrExcel

Which adblocker are you using?

Disable AdBlock

Follow these easy steps to disable AdBlock

1)Click on the icon in the browser’s toolbar.
2)Click on the icon in the browser’s toolbar.
2)Click on the "Pause on this site" option.
Go back

Disable AdBlock Plus

Follow these easy steps to disable AdBlock Plus

1)Click on the icon in the browser’s toolbar.
2)Click on the toggle to disable it for "mrexcel.com".
Go back

Disable uBlock Origin

Follow these easy steps to disable uBlock Origin

1)Click on the icon in the browser’s toolbar.
2)Click on the "Power" button.
3)Click on the "Refresh" button.
Go back

Disable uBlock

Follow these easy steps to disable uBlock

1)Click on the icon in the browser’s toolbar.
2)Click on the "Power" button.
3)Click on the "Refresh" button.
Go back
Back
Top