Excel Userform works but...

[renigar]

This is the first userform I've done and it works but I would appreciate someone in the know looking at it and suggesting how it could be better. I've looked at various reference books and online sites and feel like I need a userforms for dummies book. I'm using a combo box to allow the user to select a month. I have a Cancel button that unloads the form and a Continue button that for some reason requires two clicks to proceed. So I know something is not quite right. This is similar to some of the other post but not the same. My macro when complete will take an exported report and plug the data into the proper columns based on the user selecting the month in the userform. Any suggestions to improve my knowledge will be greatly appreciated. My code follows.

[highlight #FCE94F]Edit - Based on suggestions provided by Combo I have edited the code as shown in the highlighted areas.[/highlight]

Public Sub cboMonths_Change()
' Combo box
End Sub
__________________________________________________________
Public Sub cmdCancel_Click()
    Unload sMonth
End Sub
____________________________________________________________
Public Sub cmdContinue_Click()
    strMonth = sMonth.cboMonths.Value
    sMonth.Hide
End Sub
_________________________________________________________________
Private Sub Label2_Click()
                           [highlight #FCE94F]' I got rid of this, it wasn't needed[/highlight]
End Sub
____________________________________________________________________________
Public Sub UserForm_Initialize()
    
ufMonths = Array("January", "February", "March", "April", "May", "June", _
            "July", "August", "September", "October", "November", "December")
    
sMonth.cboMonths.List = ufMonths

With sMonth
.StartUpPosition = 0
.Left = Application.Left + (0.5 * Application.Width) - (0.5 * .Width)
.Top = Application.Top + (0.5 * Application.Height) - (0.5 * .Height)
' .Show                     [highlight #FCE94F]I got rid of this line, it was redundant[/highlight]
End With
    
End Sub
_________________________________________________________________________
Global strMonth As String              'Procedure starts here
_________________________________________________________________________
Public Sub ImportGroupData()

Dim Finfo As String         ' Used for file extension filters
Dim FilterIndex As Integer  ' Used to indicate default file extension
Dim Title As String         ' Used to hold the file dialog title text
Dim FPath As Variant        ' Holds the file name and path that is selected
Dim FName As Variant        ' Holds the name of the file that is selected
Dim dMonth As String        ' Holds the name of the month the data applies to

'   Allows the user to use the File Open dialog to select a file.
'   Set up list of file extension filters for the file type drop down,
'   I used wild card only. Others could be added.
Finfo = "All Files (*.*),*.*"
'   Display All Files by default. If you have more extensions in the above list
'   the FilterIndex determines what number in the list shows by default.
FilterIndex = 1
'   Set the dialog box title caption
Title = "Select the file to import"
'   Get the filename
FPath = Application.GetOpenFilename(Finfo, _
FilterIndex, Title)
'   Handle return info from dialog box
If FPath = False Then
    MsgBox "No file was selected."
End If
'   Exit macro if no file is selected
If FPath = False Then Exit Sub

' Grabs the file name from the full path
    FName = Dir(FPath)
' Open the selected file
    Workbooks.Open FileName:=FName
' Make the workbook active
    Windows(FName).Activate
    
' Open a userform to select a month for the data to apply to
sMonth.Show

dMonth = strMonth

' Check if userform is still in memory before unloading it. [highlight #FCE94F]I added this[/highlight][highlight #FCE94F][/highlight]
If Not sMonth Is Nothing Then
    Unload sMonth
End If
 
' Code will continue here
 
End Sub

 

[SkipVought]

Hi,

Unfortunately we don't know the functional requirements, just your code.

You state…
"I'm using a combo box to allow the user to select a month."

So my question would be "why select a month (string or number?) instead of a Date?"

But them you state that the month selected determines where report data goes. So what dies that mean, into a different TAB or a different COLUMN which raises the specter other workbook design horrors.

Bottom line I need more specific information about what your application is attempting to accomplish.

Please describe the report. That you're importing a "report" makes me cringe.

How is the data from the report mapped to your workbook?

BTW, IMNSHO, a spreadsheet is supposed to be a hands on tool so consequently in 25 years of designing Excel applications I never inserted a Userform between the user and the data. Used a LOT of controls on the sheet. Lots of VBA. Love that.

Skip,
[sub]
Just traded in my OLD subtlety...
for a NUance![/sub]
"The most incomprehensible thing about the universe is that it is comprehensible" A. Einstein

You Matter...
unless you multiply yourself by the speed of light squared, then...
You Energy!

 

[combo]

It looks like you put together codes from the userform and standard (starting from strMonth declaration) modules.
For me you should review calls to the form and usage of the show, hide and unload commands.

You may use [tt]Me[/tt] instead of [tt]sMonth[/tt] (as [tt]Unload Me[/tt] instead of [tt]Unload sMonth[/tt]). 'Me' refers to object based on the class, in this case month picking form.
The same for other calls to userform in its code ([tt]sMonth.cboMonths.List = ufMonths[/tt], [tt]With sMonth[/tt], etc.).

In [tt]cmdContinue_Click[/tt] procedure you should also unload the form instead of hiding, otherwise it will stay in the memory.

You don't need additional [tt].Show[/tt] in [tt]UserForm_Initialize()[/tt]. Initialize event is fired when loading the form, Show command is already in ImportGroupData() procedure.

Two clicks - there may be a problem with focus, try to move focus to cmdContinue after processing cboMonths. Do you need [tt]Label2_Click[/tt] procedure?

In such configuration I would set to [tt]True[/tt] the properties [tt]Default[/tt] of [tt]cmdContinue[/tt] and [tt]Cancel[/tt] of [tt]cmdCancel[/tt] buttons, they will interact with keyboard (Enter and Esc keys).



combo

 

[renigar]

Combo,
Thanks for your suggestions. The userform is working properly now. I edited the posted code above to show what was changed. I stuck with sMonth instead of Me because being new to userforms seeing the name means more to me. I think getting rid of Label2_Click may have fixed the two clicks on the Continue button. I changed the default properties on the two buttons to True as suggested. I added code to unload the userform in the procedure after the data is handed off.

Skip,
I didn't want to get into all the details if possible. I work for a public water agency in California and due to the drought we are doing comsumption analysis in anticipation of enforcing mandatory cutbacks through penalties on water bills for over consumption. The Utility Billing software has canned reports (which I know you dislike) that are not flexible enough for our needs. Thus Excel comes into play. Thank you for taking a look.

 

[SkipVought]

I feel your pain.

I have used reports as a last resort, which is where you are.

I apologize that I came across in such a strong way. I'll contribute what I can.

The structure of the table you design and load from these reports will make or break the effectiveness of the table and what you may be able to achieve.

Skip,
[sub]
Just traded in my OLD subtlety...
for a NUance![/sub]
"The most incomprehensible thing about the universe is that it is comprehensible" A. Einstein

You Matter...
unless you multiply yourself by the speed of light squared, then...
You Energy!

 

[combo]

I reccomend to remove:
[pre]If Not sMonth Is Nothing Then
Unload sMonth
End If[/pre]

VBA does a lot behind the scenes. A userform (sMonth in your case) is a class with visual designer. Whenever you call a userform that is not in memory, it instantiates the class, as after declaration [tt]Dim sMonth as New sMonth[/tt]. So you create an instance to destroy it immediately.

Another way of working with userforms and an example for understanding how they work:
1. UserForm1 with Label1 and CommandButton1, code:

Private Sub CommandButton1_Click()
    Me.Hide
    ' Unload Me
End Sub

2. Standard module, code:

Sub test()
Dim uf As UserForm1
Set uf = New UserForm1
' it is possible to process the UserForm1 instance now:
uf.Label1.Caption = "first display of the form"
'show uf, modal, so module code waits here
 uf.Show
' after Me.Hide the code passed here
' uf.Show vbModeless ' don't stop the code too
'we can show it again with new label caption
uf.Label1.Caption = "second display of the form"
uf.Show
' or destroy
Unload uf
' and set uf variable to nothing
Set uf = Nothing
End Sub

If, in the userform module, you replace [tt]Me.Hide[/tt] by [tt]Unload Me[/tt], an automation error occurs.

combo

 

[Andrzejek]

I would also strongly suggest using [blue][tt]Option Explicit[/tt][/blue] at the top of your code.

---- Andy

"Hmm...they have the internet on computers now"--Homer Simpson

 

[renigar]

Combo,
Thanks for the info. Can't say I fully understand it yet. I will have to play with this and see what happens, let it sink in to the brain cells.

Andrzejek,
Thanks. Noted.

 

[combo]

Think about an userform in your VBA project as class definition. In the other code you create an instance, either with the same name of object as class in your project, or custom ([tt]Dim frmCustomName As sMonth[/tt]). In the first case the instance is created by [tt]sMonth.Show[/tt], in the second two lines are required (if no [tt]New[/tt] in declaration: [tt]Dim frmCustomName As sMonth: Set frmCustomName = New sMonth[/tt]. The second technique allows to create for instance two modeless copies of userform, if there is any reason for this.
The form with its data exist until it is destroyed, either by closing by user, or by code ([tt]Unload[/tt]).

combo

 

[RobertPhillips]

As well as combo's comments, you shouldn't reference the form within the form code using the form variable, use the Me keyword instead.

Here is how I would do it.

Module code

Global strMonth As String              'Procedure starts here
Global sMonth As UserForm1             'Change to your form name <<<<<<<<<<<<<<

Public Sub ImportGroupData()
Dim Finfo As String         ' Used for file extension filters
Dim FilterIndex As Integer  ' Used to indicate default file extension
Dim Title As String         ' Used to hold the file dialog title text
Dim FPath As Variant        ' Holds the file name and path that is selected
Dim FName As Variant        ' Holds the name of the file that is selected
Dim dMonth As String        ' Holds the name of the month the data applies to

    Finfo = "All Files (*.*),*.*"
    FilterIndex = 1
    Title = "Select the file to import"
    FPath = Application.GetOpenFilename(Finfo, _
    FilterIndex, Title)
    If FPath = False Then
        MsgBox "No file was selected."
        Exit Sub
    End If

    FName = Dir(FPath)
    Workbooks.Open Filename:=FName
    Windows(FName).Activate
    
    ufMonths = 

    
    Set sMonth = New UserForm1
    With sMonth
    
        .StartUpPosition = 0
        .Left = Application.Left + (0.5 * Application.Width) - (0.5 * .Width)
        .Top = Application.Top + (0.5 * Application.Height) - (0.5 * .Height)

        .cboMonths.List = Array("January", "February", "March", "April", "May", "June", _
                                "July", "August", "September", "October", "November", "December")

        .Show
        If .Cancel Then
        
            Unload sMonth
        Else
        
            .Hide
    
            dMonth = .SelectedMonth
            
            'rest of your code
        End If
    End With

End Sub

Userform code

Dim mCancel As Boolean

Public Property Get Cancel() As Boolean
    Cancel = mCancel
End Property

Public Property Get SelectedMonth() As String
    SelectedMonth = Me.cboMonths.Value
End Property

Public Sub cboMonths_Change()
' Combo box
End Sub

Public Sub cmdCancel_Click()
    mCancel = True
    Me.Hide
End Sub

Public Sub cmdContinue_Click()
    mCancel = False
    Me.Hide
End Sub

Private Sub UserForm_QueryClose(Cancel As Integer, CloseMode As Integer)

    If CloseMode = vbFormControlMenu Then
    
        mCancel = True
    End If
End Sub

 

[Andrzejek]

Robert,

renigar said before:

I stuck with sMonth instead of Me because being new to userforms seeing the name means more to me.

And I can understand his approach, makes sense.

BTW - you can totally skip [tt]Me[/tt] or [tt]sMonth[/tt] if you refer to any controls on the Form itself:

Public Property Get SelectedMonth() As String
    SelectedMonth = cboMonths.Value   [green]'instead of Me.cboMonths.Value 
                                      'or sMonth.cboMonths.Value[/green]
End Property

Of course, you still want to have intelisense available, but you can always get this functionality by typing in code [tt]cboM[/tt] and hit Ctrl-Space

---- Andy

"Hmm...they have the internet on computers now"--Homer Simpson

 

[strongm]

I'd also replace

[COLOR=blue]With sMonth
    .StartUpPosition = 0
    .Left = Application.Left + (0.5 * Application.Width) - (0.5 * .Width)
    .Top = Application.Top + (0.5 * Application.Height) - (0.5 * .Height)
    [COLOR=green]' .Show                     I got rid of this line, it was redundant[/color]
End With[/color]

with

[COLOR=blue]StartUpPosition = 1[/color]

 

[RobertPhillips]

I didn't notice that he had said that Andy, but I still think he shouldn't. The form is a class, Me refers to the class, so use it. Why do you need to know the name when you are in the class? Plus, it is another place to modify if you change the name of the variable.

As for not using me to qualify the controls, I know you can, but again I don't think you should. It's like default properties, I wouldn't use them either. Programming deserves good style.

 

[strongm]

> The form is a class, Me refers to the class, so use it.

Here's a simple class:

Option Explicit

Public name As String
 
Private Sub Hello()
    MsgBox "Hello my name is " & name   
End Sub

Public Sub example()
    Call Hello
End Sub

Private Sub Class_Initialize()
    name = "Wombat"
End Sub

Would you prefer to use Me.name and Me.Hello, as below

Option Explicit

Public name As String
 
Private Sub Hello()
    MsgBox "Hello my name is " & Me.name
End Sub

Public Sub example()
    Call Me.Hello
End Sub

Private Sub Class_Initialize()
    name = "Wombat"
End Sub

That would be ... odd. There are use cases for Me (such passing information about the currently executing instance of a class to a procedure in another module, or to prevent masking of VBA functions), but it is fairly gratuitous and unnecessary to use it within the class itself (and, as you yourself point out, a form is just a class). As you also say, Programming deserves good style. Plus it doesn't always work as you might expect. Without going into the reasons, if you actually try and use second example presented above, it will fail ...

 

[Andrzejek]

renigar said at the very begining:

This is the first userform I've done and it works but I would appreciate someone in the know looking at it and suggesting how it could be better.

So, we do suggest some changes. It is up to renigar to use them or not.
But the most important part is the underlined one...

When I started programing, I was ecstatic when my code worked! No matter how messy, or disorganized, or not up-to-standards, or … Later, when I ‘developed’ as a programmer, my code, organization, standards, etc. changed and (hopefully) is more what other ‘seasoned’ programmers may consider: ‘OK, I can live with it’

The same may (hopefully, will) happen to renigar (not that is a ‘criticism’ of your code, just a ‘critique’)


---- Andy

"Hmm...they have the internet on computers now"--Homer Simpson

 

[renigar]

Thanks Everyone,
I really appreciate all the tips.
renigar