PDA

View Full Version : Absolute beginner in need of help



TomKix
02-13-2012, 02:26 PM
Hi all,

I am a complete beginner when it comes to VBA. I've only been using it for 24 hours.

I've coded the beginnings of a programme and ran a debug on it. Problem is it comes back with ; " Compile error : Procedure too long"

It is based around a lot of conditionals such that if a series of data are entered into cells it will then select and express data from other sheets in the neighbouring cell of the initial sheet

e.g. if D3,4 and 5, sheet 1 contain the correct numbers to correspond to cell B3 ,sheet 2 the value of cell B3 is shown in Cell E5 sheet 1.

I've attached one of the notepads of code; there are 7 of these in the original VBA in my excel document.

I have need to add further options of a similar nature to the document as a whole.

Ideas on how to get around this problem are greatly appreciated.

Thanks.

Aussiebear
02-13-2012, 02:51 PM
Welcome to the VBAX forum. Have a go at attaching your workbook by selecting the Go Advanced button, scroll down to Manage Attachments and follow the prompts from there. This way we then get to see what you have in front of you and we'll try to assist from there.

TomKix
02-13-2012, 03:17 PM
Afraid my Excel is too large to upload and the txt doc that I wrote the code in isn't a recognised upload type. So, I've attached a section of the in word.

GTO
02-13-2012, 11:49 PM
Greetings Tom,

Let me echo Ted in welcoming you to vbaexpress :hi:

First, as to attachments: If the excel wb is over an MB, you probably want to strip it down a bit, keeping the bits you wnat help with. Also, if you want or need to attach a text file, you can zip it and attach the zip.

I took a quick look and think you can start trimming this:

If Range("D3").Value = 6 And Range("D4").Value = 1 And Range("D5") = 4 Then
Range("E5").Value = Application.Workbooks("Book1").Worksheets("EWI").Range("B3").Value

End If

If Range("D3").Value = 7 And Range("D4").Value = 1 And Range("D5") = 4 Then
Range("E5").Value = Application.Workbooks("Book1").Worksheets("EWI").Range("C3").Value

End If

If Range("D3").Value = 8 And Range("D4").Value = 1 And Range("D5") = 4 Then
Range("E5").Value = Application.Workbooks("Book1").Worksheets("EWI").Range("D3").Value

End If


...to something like this:


If Range("D4").Value = 1 And Range("D5") = 4 Then
Select Case Range("D3")
Case 6
Range("E5").Value = Application.Workbooks("Book1").Worksheets("EWI").Range("B3").Value
Case 7
Range("E5").Value = Application.Workbooks("Book1").Worksheets("EWI").Range("C3").Value
Case 8
Range("E5").Value = Application.Workbooks("Book1").Worksheets("EWI").Range("D3").Value
Case 9
Range("E5").Value = Application.Workbooks("Book1").Worksheets("EWI").Range("E3").Value
Case 10
Range("E5").Value = Application.Workbooks("Book1").Worksheets("EWI").Range("F3").Value
Case 11
Range("E5").Value = Application.Workbooks("Book1").Worksheets("EWI").Range("G3").Value
Case 12
Range("E5").Value = Application.Workbooks("Book1").Worksheets("EWI").Range("H3").Value
Case Else
'...maybe something else
End Select
End If


Although pseudo code (hopefully no glitches, typed ratehr quickly), you can see that we only need to test D4 and D5 once :-)

Also, as you mentioned that excel complained that the procedure is too long, I am betting that setting some references to the worksheet(s) / range(s) involved would help.

Well, hope that helps a little at least,

Mark

TomKix
02-14-2012, 10:42 AM
Thanks for the help Mark . That's an interesting function that I hadn't come across yet. Certainly makes things look tidier.

I've attached the full workbook as I have a slight concern that it will take just as many lines to make with this new idea since the values of D3,4 and 5 need to be variable and that is before we get onto the next option in the list of things to choose from.

I've got no problem sitting typing things out for ages' the problem is just the length limit of the VBA code :(

GTO
02-15-2012, 12:11 AM
Hi Tom,

I'm afraid 17,000 lines of code is a bit long to read, but if I am getting the basic idea, let's try this to see if it's on the right path.


Private Sub CommandButton1_Click()
Dim wksCalc As Worksheet
Dim wksSourceData As Worksheet
Dim strSourceSheetName As String
Set wksCalc = ThisWorkbook.Worksheets("Calculator")

With wksCalc
'D3 = students
'D4 = weeks
'D5 = class no.
If .Range("D3").Value >= 6 And .Range("D3").Value <= 15 _
And .Range("D4").Value >= 1 And .Range("D4").Value <= 52 _
And .Range("D5") >= 1 And .Range("D5") <= 3 Then

Select Case .Range("D5").Value
Case 1
strSourceSheetName = "EWI"
Case 2
strSourceSheetName = "EWC"
Case 3
strSourceSheetName = "EWPT"
End Select

Set wksSourceData = ThisWorkbook.Worksheets(strSourceSheetName)

.Range("E5").Value = _
wksSourceData.Cells(.Range("D4").Value + 2, .Range("D3").Value - 4).Value
End If
End With
End Sub


In quickly scrolling through, I did not that class 1 and 4 appear to use the same source sheet ('EWI') for pricing. If that is correct, then the first Case could be:

Case 1, 4

...and so on.

Does that help?

Mark

GTO
02-15-2012, 12:17 AM
PS. I see that you just started in vba. Put this at the very top of all your modules. (Both the module for each worksheet, and the Standard Module you inserted.)
Option Explicit



Then go to the menu bar in the code window. Tools|Options and select the Editor tab. Tick the 'Require Variable Declaration' checkbox.

This will then auto-add Option Explicit to new modules you create. It will save you headaches from mis-named variables later.

Hope that helps,

Mark

TomKix
02-15-2012, 04:52 AM
Wow.That is phenomenal. Shows just how much of a beginner I am.

Thanks Mark, that is a massive help in producing this thing.
When you and VBA refer to "Require Variable Declaration" this means the pieces of code that state "Dim XYZ", as Dim is declaring your variables, right?

The code works like a charm and I have expanded it to include the full range of courses as you can see from my attached script.

Option Explicit
Private Sub CommandButton1_Click()
Dim wksCalc As Worksheet
Dim wksSourceData As Worksheet
Dim strSourceSheetName As String
Set wksCalc = ThisWorkbook.Worksheets("Calculator")

With wksCalc
'D3 = students
'D4 = weeks
'D5 = class no.
If .Range("D3").Value >= 6 And .Range("D3").Value <= 15 _
And .Range("D4").Value >= 1 And .Range("D4").Value <= 52 _
And .Range("D5") >= 1 And .Range("D5") <= 7 Then

Select Case .Range("D5").Value
Case 1
strSourceSheetName = "EWI"
Case 2
strSourceSheetName = "EWC"
Case 3
strSourceSheetName = "EWPT"
Case 4
strSourceSheetName = "EWI"
Case 5
strSourceSheetName = "1_1 classes"
Case 6
strSourceSheetName = "C6"
Case 7
strSourceSheetName = "C6C"
End Select

Set wksSourceData = ThisWorkbook.Worksheets(strSourceSheetName)

.Range("E5").Value = _
wksSourceData.Cells(.Range("D4").Value + 2, .Range("D3").Value - 4).Value
End If
End With
End Sub




I gather that if I adjust the pages this code refers to I then ought to be able to use it for the other options in my list of choices to generate the same outcome for them.

Thanks for pointing me in the right direction, the 17000 lines of code were a learning experience if nothing else :omg2:

TomKix
02-15-2012, 05:57 PM
HI guys,
I've been playing about with the code and made the code as shown at base of this post.
Good news is it works but I do have a couple of questions i was hoping you could help me with.
1) Can I make a variable a word; e.g. instead of typing one for 'yes' can i have someone just type 'y' or 'yes'

2) I'm not able to get my accommodation code to work and I think it has something to do with the fact that I don't fully undertstand this part of the code

Set wksSourceData = ThisWorkbook.Worksheets(strSourceSheetName)

.Range("E5").Value = _
wksSourceData.Cells(.Range("D4").Value + 2, .Range("D3").Value - 4).Value


I understand that;
- it is setting the source data for the select case code
- It is setting the range on the worksheet for E5 to work from when producing values

What I don't understand is what the +2 and -4 denote nor how to make this work for the accommodation page.

Hopefully I'm not being totally dense.

Cheers
Tom





Option Explicit
Private Sub CommandButton1_Click()
Dim wksCalc As Worksheet
Dim wksSourceData As Worksheet
Dim strSourceSheetName As String
Set wksCalc = ThisWorkbook.Worksheets("Calculator")

With wksCalc
'D3 = class size
'D4 = weeks
'D5 = class no.
If .Range("D3").Value >= 6 And .Range("D3").Value <= 15 _
And .Range("D4").Value >= 1 And .Range("D4").Value <= 52 _
And .Range("D5") >= 1 And .Range("D5") <= 7 Then

Select Case .Range("D5").Value
Case 1, 4
strSourceSheetName = "EWI"
Case 2
strSourceSheetName = "EWC"
Case 3
strSourceSheetName = "EWPT"
Case 5
strSourceSheetName = "1_1 classes"
Case 6
strSourceSheetName = "C6"
Case 7
strSourceSheetName = "C6C"
End Select

Set wksSourceData = ThisWorkbook.Worksheets(strSourceSheetName)

.Range("E5").Value = _
wksSourceData.Cells(.Range("D4").Value + 2, .Range("D3").Value - 4).Value
End If
End With

'__________________________________________________________________________ ________________
'
'Exams
'
With wksCalc
'D4 = weeks
'D6 = Exam option
If Range("D4").Value >= 1 And .Range("D4").Value <= 52 _
And Range("D6").Value = 1 Then
Range("E6").Value = ThisWorkbook.Worksheets("Data").Range("B47").Value
End If

If Range("D4").Value >= 1 And .Range("D4").Value <= 52 _
And Range("D6").Value = 2 Then
Range("E6").Value = ThisWorkbook.Worksheets("Data").Range("B48").Value
End If
End With
'__________________________________________________________________________ ________________
'
'Accommodation
'
With wksCalc
'D4 = weeks
'D7 = Accommodation option
If .Range("D4").Value >= 1 And .Range("D4").Value <= 52 _
And .Range("D7").Value >= 1 And .Range("D7") <= 61 Then

Select Case .Range("D7").Value
Case 1
strSourceSheetName = "Accommodation"
End Select

End If
End With
'__________________________________________________________________________ ________________
'
'Room Retainer
'
With wksCalc
'D9 = Room Retainer option
If Range("D9").Value = 1 Then
Range("E9").Value = ThisWorkbook.Worksheets("Data").Range("B55").Value
End If
End With
'__________________________________________________________________________ ________________
'
'Transfer
'
With wksCalc
'D3 = Group size
'D10 = Transfer
If .Range("D10") = 1 Then
Select Case .Range("D3").Value
Case 1
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("G61").Value
Case 2
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("G62").Value
Case 3
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("G63").Value
Case 4
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("G64").Value
Case 5
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("G65").Value
Case 6
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("G66").Value
Case 7
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("G67").Value
Case 8
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("G68").Value
End Select

Else

If .Range("D10") = 2 Then
Select Case .Range("D3").Value
Case 1
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("H61").Value
Case 2
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("H62").Value
Case 3
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("H63").Value
Case 4
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("H64").Value
Case 5
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("H65").Value
Case 6
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("H66").Value
Case 7
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("H67").Value
Case 8
Range("E10").Value = ThisWorkbook.Worksheets("Data").Range("H68").Value
End Select

End If

End If

End With

'__________________________________________________________________________ _____________


End Sub

Paul_Hossler
02-15-2012, 08:18 PM
'save typing
Set wksData = ThisWorkbook.Worksheets("Data")

With wksCalc
'D3 = Group size
'D10 = Transfer
If .Range("D10") = 1 Then
Select Case .Range("D3").Value
Case 1
' I really think you want the 'dot' in front of Range to use
' the With wksCalc
' Otherwise Range ("E10") will be against the Activesheet,
' which might not be the one you want
' Range("E10").Value = wksData.Range("G61").Value
.Range("E10").Value = wksData.Range("G61").Value
Case 2
.Range("E10").Value = wksData.Range("G62").Value






But you could also use the values in D3 and D10 as the number of rows and columns to 'shift' from a starting cell (I think I did this right)


With wksCalc
.Range("E10").Value = wksData.Range("F60").Offset(.Range("D3").Value, .Range("D10")).Value
End With


So if D3 = 4 and D10 = 2, you would go 4 rows down and 2 columns over from F10, and put H64 into E10

Paul

Paul_Hossler
02-15-2012, 08:40 PM
A few suggestions for less typing and more memory jogging:


Option Explicit
'Might want to define and set some Public variables for readability
Public ClassSize As Range
Public NumberOfWeeks As Range
Public CourseCode As Range
Public CourseCost As Range
Public ExamCode As Range
Public AccommodationCcode As Range
Public AdditionalNightCode As Range
Public RoomRetainer As Range
Public ExamCode2 As Range
Public TransferCode As Range
Public Total As Range

Sub JustMySuggestions()

'Init glocal variables
Set wksCalc = ThisWorkbook.Worksheets("Calc")
With wksCalc
Set ClassSize = Range("D3")
Set NumberOfWeeks = Range("D4")
Set CourseCode = Range("D5")
Set CourseCost = Range("E5")
'etc
End With

'so you could do something like
If ClassSize.Value = 6 And NumberOfWeeks.Value = 1 And CourseCode.Value = 1 Then
CourseCost.Value = Application.Workbooks("Book1").Worksheets("EWI").Range("B3").Value

End Sub



If Class size was not updated by your program, you could just use a Long and use the value


If ClassSize = 6 And NumberOfWeeks = 1 And CourseCode = 1 Then



Matter of style and personal opinion


By the way, I wasn't understand some of the code like this

Looks like D7 varies between 1 and 61, but in the Select statement, you only match on Case 1, so wouldn't strSourceSheetName = "" for 2 to 61?


'
'Accommodation
'
With wksCalc
'D4 = weeks
'D7 = Accommodation option
If .Range("D4").Value >= 1 And .Range("D4").Value <= 52 _
And .Range("D7").Value >= 1 And .Range("D7") <= 61 Then

Select Case .Range("D7").Value
Case 1
strSourceSheetName = "Accommodation"
End Select

End If
End With
'__________________________________________________________________________ ________________


Paul

TomKix
02-16-2012, 04:56 AM
Hi Paul,

Thanks for your suggestions, there is certainly some interesting reading here.

If I set the variables to public am I right in saying that will set them for the entire code as opposed to each section of code?

You are correct about the Accommodation code. I had just set that as a test to see if could get it to work and added the further case option of <=61 afterwards. If you take a quick look the attached workbook on the above post you'll see that in fact it's really a sequence of 1 to 7 with the option to use 11 to 13, 21 to 23, 51 and 61 as values to denote variation in the content of options 1,2,5 and 6.

Paul_Hossler
02-16-2012, 10:01 AM
Online help is pretty good about what's called 'Scope', which defines where you can use the variable



'----------------- Module1

Option Explicit

Public AllModules_AllSubs As Long
Dim ThisModule_AllSubs As Long
'
Sub Sub_1()
Dim OnlyThisModule As Long

OnlyThisModule = 2

ThisModule_AllSubs = 3

AllModules_AllSubs = 5

MsgBox (OnlyThisModule * ThisModule_AllSubs * AllModules_AllSubs)

End Sub

Sub Sub_2()
' OnlyThisModule = 2 '----- Variable Not Defined message

ThisModule_AllSubs = 3

AllModules_AllSubs = 5

MsgBox (ThisModule_AllSubs * AllModules_AllSubs)

End Sub


If Sub_1 were in Module2, it would fail because ThisModule_AllSubs is only visible in Module1

Personally, I try to avoid global variable unless necessary since they can make problems very hard to track down

You might want to modularize a lot by creating single purpose functions and sub that return data to a higher level. I find that it make debugging easier, as well as improves the readability

Paul

TomKix
02-18-2012, 05:27 PM
Ok, I've had a play around with the variables as laid out by Paul. Had some interesting results, some awful and some that make sense. Unfortunately I'm still not getting how I can arrange the code for the accommodation section to make it work.
Any ideas?

Paul_Hossler
02-19-2012, 12:10 PM
My gut feel is that you would make your life easier by modularizing

For example, the 17,000 lines of code could be replaced by a single user defined function (UDF)

It goes into a standard module, and is called just like a built-in excel function

In E5 the formula would be =CourseCodePrice(D3,D4,D5)



Option Explicit

' Cell E5 has =CourseCodePrice(D3,D4,D5)
Function CourseCodePrice(ClassSize As Long, NumberOfWeeks As Long, CourseCode As Long) As Variant

'default value
CourseCodePrice = CVErr(xlErrNA)

'check errors
Select Case CourseCode
Case 1 To 4
If (ClassSize < 6) Or (ClassSize > 15) Then Exit Function
If (NumberOfWeeks < 1) Or (NumberOfWeeks > 52) Then Exit Function

Case 5
If (ClassSize < 6) Or (ClassSize > 30) Then Exit Function
If ClassSize Mod 5 <> 0 Then Exit Function
If (NumberOfWeeks < 1) Or (NumberOfWeeks > 52) Then Exit Function

Case 6 To 7
If (ClassSize < 1) Or (ClassSize > 6) Then Exit Function
If (NumberOfWeeks < 1) Or (NumberOfWeeks > 52) Then Exit Function

Case Else
Exit Function
End Select



On Error Resume Next
Select Case CourseCode
Case 1, 4
CourseCodePrice = ThisWorkbook.Worksheets("EWI").Cells(NumberOfWeeks + 2, ClassSize - 4).Value
Case 2
CourseCodePrice = ThisWorkbook.Worksheets("EWC").Cells(NumberOfWeeks + 2, ClassSize - 4).Value
Case 3
CourseCodePrice = ThisWorkbook.Worksheets("EWPT").Cells(NumberOfWeeks + 2, ClassSize - 4).Value
Case 5
CourseCodePrice = ThisWorkbook.Worksheets("1_1 classes").Cells(NumberOfWeeks + 2, Int(ClassSize / 5) + 1).Value
Case 6
CourseCodePrice = ThisWorkbook.Worksheets("C6").Cells(NumberOfWeeks + 2, ClassSize + 1).Value
Case 7
CourseCodePrice = ThisWorkbook.Worksheets("C6C").Cells(NumberOfWeeks + 2, ClassSize + 1).Value
End Select
On Error GoTo 0
End Function



These is some simple error checking, but the other pieces of data you need could have their own UDF's also

Of course, you can also call a UDF from other VBA code if you want

Paul