Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

#Variables

I don't see any of your variables defined. (That's like blasphemy here)

  1. Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

  2. When you don't define your variable, VBA will declare it as a Variant, which are objects:

Performance. A variable you declare with the Object type is flexible enough to contain a reference to any object. However, when you invoke a method or property on such a variable, you always incur late binding (at run time). To force early binding (at compile time) and better performance, declare the variable with a specific class name, or cast it to the specific data type.

By not declaring variables, you could possibly be paying a penalty.

  1. Variable names - give your variables meaningful names.

  2. Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

  3. Since you have something like irow, I'll just throw this in as well - Integers - integers are obsolete integers are obsolete. According to msdn VBA silently converts all integers to long.


###Your function

 Function findcol(Text)

Should be

Private Function LocateColumnByName(ByVal columnName As String) As Long
 Const SEARCH_ROW As Long = 9
 Dim foundColumn As Long
 On Error GoTo errHandler
 foundColumn = Sheet1.Cells(SEARCH_ROW, 1).EntireRow.Find(What:=columnName, LookIn:=xlValues, lookat:=xlPart).Column
 LocateColumnByName = foundColumn
 Exit Function
 
errHandler:
 MsgBox "Error : '" & columnName & "' column not found. Please report this error to the global team"
End Function

You'll need to handle the return of nothing in the main sub, or have the error assign 0 and handle that.

#Variables

I don't see any of your variables defined. (That's like blasphemy here)

  1. Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

  2. When you don't define your variable, VBA will declare it as a Variant, which are objects:

Performance. A variable you declare with the Object type is flexible enough to contain a reference to any object. However, when you invoke a method or property on such a variable, you always incur late binding (at run time). To force early binding (at compile time) and better performance, declare the variable with a specific class name, or cast it to the specific data type.

By not declaring variables, you could possibly be paying a penalty.

  1. Variable names - give your variables meaningful names.

  2. Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

  3. Since you have something like irow, I'll just throw this in as well - Integers - integers are obsolete. According to msdn VBA silently converts all integers to long.


###Your function

 Function findcol(Text)

Should be

Private Function LocateColumnByName(ByVal columnName As String) As Long
 Const SEARCH_ROW As Long = 9
 Dim foundColumn As Long
 On Error GoTo errHandler
 foundColumn = Sheet1.Cells(SEARCH_ROW, 1).EntireRow.Find(What:=columnName, LookIn:=xlValues, lookat:=xlPart).Column
 LocateColumnByName = foundColumn
 Exit Function
 
errHandler:
 MsgBox "Error : '" & columnName & "' column not found. Please report this error to the global team"
End Function

You'll need to handle the return of nothing in the main sub, or have the error assign 0 and handle that.

#Variables

I don't see any of your variables defined. (That's like blasphemy here)

  1. Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

  2. When you don't define your variable, VBA will declare it as a Variant, which are objects:

Performance. A variable you declare with the Object type is flexible enough to contain a reference to any object. However, when you invoke a method or property on such a variable, you always incur late binding (at run time). To force early binding (at compile time) and better performance, declare the variable with a specific class name, or cast it to the specific data type.

By not declaring variables, you could possibly be paying a penalty.

  1. Variable names - give your variables meaningful names.

  2. Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

  3. Since you have something like irow, I'll just throw this in as well - Integers - integers are obsolete. According to msdn VBA silently converts all integers to long.


###Your function

 Function findcol(Text)

Should be

Private Function LocateColumnByName(ByVal columnName As String) As Long
 Const SEARCH_ROW As Long = 9
 Dim foundColumn As Long
 On Error GoTo errHandler
 foundColumn = Sheet1.Cells(SEARCH_ROW, 1).EntireRow.Find(What:=columnName, LookIn:=xlValues, lookat:=xlPart).Column
 LocateColumnByName = foundColumn
 Exit Function
 
errHandler:
 MsgBox "Error : '" & columnName & "' column not found. Please report this error to the global team"
End Function

You'll need to handle the return of nothing in the main sub, or have the error assign 0 and handle that.

added 560 characters in body
Source Link
Raystafarian
  • 7.3k
  • 1
  • 23
  • 60

#Variables

I don't see any of your variables defined. (That's like blasphemy here)

  1. Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

  2. When you don't define your variable, VBA will declare it as a Variant, which are objects:

Performance. A variable you declare with the Object type is flexible enough to contain a reference to any object. However, when you invoke a method or property on such a variable, you always incur late binding (at run time). To force early binding (at compile time) and better performance, declare the variable with a specific class name, or cast it to the specific data type.

By not declaring variables, you could possibly be paying a penalty.

  1. Variable names - give your variables meaningful names.

  2. Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

  3. Since you have something like irow, I'll just throw this in as well - Integers - integers are obsolete. According to msdn VBA silently converts all integers to long.


###Your function

 Function findcol(Text)

Should be

Private Function LocateColumnByName(ByVal columnName asAs String) asAs Long
 Const SEARCH_ROW As Long = 9
 Dim foundColumn As Long
 On Error GoTo errHandler
 foundColumn = Sheet1.Cells(SEARCH_ROW, 1).EntireRow.Find(What:=columnName, LookIn:=xlValues, lookat:=xlPart).Column
 LocateColumnByName = foundColumn
 Exit Function
 
errHandler:
 MsgBox "Error : '" & columnName & "' column not found. Please report this error to the global team"
End Function

You'll need to handle the return of nothing in the main sub, or have the error assign 0 and handle that.

#Variables

I don't see any of your variables defined. (That's like blasphemy here)

  1. Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

  2. When you don't define your variable, VBA will declare it as a Variant, which are objects:

Performance. A variable you declare with the Object type is flexible enough to contain a reference to any object. However, when you invoke a method or property on such a variable, you always incur late binding (at run time). To force early binding (at compile time) and better performance, declare the variable with a specific class name, or cast it to the specific data type.

By not declaring variables, you could possibly be paying a penalty.

  1. Variable names - give your variables meaningful names.

  2. Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

  3. Since you have something like irow, I'll just throw this in as well - Integers - integers are obsolete. According to msdn VBA silently converts all integers to long.


###Your function

 Function findcol(Text)

Should be

Private Function LocateColumnByName(ByVal columnName as String) as Long

#Variables

I don't see any of your variables defined. (That's like blasphemy here)

  1. Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

  2. When you don't define your variable, VBA will declare it as a Variant, which are objects:

Performance. A variable you declare with the Object type is flexible enough to contain a reference to any object. However, when you invoke a method or property on such a variable, you always incur late binding (at run time). To force early binding (at compile time) and better performance, declare the variable with a specific class name, or cast it to the specific data type.

By not declaring variables, you could possibly be paying a penalty.

  1. Variable names - give your variables meaningful names.

  2. Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

  3. Since you have something like irow, I'll just throw this in as well - Integers - integers are obsolete. According to msdn VBA silently converts all integers to long.


###Your function

 Function findcol(Text)

Should be

Private Function LocateColumnByName(ByVal columnName As String) As Long
 Const SEARCH_ROW As Long = 9
 Dim foundColumn As Long
 On Error GoTo errHandler
 foundColumn = Sheet1.Cells(SEARCH_ROW, 1).EntireRow.Find(What:=columnName, LookIn:=xlValues, lookat:=xlPart).Column
 LocateColumnByName = foundColumn
 Exit Function
 
errHandler:
 MsgBox "Error : '" & columnName & "' column not found. Please report this error to the global team"
End Function

You'll need to handle the return of nothing in the main sub, or have the error assign 0 and handle that.

Source Link
Raystafarian
  • 7.3k
  • 1
  • 23
  • 60

#Variables

I don't see any of your variables defined. (That's like blasphemy here)

  1. Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

  2. When you don't define your variable, VBA will declare it as a Variant, which are objects:

Performance. A variable you declare with the Object type is flexible enough to contain a reference to any object. However, when you invoke a method or property on such a variable, you always incur late binding (at run time). To force early binding (at compile time) and better performance, declare the variable with a specific class name, or cast it to the specific data type.

By not declaring variables, you could possibly be paying a penalty.

  1. Variable names - give your variables meaningful names.

  2. Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

  3. Since you have something like irow, I'll just throw this in as well - Integers - integers are obsolete. According to msdn VBA silently converts all integers to long.


###Your function

 Function findcol(Text)

Should be

Private Function LocateColumnByName(ByVal columnName as String) as Long
lang-vb

AltStyle によって変換されたページ (->オリジナル) /