Merge info from two sheets info one list The Next CEO of Stack OverflowCopying cells from one sheet to anotherRefer to other cells besides the one in the Cells.FindConsolidate used student hours into master sheet for exportCompare and find duplicates in 2 corresponding columns in 2 sheetsPairing all identifiers from 3 sheets and list them on a report worksheetA loop that assembles an Excel sheet by assembling matches from other sheetsMerging two Excel sheets by matching on two columnsSlow VBA macro using nested loops and autofilter to consolidate select data from 2 worksheets into 1Macro that combines data from multiple worksheetsCopying ranges from multiple Excel sheets into a main sheet
At which OSI layer a user-generated data resides?
Why has the US not been more assertive in confronting Russia in recent years?
What is the result of assigning to std::vector<T>::begin()?
Help understanding this unsettling image of Titan, Epimetheus, and Saturn's rings?
Non-deterministic sum of floats
Skipping indices in a product
Is it professional to write unrelated content in an almost-empty email?
Example of a Mathematician/Physicist whose Other Publications during their PhD eclipsed their PhD Thesis
How do I go from 300 unfinished/half written blog posts, to published posts?
What happens if you roll doubles 3 times then land on "Go to jail?"
How are problems classified in Complexity Theory?
Why do airplanes bank sharply to the right after air-to-air refueling?
Received an invoice from my ex-employer billing me for training; how to handle?
Would a galaxy be visible from outside, but nearby?
multiple labels for a single equation
Interfacing a button to MCU (and PC) with 50m long cable
Why don't programming languages automatically manage the synchronous/asynchronous problem?
How to avoid supervisors with prejudiced views?
What benefits would be gained by using human laborers instead of drones in deep sea mining?
How did the Bene Gesserit know how to make a Kwisatz Haderach?
Anatomically Correct Strange Women In Ponds Distributing Swords
Should I tutor a student who I know has cheated on their homework?
Between two walls
Is it my responsibility to learn a new technology in my own time my employer wants to implement?
Merge info from two sheets info one list
The Next CEO of Stack OverflowCopying cells from one sheet to anotherRefer to other cells besides the one in the Cells.FindConsolidate used student hours into master sheet for exportCompare and find duplicates in 2 corresponding columns in 2 sheetsPairing all identifiers from 3 sheets and list them on a report worksheetA loop that assembles an Excel sheet by assembling matches from other sheetsMerging two Excel sheets by matching on two columnsSlow VBA macro using nested loops and autofilter to consolidate select data from 2 worksheets into 1Macro that combines data from multiple worksheetsCopying ranges from multiple Excel sheets into a main sheet
$begingroup$
I've created a code that works but takes time to run.
Is there any way of making this code work in a more efficient way?
In short terms I want to:
make a new copy of sheet 1 and 2
Select the row with the lowest value in sheet 1
- Paste this row in sheet 3, and select item-number, rownumber and OP-number from this row
Delete copied row in sheet 1
Select row from sheet 2 with the same item-number, rownumber and that has the LOWEST rownumber
- Paste this row in sheet 3
- Delete copied row in sheet 2
Sheet 1 contains 34.000 rows and sheet 2 about 57.000 rows.
This means I'm making a lot of loops in this existing code, and I'm looking for any way to improve this code to work faster.
CODE:
Option Explicit
Sub SpecialCopy()
'~~> 1. Copy sheets to new locations
Dim lr_op As Long, lr_prod As Long, rng_cProd As Range, rng_cOp As Range
'~~> Copy products to new sheet
lr_prod = Sheets("ProdRows_Mo").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_cProd = Sheets("ProdRows_Mo").Range("A27:A" & lr_prod - 27)
rng_cProd.EntireRow.Copy Sheets("ProdRows_Mo_copy").Cells(1, 1).End(xlUp)(1)
'~~> Copy op to new sheet
lr_op = Sheets("OpRows_Mo").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_cOp = Sheets("OpRows_Mo").Range("A27:A" & lr_op - 27)
rng_cOp.EntireRow.Copy Sheets("OpRows_Mo_copy").Cells(1, 1).End(xlUp)(1)
'~~> End 1
'~~> 2. Loop op page for lowest value in "A"
'~~> Count rows in OpRows_copy
Dim rw As Range, rng_fOp As Range, item_mini As Long, item_no As Long, fetch_row As Long, op_no As Long
Dim j As Long, i As Range, vmin As Long, found As Range, item_no_comp As Long, pos_value As Integer, bel_to_op As Long
Do While j < lr_op
With Worksheets("OpRows_Mo_copy")
lr_op = Sheets("OpRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_fOp = Sheets("OpRows_Mo_copy").Range("A1:A" & lr_op)
vmin = Application.WorksheetFunction.Min(rng_fOp)
'MsgBox ("OP " & vmin & "-" & vmin)
Set i = Sheets("OpRows_Mo_copy").Range("A1:A" & lr_op).Find(what:=vmin, LookIn:=xlValues, lookat:=xlWhole)
item_no = .Cells(i.Row, 6).Value
op_no = .Cells(i.Row, 20).Value
fetch_row = i.Row
'Copy the other cells in the row containing the minimum value to the new worksheet.
Sheets("OpRows_Mo_copy").Cells(fetch_row, 1).EntireRow.Copy Sheets("ProdRows_PY").Cells(Rows.Count, 1).End(xlUp).Offset(1)
'Insert pos_value to copied row
If item_no_comp = item_no Then
pos_value = pos_value + 10
Else
pos_value = 10
item_no_comp = item_no
End If
Sheets("ProdRows_PY").Cells(Rows.Count, 5).End(xlUp).Offset(1).Value = pos_value
'Delete the "old" row
Sheets("OpRows_Mo_copy").Rows(fetch_row).Delete
'Set op-no row to
bel_to_op = pos_value
End With
'~~> End 2
'~~> 3. Loop prod page for the lowest value
Dim x As Range, y As Range, c_rows As Integer, row_no As Long, rng_fProd As Range, pos_no As Long, counter As Integer
'~~> Count rows in prodRows_copy
With Worksheets("ProdRows_Mo_copy")
Do
lr_prod = Sheets("ProdRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_fProd = Sheets("ProdRows_Mo_copy").Range("A1:A" & lr_prod)
For Each y In rng_fProd
If item_no = .Cells(y.Row, 7).Value And op_no = .Cells(y.Row, 14).Value Then
If pos_no = 0 Then
row_no = y.Row
pos_no = .Cells(y.Row, 12).Value
ElseIf pos_no > 0 And pos_no > .Cells(y.Row, 12).Value Then
row_no = y.Row
pos_no = .Cells(y.Row, 12).Value
End If
Else
End If
Next y
If pos_no = 0 Then
'endOfProd = True
Exit Do
Else
'Copy the other cells in the row containing the minimum value to the new worksheet.
Sheets("ProdRows_Mo_copy").Cells(row_no, 1).EntireRow.Copy Sheets("ProdRows_PY").Cells(Rows.Count, 1).End(xlUp).Offset(1)
'Insert PY-pos_value to copied row
If item_no_comp = item_no Then
pos_value = pos_value + 10
Else
pos_value = 10
item_no_comp = item_no
End If
Sheets("ProdRows_PY").Cells(Rows.Count, 5).End(xlUp).Offset(1).Value = pos_value
Sheets("ProdRows_PY").Cells(Rows.Count, 5).End(xlUp).Offset(0, -1).Value = bel_to_op
'Delete the "old" row
Sheets("ProdRows_Mo_copy").Rows(row_no).Delete
row_no = 0
pos_no = 0
End If
Loop
End With
lr_op = lr_op - 1
Loop
End Sub
vba excel
$endgroup$
bumped to the homepage by Community♦ 3 mins ago
This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.
add a comment |
$begingroup$
I've created a code that works but takes time to run.
Is there any way of making this code work in a more efficient way?
In short terms I want to:
make a new copy of sheet 1 and 2
Select the row with the lowest value in sheet 1
- Paste this row in sheet 3, and select item-number, rownumber and OP-number from this row
Delete copied row in sheet 1
Select row from sheet 2 with the same item-number, rownumber and that has the LOWEST rownumber
- Paste this row in sheet 3
- Delete copied row in sheet 2
Sheet 1 contains 34.000 rows and sheet 2 about 57.000 rows.
This means I'm making a lot of loops in this existing code, and I'm looking for any way to improve this code to work faster.
CODE:
Option Explicit
Sub SpecialCopy()
'~~> 1. Copy sheets to new locations
Dim lr_op As Long, lr_prod As Long, rng_cProd As Range, rng_cOp As Range
'~~> Copy products to new sheet
lr_prod = Sheets("ProdRows_Mo").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_cProd = Sheets("ProdRows_Mo").Range("A27:A" & lr_prod - 27)
rng_cProd.EntireRow.Copy Sheets("ProdRows_Mo_copy").Cells(1, 1).End(xlUp)(1)
'~~> Copy op to new sheet
lr_op = Sheets("OpRows_Mo").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_cOp = Sheets("OpRows_Mo").Range("A27:A" & lr_op - 27)
rng_cOp.EntireRow.Copy Sheets("OpRows_Mo_copy").Cells(1, 1).End(xlUp)(1)
'~~> End 1
'~~> 2. Loop op page for lowest value in "A"
'~~> Count rows in OpRows_copy
Dim rw As Range, rng_fOp As Range, item_mini As Long, item_no As Long, fetch_row As Long, op_no As Long
Dim j As Long, i As Range, vmin As Long, found As Range, item_no_comp As Long, pos_value As Integer, bel_to_op As Long
Do While j < lr_op
With Worksheets("OpRows_Mo_copy")
lr_op = Sheets("OpRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_fOp = Sheets("OpRows_Mo_copy").Range("A1:A" & lr_op)
vmin = Application.WorksheetFunction.Min(rng_fOp)
'MsgBox ("OP " & vmin & "-" & vmin)
Set i = Sheets("OpRows_Mo_copy").Range("A1:A" & lr_op).Find(what:=vmin, LookIn:=xlValues, lookat:=xlWhole)
item_no = .Cells(i.Row, 6).Value
op_no = .Cells(i.Row, 20).Value
fetch_row = i.Row
'Copy the other cells in the row containing the minimum value to the new worksheet.
Sheets("OpRows_Mo_copy").Cells(fetch_row, 1).EntireRow.Copy Sheets("ProdRows_PY").Cells(Rows.Count, 1).End(xlUp).Offset(1)
'Insert pos_value to copied row
If item_no_comp = item_no Then
pos_value = pos_value + 10
Else
pos_value = 10
item_no_comp = item_no
End If
Sheets("ProdRows_PY").Cells(Rows.Count, 5).End(xlUp).Offset(1).Value = pos_value
'Delete the "old" row
Sheets("OpRows_Mo_copy").Rows(fetch_row).Delete
'Set op-no row to
bel_to_op = pos_value
End With
'~~> End 2
'~~> 3. Loop prod page for the lowest value
Dim x As Range, y As Range, c_rows As Integer, row_no As Long, rng_fProd As Range, pos_no As Long, counter As Integer
'~~> Count rows in prodRows_copy
With Worksheets("ProdRows_Mo_copy")
Do
lr_prod = Sheets("ProdRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_fProd = Sheets("ProdRows_Mo_copy").Range("A1:A" & lr_prod)
For Each y In rng_fProd
If item_no = .Cells(y.Row, 7).Value And op_no = .Cells(y.Row, 14).Value Then
If pos_no = 0 Then
row_no = y.Row
pos_no = .Cells(y.Row, 12).Value
ElseIf pos_no > 0 And pos_no > .Cells(y.Row, 12).Value Then
row_no = y.Row
pos_no = .Cells(y.Row, 12).Value
End If
Else
End If
Next y
If pos_no = 0 Then
'endOfProd = True
Exit Do
Else
'Copy the other cells in the row containing the minimum value to the new worksheet.
Sheets("ProdRows_Mo_copy").Cells(row_no, 1).EntireRow.Copy Sheets("ProdRows_PY").Cells(Rows.Count, 1).End(xlUp).Offset(1)
'Insert PY-pos_value to copied row
If item_no_comp = item_no Then
pos_value = pos_value + 10
Else
pos_value = 10
item_no_comp = item_no
End If
Sheets("ProdRows_PY").Cells(Rows.Count, 5).End(xlUp).Offset(1).Value = pos_value
Sheets("ProdRows_PY").Cells(Rows.Count, 5).End(xlUp).Offset(0, -1).Value = bel_to_op
'Delete the "old" row
Sheets("ProdRows_Mo_copy").Rows(row_no).Delete
row_no = 0
pos_no = 0
End If
Loop
End With
lr_op = lr_op - 1
Loop
End Sub
vba excel
$endgroup$
bumped to the homepage by Community♦ 3 mins ago
This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.
$begingroup$
The guidelines for how to ask questions on Code Review are a little confusing, but generally speaking, questions should contain more larger picture context of what you use this code for. If you edit your question to follow this convention, you may get more views.
$endgroup$
– puzzlepiece87
Feb 27 at 19:49
add a comment |
$begingroup$
I've created a code that works but takes time to run.
Is there any way of making this code work in a more efficient way?
In short terms I want to:
make a new copy of sheet 1 and 2
Select the row with the lowest value in sheet 1
- Paste this row in sheet 3, and select item-number, rownumber and OP-number from this row
Delete copied row in sheet 1
Select row from sheet 2 with the same item-number, rownumber and that has the LOWEST rownumber
- Paste this row in sheet 3
- Delete copied row in sheet 2
Sheet 1 contains 34.000 rows and sheet 2 about 57.000 rows.
This means I'm making a lot of loops in this existing code, and I'm looking for any way to improve this code to work faster.
CODE:
Option Explicit
Sub SpecialCopy()
'~~> 1. Copy sheets to new locations
Dim lr_op As Long, lr_prod As Long, rng_cProd As Range, rng_cOp As Range
'~~> Copy products to new sheet
lr_prod = Sheets("ProdRows_Mo").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_cProd = Sheets("ProdRows_Mo").Range("A27:A" & lr_prod - 27)
rng_cProd.EntireRow.Copy Sheets("ProdRows_Mo_copy").Cells(1, 1).End(xlUp)(1)
'~~> Copy op to new sheet
lr_op = Sheets("OpRows_Mo").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_cOp = Sheets("OpRows_Mo").Range("A27:A" & lr_op - 27)
rng_cOp.EntireRow.Copy Sheets("OpRows_Mo_copy").Cells(1, 1).End(xlUp)(1)
'~~> End 1
'~~> 2. Loop op page for lowest value in "A"
'~~> Count rows in OpRows_copy
Dim rw As Range, rng_fOp As Range, item_mini As Long, item_no As Long, fetch_row As Long, op_no As Long
Dim j As Long, i As Range, vmin As Long, found As Range, item_no_comp As Long, pos_value As Integer, bel_to_op As Long
Do While j < lr_op
With Worksheets("OpRows_Mo_copy")
lr_op = Sheets("OpRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_fOp = Sheets("OpRows_Mo_copy").Range("A1:A" & lr_op)
vmin = Application.WorksheetFunction.Min(rng_fOp)
'MsgBox ("OP " & vmin & "-" & vmin)
Set i = Sheets("OpRows_Mo_copy").Range("A1:A" & lr_op).Find(what:=vmin, LookIn:=xlValues, lookat:=xlWhole)
item_no = .Cells(i.Row, 6).Value
op_no = .Cells(i.Row, 20).Value
fetch_row = i.Row
'Copy the other cells in the row containing the minimum value to the new worksheet.
Sheets("OpRows_Mo_copy").Cells(fetch_row, 1).EntireRow.Copy Sheets("ProdRows_PY").Cells(Rows.Count, 1).End(xlUp).Offset(1)
'Insert pos_value to copied row
If item_no_comp = item_no Then
pos_value = pos_value + 10
Else
pos_value = 10
item_no_comp = item_no
End If
Sheets("ProdRows_PY").Cells(Rows.Count, 5).End(xlUp).Offset(1).Value = pos_value
'Delete the "old" row
Sheets("OpRows_Mo_copy").Rows(fetch_row).Delete
'Set op-no row to
bel_to_op = pos_value
End With
'~~> End 2
'~~> 3. Loop prod page for the lowest value
Dim x As Range, y As Range, c_rows As Integer, row_no As Long, rng_fProd As Range, pos_no As Long, counter As Integer
'~~> Count rows in prodRows_copy
With Worksheets("ProdRows_Mo_copy")
Do
lr_prod = Sheets("ProdRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_fProd = Sheets("ProdRows_Mo_copy").Range("A1:A" & lr_prod)
For Each y In rng_fProd
If item_no = .Cells(y.Row, 7).Value And op_no = .Cells(y.Row, 14).Value Then
If pos_no = 0 Then
row_no = y.Row
pos_no = .Cells(y.Row, 12).Value
ElseIf pos_no > 0 And pos_no > .Cells(y.Row, 12).Value Then
row_no = y.Row
pos_no = .Cells(y.Row, 12).Value
End If
Else
End If
Next y
If pos_no = 0 Then
'endOfProd = True
Exit Do
Else
'Copy the other cells in the row containing the minimum value to the new worksheet.
Sheets("ProdRows_Mo_copy").Cells(row_no, 1).EntireRow.Copy Sheets("ProdRows_PY").Cells(Rows.Count, 1).End(xlUp).Offset(1)
'Insert PY-pos_value to copied row
If item_no_comp = item_no Then
pos_value = pos_value + 10
Else
pos_value = 10
item_no_comp = item_no
End If
Sheets("ProdRows_PY").Cells(Rows.Count, 5).End(xlUp).Offset(1).Value = pos_value
Sheets("ProdRows_PY").Cells(Rows.Count, 5).End(xlUp).Offset(0, -1).Value = bel_to_op
'Delete the "old" row
Sheets("ProdRows_Mo_copy").Rows(row_no).Delete
row_no = 0
pos_no = 0
End If
Loop
End With
lr_op = lr_op - 1
Loop
End Sub
vba excel
$endgroup$
I've created a code that works but takes time to run.
Is there any way of making this code work in a more efficient way?
In short terms I want to:
make a new copy of sheet 1 and 2
Select the row with the lowest value in sheet 1
- Paste this row in sheet 3, and select item-number, rownumber and OP-number from this row
Delete copied row in sheet 1
Select row from sheet 2 with the same item-number, rownumber and that has the LOWEST rownumber
- Paste this row in sheet 3
- Delete copied row in sheet 2
Sheet 1 contains 34.000 rows and sheet 2 about 57.000 rows.
This means I'm making a lot of loops in this existing code, and I'm looking for any way to improve this code to work faster.
CODE:
Option Explicit
Sub SpecialCopy()
'~~> 1. Copy sheets to new locations
Dim lr_op As Long, lr_prod As Long, rng_cProd As Range, rng_cOp As Range
'~~> Copy products to new sheet
lr_prod = Sheets("ProdRows_Mo").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_cProd = Sheets("ProdRows_Mo").Range("A27:A" & lr_prod - 27)
rng_cProd.EntireRow.Copy Sheets("ProdRows_Mo_copy").Cells(1, 1).End(xlUp)(1)
'~~> Copy op to new sheet
lr_op = Sheets("OpRows_Mo").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_cOp = Sheets("OpRows_Mo").Range("A27:A" & lr_op - 27)
rng_cOp.EntireRow.Copy Sheets("OpRows_Mo_copy").Cells(1, 1).End(xlUp)(1)
'~~> End 1
'~~> 2. Loop op page for lowest value in "A"
'~~> Count rows in OpRows_copy
Dim rw As Range, rng_fOp As Range, item_mini As Long, item_no As Long, fetch_row As Long, op_no As Long
Dim j As Long, i As Range, vmin As Long, found As Range, item_no_comp As Long, pos_value As Integer, bel_to_op As Long
Do While j < lr_op
With Worksheets("OpRows_Mo_copy")
lr_op = Sheets("OpRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_fOp = Sheets("OpRows_Mo_copy").Range("A1:A" & lr_op)
vmin = Application.WorksheetFunction.Min(rng_fOp)
'MsgBox ("OP " & vmin & "-" & vmin)
Set i = Sheets("OpRows_Mo_copy").Range("A1:A" & lr_op).Find(what:=vmin, LookIn:=xlValues, lookat:=xlWhole)
item_no = .Cells(i.Row, 6).Value
op_no = .Cells(i.Row, 20).Value
fetch_row = i.Row
'Copy the other cells in the row containing the minimum value to the new worksheet.
Sheets("OpRows_Mo_copy").Cells(fetch_row, 1).EntireRow.Copy Sheets("ProdRows_PY").Cells(Rows.Count, 1).End(xlUp).Offset(1)
'Insert pos_value to copied row
If item_no_comp = item_no Then
pos_value = pos_value + 10
Else
pos_value = 10
item_no_comp = item_no
End If
Sheets("ProdRows_PY").Cells(Rows.Count, 5).End(xlUp).Offset(1).Value = pos_value
'Delete the "old" row
Sheets("OpRows_Mo_copy").Rows(fetch_row).Delete
'Set op-no row to
bel_to_op = pos_value
End With
'~~> End 2
'~~> 3. Loop prod page for the lowest value
Dim x As Range, y As Range, c_rows As Integer, row_no As Long, rng_fProd As Range, pos_no As Long, counter As Integer
'~~> Count rows in prodRows_copy
With Worksheets("ProdRows_Mo_copy")
Do
lr_prod = Sheets("ProdRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
Set rng_fProd = Sheets("ProdRows_Mo_copy").Range("A1:A" & lr_prod)
For Each y In rng_fProd
If item_no = .Cells(y.Row, 7).Value And op_no = .Cells(y.Row, 14).Value Then
If pos_no = 0 Then
row_no = y.Row
pos_no = .Cells(y.Row, 12).Value
ElseIf pos_no > 0 And pos_no > .Cells(y.Row, 12).Value Then
row_no = y.Row
pos_no = .Cells(y.Row, 12).Value
End If
Else
End If
Next y
If pos_no = 0 Then
'endOfProd = True
Exit Do
Else
'Copy the other cells in the row containing the minimum value to the new worksheet.
Sheets("ProdRows_Mo_copy").Cells(row_no, 1).EntireRow.Copy Sheets("ProdRows_PY").Cells(Rows.Count, 1).End(xlUp).Offset(1)
'Insert PY-pos_value to copied row
If item_no_comp = item_no Then
pos_value = pos_value + 10
Else
pos_value = 10
item_no_comp = item_no
End If
Sheets("ProdRows_PY").Cells(Rows.Count, 5).End(xlUp).Offset(1).Value = pos_value
Sheets("ProdRows_PY").Cells(Rows.Count, 5).End(xlUp).Offset(0, -1).Value = bel_to_op
'Delete the "old" row
Sheets("ProdRows_Mo_copy").Rows(row_no).Delete
row_no = 0
pos_no = 0
End If
Loop
End With
lr_op = lr_op - 1
Loop
End Sub
vba excel
vba excel
edited Feb 15 at 11:53
skaul05
1052
1052
asked Feb 15 at 6:18
Mr CMr C
12
12
bumped to the homepage by Community♦ 3 mins ago
This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.
bumped to the homepage by Community♦ 3 mins ago
This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.
$begingroup$
The guidelines for how to ask questions on Code Review are a little confusing, but generally speaking, questions should contain more larger picture context of what you use this code for. If you edit your question to follow this convention, you may get more views.
$endgroup$
– puzzlepiece87
Feb 27 at 19:49
add a comment |
$begingroup$
The guidelines for how to ask questions on Code Review are a little confusing, but generally speaking, questions should contain more larger picture context of what you use this code for. If you edit your question to follow this convention, you may get more views.
$endgroup$
– puzzlepiece87
Feb 27 at 19:49
$begingroup$
The guidelines for how to ask questions on Code Review are a little confusing, but generally speaking, questions should contain more larger picture context of what you use this code for. If you edit your question to follow this convention, you may get more views.
$endgroup$
– puzzlepiece87
Feb 27 at 19:49
$begingroup$
The guidelines for how to ask questions on Code Review are a little confusing, but generally speaking, questions should contain more larger picture context of what you use this code for. If you edit your question to follow this convention, you may get more views.
$endgroup$
– puzzlepiece87
Feb 27 at 19:49
add a comment |
1 Answer
1
active
oldest
votes
$begingroup$
I'm seeing a few things you could do to improve your code. I won't focus just on speed considerations.
Explicitly qualify your objects. Don't let Excel make any assumptions about them. Letting Excel make assumptions leads to frustrating, unpredictable, hard to diagnose errors.
Before:Sheets()
After:ThisWorkbook.Sheets().
Use
Worksheets()instead ofSheets(), becauseSheets()can also refer toListObjectsI believe. This will help you avoid referring to the wrong object.
Before:.Sheets()
After:.Worksheets().
You should use multiple
Sub()s to accomplish this purpose. Your existing sub is too long and has too many variables. Using multiple subs will help you more quickly pinpoint errors and ease code reuse.
Before: Everything inSpecialCopy()
After:SpecialCopy()broken into multiple pieces, each of which has its ownSub()orFunction()with a descriptive name describing what it does. EachSub()orFunction()you create is stored in the same module and you execute those names insideSpecialCopy()to execute those code pieces.
You should use one variable on each line to make your code easier to read. Following the above recommendation to use multiple
Sub()s will help you have fewer variables active at a time, decreasing your memory footprint and eliminating the need to put multiple variables on the same line to conserve screen space.
Before:Dim rw As Range, rng_fOp As Range, item_mini As Long, item_no As Long, fetch_row As Long, op_no As Long
After:Dim rw As Rangeas one line with the rest on subsequent lines
If you have worksheets you know ahead of time you want to refer to, give them names in the Project Explorer.
Before:Sheets("ProdRows_Mo").Range
After: ProdRows_Mo.Range`
Get rid of
.End(xlUp)(1)after.Cells(1,1). It's not accomplishing anything.
Before:Sheets("OpRows_Mo_copy").Cells(1, 1).End(xlUp)(1)
After:Sheets("OpRows_Mo_copy").Cells(1, 1)
Indent code inside blocks. After using
For,For Each,Do While,With, etc., your next lines shouldn't be spaced with the same left margin.
Before:Do While j < lr_op/With Worksheets("OpRows_Mo_copy")/lr_op = Sheets("OpRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
After:Do While j < lr_op/ (indent)With Worksheets("OpRows_Mo_copy")/ (2x indent)lr_op = Sheets("OpRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
- Every time you want to cycle through every cell in a range, just read it into an array. Every time you refer to a cell's value on a worksheet via a range object, Excel has to read it from the worksheet, one of the more time-expensive operations it does. Instead, load the range into an array in memory, and you can quickly test every value without having to touch the worksheet. Here, you can use my function:
Private Function ConvertRangeToArray(ByVal rngInQuestion As Range) As Variant
Dim arrRangeToArray() As Variant
With rngInQuestion
If .Cells.Count = 1 Then
ReDim arrRangeToArray(1 To 1, 1 To 1)
arrRangeToArray(1, 1) = .Cells(1, 1).Value
Else
arrRangeToArray = .Value
End If
End With
ConvertRangeToArray = arrRangeToArray
End Function
This is enough to get you started. Good effort on your code, I look forward to seeing you improve further.
$endgroup$
add a comment |
Your Answer
StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");
StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");
StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);
else
createEditor();
);
function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader:
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
,
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);
);
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f213495%2fmerge-info-from-two-sheets-info-one-list%23new-answer', 'question_page');
);
Post as a guest
Required, but never shown
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
I'm seeing a few things you could do to improve your code. I won't focus just on speed considerations.
Explicitly qualify your objects. Don't let Excel make any assumptions about them. Letting Excel make assumptions leads to frustrating, unpredictable, hard to diagnose errors.
Before:Sheets()
After:ThisWorkbook.Sheets().
Use
Worksheets()instead ofSheets(), becauseSheets()can also refer toListObjectsI believe. This will help you avoid referring to the wrong object.
Before:.Sheets()
After:.Worksheets().
You should use multiple
Sub()s to accomplish this purpose. Your existing sub is too long and has too many variables. Using multiple subs will help you more quickly pinpoint errors and ease code reuse.
Before: Everything inSpecialCopy()
After:SpecialCopy()broken into multiple pieces, each of which has its ownSub()orFunction()with a descriptive name describing what it does. EachSub()orFunction()you create is stored in the same module and you execute those names insideSpecialCopy()to execute those code pieces.
You should use one variable on each line to make your code easier to read. Following the above recommendation to use multiple
Sub()s will help you have fewer variables active at a time, decreasing your memory footprint and eliminating the need to put multiple variables on the same line to conserve screen space.
Before:Dim rw As Range, rng_fOp As Range, item_mini As Long, item_no As Long, fetch_row As Long, op_no As Long
After:Dim rw As Rangeas one line with the rest on subsequent lines
If you have worksheets you know ahead of time you want to refer to, give them names in the Project Explorer.
Before:Sheets("ProdRows_Mo").Range
After: ProdRows_Mo.Range`
Get rid of
.End(xlUp)(1)after.Cells(1,1). It's not accomplishing anything.
Before:Sheets("OpRows_Mo_copy").Cells(1, 1).End(xlUp)(1)
After:Sheets("OpRows_Mo_copy").Cells(1, 1)
Indent code inside blocks. After using
For,For Each,Do While,With, etc., your next lines shouldn't be spaced with the same left margin.
Before:Do While j < lr_op/With Worksheets("OpRows_Mo_copy")/lr_op = Sheets("OpRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
After:Do While j < lr_op/ (indent)With Worksheets("OpRows_Mo_copy")/ (2x indent)lr_op = Sheets("OpRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
- Every time you want to cycle through every cell in a range, just read it into an array. Every time you refer to a cell's value on a worksheet via a range object, Excel has to read it from the worksheet, one of the more time-expensive operations it does. Instead, load the range into an array in memory, and you can quickly test every value without having to touch the worksheet. Here, you can use my function:
Private Function ConvertRangeToArray(ByVal rngInQuestion As Range) As Variant
Dim arrRangeToArray() As Variant
With rngInQuestion
If .Cells.Count = 1 Then
ReDim arrRangeToArray(1 To 1, 1 To 1)
arrRangeToArray(1, 1) = .Cells(1, 1).Value
Else
arrRangeToArray = .Value
End If
End With
ConvertRangeToArray = arrRangeToArray
End Function
This is enough to get you started. Good effort on your code, I look forward to seeing you improve further.
$endgroup$
add a comment |
$begingroup$
I'm seeing a few things you could do to improve your code. I won't focus just on speed considerations.
Explicitly qualify your objects. Don't let Excel make any assumptions about them. Letting Excel make assumptions leads to frustrating, unpredictable, hard to diagnose errors.
Before:Sheets()
After:ThisWorkbook.Sheets().
Use
Worksheets()instead ofSheets(), becauseSheets()can also refer toListObjectsI believe. This will help you avoid referring to the wrong object.
Before:.Sheets()
After:.Worksheets().
You should use multiple
Sub()s to accomplish this purpose. Your existing sub is too long and has too many variables. Using multiple subs will help you more quickly pinpoint errors and ease code reuse.
Before: Everything inSpecialCopy()
After:SpecialCopy()broken into multiple pieces, each of which has its ownSub()orFunction()with a descriptive name describing what it does. EachSub()orFunction()you create is stored in the same module and you execute those names insideSpecialCopy()to execute those code pieces.
You should use one variable on each line to make your code easier to read. Following the above recommendation to use multiple
Sub()s will help you have fewer variables active at a time, decreasing your memory footprint and eliminating the need to put multiple variables on the same line to conserve screen space.
Before:Dim rw As Range, rng_fOp As Range, item_mini As Long, item_no As Long, fetch_row As Long, op_no As Long
After:Dim rw As Rangeas one line with the rest on subsequent lines
If you have worksheets you know ahead of time you want to refer to, give them names in the Project Explorer.
Before:Sheets("ProdRows_Mo").Range
After: ProdRows_Mo.Range`
Get rid of
.End(xlUp)(1)after.Cells(1,1). It's not accomplishing anything.
Before:Sheets("OpRows_Mo_copy").Cells(1, 1).End(xlUp)(1)
After:Sheets("OpRows_Mo_copy").Cells(1, 1)
Indent code inside blocks. After using
For,For Each,Do While,With, etc., your next lines shouldn't be spaced with the same left margin.
Before:Do While j < lr_op/With Worksheets("OpRows_Mo_copy")/lr_op = Sheets("OpRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
After:Do While j < lr_op/ (indent)With Worksheets("OpRows_Mo_copy")/ (2x indent)lr_op = Sheets("OpRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
- Every time you want to cycle through every cell in a range, just read it into an array. Every time you refer to a cell's value on a worksheet via a range object, Excel has to read it from the worksheet, one of the more time-expensive operations it does. Instead, load the range into an array in memory, and you can quickly test every value without having to touch the worksheet. Here, you can use my function:
Private Function ConvertRangeToArray(ByVal rngInQuestion As Range) As Variant
Dim arrRangeToArray() As Variant
With rngInQuestion
If .Cells.Count = 1 Then
ReDim arrRangeToArray(1 To 1, 1 To 1)
arrRangeToArray(1, 1) = .Cells(1, 1).Value
Else
arrRangeToArray = .Value
End If
End With
ConvertRangeToArray = arrRangeToArray
End Function
This is enough to get you started. Good effort on your code, I look forward to seeing you improve further.
$endgroup$
add a comment |
$begingroup$
I'm seeing a few things you could do to improve your code. I won't focus just on speed considerations.
Explicitly qualify your objects. Don't let Excel make any assumptions about them. Letting Excel make assumptions leads to frustrating, unpredictable, hard to diagnose errors.
Before:Sheets()
After:ThisWorkbook.Sheets().
Use
Worksheets()instead ofSheets(), becauseSheets()can also refer toListObjectsI believe. This will help you avoid referring to the wrong object.
Before:.Sheets()
After:.Worksheets().
You should use multiple
Sub()s to accomplish this purpose. Your existing sub is too long and has too many variables. Using multiple subs will help you more quickly pinpoint errors and ease code reuse.
Before: Everything inSpecialCopy()
After:SpecialCopy()broken into multiple pieces, each of which has its ownSub()orFunction()with a descriptive name describing what it does. EachSub()orFunction()you create is stored in the same module and you execute those names insideSpecialCopy()to execute those code pieces.
You should use one variable on each line to make your code easier to read. Following the above recommendation to use multiple
Sub()s will help you have fewer variables active at a time, decreasing your memory footprint and eliminating the need to put multiple variables on the same line to conserve screen space.
Before:Dim rw As Range, rng_fOp As Range, item_mini As Long, item_no As Long, fetch_row As Long, op_no As Long
After:Dim rw As Rangeas one line with the rest on subsequent lines
If you have worksheets you know ahead of time you want to refer to, give them names in the Project Explorer.
Before:Sheets("ProdRows_Mo").Range
After: ProdRows_Mo.Range`
Get rid of
.End(xlUp)(1)after.Cells(1,1). It's not accomplishing anything.
Before:Sheets("OpRows_Mo_copy").Cells(1, 1).End(xlUp)(1)
After:Sheets("OpRows_Mo_copy").Cells(1, 1)
Indent code inside blocks. After using
For,For Each,Do While,With, etc., your next lines shouldn't be spaced with the same left margin.
Before:Do While j < lr_op/With Worksheets("OpRows_Mo_copy")/lr_op = Sheets("OpRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
After:Do While j < lr_op/ (indent)With Worksheets("OpRows_Mo_copy")/ (2x indent)lr_op = Sheets("OpRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
- Every time you want to cycle through every cell in a range, just read it into an array. Every time you refer to a cell's value on a worksheet via a range object, Excel has to read it from the worksheet, one of the more time-expensive operations it does. Instead, load the range into an array in memory, and you can quickly test every value without having to touch the worksheet. Here, you can use my function:
Private Function ConvertRangeToArray(ByVal rngInQuestion As Range) As Variant
Dim arrRangeToArray() As Variant
With rngInQuestion
If .Cells.Count = 1 Then
ReDim arrRangeToArray(1 To 1, 1 To 1)
arrRangeToArray(1, 1) = .Cells(1, 1).Value
Else
arrRangeToArray = .Value
End If
End With
ConvertRangeToArray = arrRangeToArray
End Function
This is enough to get you started. Good effort on your code, I look forward to seeing you improve further.
$endgroup$
I'm seeing a few things you could do to improve your code. I won't focus just on speed considerations.
Explicitly qualify your objects. Don't let Excel make any assumptions about them. Letting Excel make assumptions leads to frustrating, unpredictable, hard to diagnose errors.
Before:Sheets()
After:ThisWorkbook.Sheets().
Use
Worksheets()instead ofSheets(), becauseSheets()can also refer toListObjectsI believe. This will help you avoid referring to the wrong object.
Before:.Sheets()
After:.Worksheets().
You should use multiple
Sub()s to accomplish this purpose. Your existing sub is too long and has too many variables. Using multiple subs will help you more quickly pinpoint errors and ease code reuse.
Before: Everything inSpecialCopy()
After:SpecialCopy()broken into multiple pieces, each of which has its ownSub()orFunction()with a descriptive name describing what it does. EachSub()orFunction()you create is stored in the same module and you execute those names insideSpecialCopy()to execute those code pieces.
You should use one variable on each line to make your code easier to read. Following the above recommendation to use multiple
Sub()s will help you have fewer variables active at a time, decreasing your memory footprint and eliminating the need to put multiple variables on the same line to conserve screen space.
Before:Dim rw As Range, rng_fOp As Range, item_mini As Long, item_no As Long, fetch_row As Long, op_no As Long
After:Dim rw As Rangeas one line with the rest on subsequent lines
If you have worksheets you know ahead of time you want to refer to, give them names in the Project Explorer.
Before:Sheets("ProdRows_Mo").Range
After: ProdRows_Mo.Range`
Get rid of
.End(xlUp)(1)after.Cells(1,1). It's not accomplishing anything.
Before:Sheets("OpRows_Mo_copy").Cells(1, 1).End(xlUp)(1)
After:Sheets("OpRows_Mo_copy").Cells(1, 1)
Indent code inside blocks. After using
For,For Each,Do While,With, etc., your next lines shouldn't be spaced with the same left margin.
Before:Do While j < lr_op/With Worksheets("OpRows_Mo_copy")/lr_op = Sheets("OpRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
After:Do While j < lr_op/ (indent)With Worksheets("OpRows_Mo_copy")/ (2x indent)lr_op = Sheets("OpRows_Mo_copy").Cells(Rows.Count, 1).End(xlUp).Row
- Every time you want to cycle through every cell in a range, just read it into an array. Every time you refer to a cell's value on a worksheet via a range object, Excel has to read it from the worksheet, one of the more time-expensive operations it does. Instead, load the range into an array in memory, and you can quickly test every value without having to touch the worksheet. Here, you can use my function:
Private Function ConvertRangeToArray(ByVal rngInQuestion As Range) As Variant
Dim arrRangeToArray() As Variant
With rngInQuestion
If .Cells.Count = 1 Then
ReDim arrRangeToArray(1 To 1, 1 To 1)
arrRangeToArray(1, 1) = .Cells(1, 1).Value
Else
arrRangeToArray = .Value
End If
End With
ConvertRangeToArray = arrRangeToArray
End Function
This is enough to get you started. Good effort on your code, I look forward to seeing you improve further.
answered Feb 27 at 20:47
puzzlepiece87puzzlepiece87
328317
328317
add a comment |
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f213495%2fmerge-info-from-two-sheets-info-one-list%23new-answer', 'question_page');
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
$begingroup$
The guidelines for how to ask questions on Code Review are a little confusing, but generally speaking, questions should contain more larger picture context of what you use this code for. If you edit your question to follow this convention, you may get more views.
$endgroup$
– puzzlepiece87
Feb 27 at 19:49