PDA

View Full Version : I need help cleaning up this code.



khodjo
12-15-2009, 02:37 PM
Just to preface this by saying i am a novice at VBA. Started about 3 weeks ago.
Anyhow, what I am I am trying to do with this code is this.

Create Header - creates the header.

Add Item
This is supposed to open a dialog box where I enter the item number from a list named as ITEM_NUMBER and the quantity.

The entry is then placed on the sheet and a vlookup is invoked to bring up the description from the list.

The dialog is then cleared to receive the next entry which is then placed on the next row. and so on.

the header code works.

the Vlookup does not. Please help...Beinga beginner, all snarky comments are welcome. thanks.

The file is attached.

lucas
12-15-2009, 02:43 PM
Moved to the excel help forum.

khodjo
12-15-2009, 02:48 PM
Thanks VBAX. My apologies.

mdmackillop
12-15-2009, 03:48 PM
Try this, using a combobox for easier data entry.

Aussiebear
12-15-2009, 03:50 PM
Just a couple of quick comments

1. When writing code try to use the "Option Explicit" function. This forces you to define any variables. Great habit to get into particularly when learning to code.

2. Minimise the use of ".Select" While the Macro Recorder does this, most programmers will tell you that it simply slows the system down. In your code example you used the following:


Range("A2:F2").Select
With Selection Range("A2:F2").Font
.Themecolor = 5
.Size = 12
.Name = "Calabri"
.Bold = True


It can be more easily written as

With Range("A2:F2").Font
.ThemeColor = 5
.Size = 12
.Name = "Calibri"
.Bold = True
End With


3. I don't actually see any code for the lookup function in your example

mdmackillop
12-15-2009, 04:59 PM
Other ways to check the number is correct

Paul_Hossler
12-15-2009, 05:53 PM
I always like to use a worksheet just to make sure that the sheet I think is active, is the same sheet Excel thinks is active


Worksheets("Estimate of Quantities").Range("A1:F1").Merge



Also hard coded "will never be more than 100" rows seem to always grow


Range("A3:F100").ClearContents


What I like to do is let Excel do all the work


Dim rData As Range

Set rData = Worksheets("Estimate of Quantities").Cells(1, 1).CurrentRegion

With rData
.Cells(3, 1).Resize(.Rows.Count - 2, .Columns.Count).ClearContents
End With



My opinions / personal style :yes

Paul

khodjo
12-16-2009, 08:29 AM
Thanks for all the responses so far. This is the code I am using for the OK button. The named range is "catalog" and the sheet it was named from is Item Catalog.
The vlookup line is supposed to take what was transferred from the dialog box into the first column, look it up in the named range Catalog and return that to the second column on the same line.

Here is what I have so far.:
Sub cmdOK_Click()
Dim NextRow As Long
Dim inumber As Long
Dim ws As Worksheet

' Activate the worksheet
Sheets("Estimate of Quantities").Activate

'Determine the next empty row
NextRow = WorksheetFunction.CountA(Range("A:A")) + 1

'Transfer New Item Number and Quantity

Cells(NextRow, 1) = txtItemNo.Text
Cells(NextRow, 3) = txtQty.Text

'Use vlookup to get description from named range

inumber = Cells(NextRow, 1)
Set ws = Sheets("ITEM CATALOG")
Result = Application.VLookup(inumber, ws.Range("ITEM_NUMBER"), 3, False)

If IsError(Result) Then
MsgBox "No Match Found, Enter a Correct Number"
Else
Cells(NextRow, 2) = Result
End If


'Clear contents for next entry
txtItemNo.Text = ""
txtQty.Text = ""
txtItemNo.SetFocus

End Sub

Simon Lloyd
12-16-2009, 11:09 AM
Cross posted and answered here http://www.mrexcel.com/forum/showthread.php?t=436439

Simon Lloyd
12-16-2009, 11:10 AM
khodjo, please take a minute to read the forum rules and the link in my signature with regards cross posting!