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










0












$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









share|improve this question











$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















0












$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









share|improve this question











$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













0












0








0





$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









share|improve this question











$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






share|improve this question















share|improve this question













share|improve this question




share|improve this question








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
















  • $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










1 Answer
1






active

oldest

votes


















0












$begingroup$

I'm seeing a few things you could do to improve your code. I won't focus just on speed considerations.




  1. 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().




  1. Use Worksheets() instead of Sheets(), because Sheets() can also refer to ListObjects I believe. This will help you avoid referring to the wrong object.




    • Before: .Sheets()


    • After: .Worksheets().




  1. 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 in SpecialCopy()


    • After: SpecialCopy() broken into multiple pieces, each of which has its own Sub() or Function() with a descriptive name describing what it does. Each Sub() or Function() you create is stored in the same module and you execute those names inside SpecialCopy() to execute those code pieces.




  1. 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 Range as one line with the rest on subsequent lines




  1. 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`




  1. 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)




  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



  1. 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.






share|improve this answer









$endgroup$













    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
    );



    );













    draft saved

    draft discarded


















    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









    0












    $begingroup$

    I'm seeing a few things you could do to improve your code. I won't focus just on speed considerations.




    1. 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().




    1. Use Worksheets() instead of Sheets(), because Sheets() can also refer to ListObjects I believe. This will help you avoid referring to the wrong object.




      • Before: .Sheets()


      • After: .Worksheets().




    1. 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 in SpecialCopy()


      • After: SpecialCopy() broken into multiple pieces, each of which has its own Sub() or Function() with a descriptive name describing what it does. Each Sub() or Function() you create is stored in the same module and you execute those names inside SpecialCopy() to execute those code pieces.




    1. 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 Range as one line with the rest on subsequent lines




    1. 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`




    1. 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)




    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



    1. 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.






    share|improve this answer









    $endgroup$

















      0












      $begingroup$

      I'm seeing a few things you could do to improve your code. I won't focus just on speed considerations.




      1. 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().




      1. Use Worksheets() instead of Sheets(), because Sheets() can also refer to ListObjects I believe. This will help you avoid referring to the wrong object.




        • Before: .Sheets()


        • After: .Worksheets().




      1. 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 in SpecialCopy()


        • After: SpecialCopy() broken into multiple pieces, each of which has its own Sub() or Function() with a descriptive name describing what it does. Each Sub() or Function() you create is stored in the same module and you execute those names inside SpecialCopy() to execute those code pieces.




      1. 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 Range as one line with the rest on subsequent lines




      1. 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`




      1. 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)




      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



      1. 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.






      share|improve this answer









      $endgroup$















        0












        0








        0





        $begingroup$

        I'm seeing a few things you could do to improve your code. I won't focus just on speed considerations.




        1. 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().




        1. Use Worksheets() instead of Sheets(), because Sheets() can also refer to ListObjects I believe. This will help you avoid referring to the wrong object.




          • Before: .Sheets()


          • After: .Worksheets().




        1. 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 in SpecialCopy()


          • After: SpecialCopy() broken into multiple pieces, each of which has its own Sub() or Function() with a descriptive name describing what it does. Each Sub() or Function() you create is stored in the same module and you execute those names inside SpecialCopy() to execute those code pieces.




        1. 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 Range as one line with the rest on subsequent lines




        1. 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`




        1. 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)




        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



        1. 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.






        share|improve this answer









        $endgroup$



        I'm seeing a few things you could do to improve your code. I won't focus just on speed considerations.




        1. 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().




        1. Use Worksheets() instead of Sheets(), because Sheets() can also refer to ListObjects I believe. This will help you avoid referring to the wrong object.




          • Before: .Sheets()


          • After: .Worksheets().




        1. 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 in SpecialCopy()


          • After: SpecialCopy() broken into multiple pieces, each of which has its own Sub() or Function() with a descriptive name describing what it does. Each Sub() or Function() you create is stored in the same module and you execute those names inside SpecialCopy() to execute those code pieces.




        1. 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 Range as one line with the rest on subsequent lines




        1. 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`




        1. 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)




        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



        1. 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.







        share|improve this answer












        share|improve this answer



        share|improve this answer










        answered Feb 27 at 20:47









        puzzlepiece87puzzlepiece87

        328317




        328317



























            draft saved

            draft discarded
















































            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.




            draft saved


            draft discarded














            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





















































            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







            Popular posts from this blog

            बाताम इन्हें भी देखें सन्दर्भ दिक्चालन सूची1°05′00″N 104°02′0″E / 1.08333°N 104.03333°E / 1.08333; 104.033331°05′00″N 104°02′0″E / 1.08333°N 104.03333°E / 1.08333; 104.03333

            Why is the 'in' operator throwing an error with a string literal instead of logging false?Why can't I use switch statement on a String?Python join: why is it string.join(list) instead of list.join(string)?Multiline String Literal in C#Why does comparing strings using either '==' or 'is' sometimes produce a different result?How to initialize an array's length in javascript?How can I print literal curly-brace characters in python string and also use .format on it?Why does ++[[]][+[]]+[+[]] return the string “10”?Why is char[] preferred over String for passwords?Why does this code using random strings print “hello world”?jQuery.inArray(), how to use it right?

            How can we generalize the fact of finite dimensional vector space to an infinte dimensional case?$k[x]$-module and cyclic module over a finite dimensional vector spaceSubspace of a finite dimensional space is finite dimensionalIf V is an infinite-dimensional vector space, and S is an infinite-dimensional subspace of V, must the dimension of V/S be finite? ExplainWhy is an infinite dimensional space so different than a finite dimensional one?base for finite dimensional vector space is not infinite dimensional vector space?Any finite-dimensional vector space is the dual space of anotherHaving Trouble Understanding Meaning Of A Finite-Dimensional Vector SpaceProve that “Every subspaces of a finite-dimensional vector space is finite-dimensional”Ring as a finite dimensional Vector space over a field KQuestion regarding basis and dimension