# Sending data from sheet to other sheet in Excel (from a dispatch note to some records)

I have this code to save data into some records. It is a little slow, so how can I make it faster?

To the first record (sheet MOV MERCADERIA) it copies 23 columns and from 1 to 29 rows. The problem is when the dispatch note is complete (when the 29 rows are full).

And to the second record (sheet CONCAT) it copies 4 columns and inserts some formulas to other 9 columns from 1 to 29 rows. And then It removes the duplicates from the sheet CONCAT as seemed in the code.

 Sub GUARDARREMITO()

Application.ScreenUpdating = False

Sheets("Remito").Select
Range("B11").Select

While ActiveCell.Value <> ""
ActiveCell.Offset(0, 3).Select
If ActiveCell = "" Then
Exit Sub
End If
ActiveCell.Offset(1, -3).Select

Wend

Range("B5").Select
FECHA = ActiveCell.Value
ActiveCell.Offset(1, 0).Select
NROREMITO = ActiveCell.Value
ActiveCell.Offset(1, 0).Select
CTIPOREMITO = ActiveCell.Value
ActiveCell.Offset(0, 1).Select
TIPOREMITO = ActiveCell.Value
ActiveCell.Offset(1, -1).Select
CPROVEEDOR = ActiveCell.Value
ActiveCell.Offset(0, 1).Select
PROVEEDOR = ActiveCell.Value
ActiveCell.Offset(1, -1).Select
CRESPONSABLE = ActiveCell.Value
ActiveCell.Offset(0, 1).Select
RESPONSABLE = ActiveCell.Value
ActiveCell.Offset(34, -1).Select
OBSERVACIONES = ActiveCell.Value
ActiveCell.Offset(-2, 3).Select
TOTART = ActiveCell.Value
ActiveCell.Offset(0, 6).Select
CMVTOT = ActiveCell.Value
ActiveCell.Offset(-30, 11).Select
ITEMTOT = ActiveCell.Value

Range("B11").Select

While ActiveCell.Value <> ""
CODART = ActiveCell.Value
ActiveCell.Offset(0, 1).Select
CODCLR = ActiveCell.Value
ActiveCell.Offset(0, 1).Select
TALLE = ActiveCell.Value
ActiveCell.Offset(0, 1).Select
ActiveCell.Offset(0, 1).Select
DESCRIPCION = ActiveCell.Value
ActiveCell.Offset(0, 1).Select
CLR = ActiveCell.Value
ActiveCell.Offset(0, 2).Select
CONCATENAR = ActiveCell.Value
ActiveCell.Offset(0, 1).Select
CMVUNIT = ActiveCell.Value
ActiveCell.Offset(0, 1).Select
CMVCANT = ActiveCell.Value
ActiveCell.Offset(0, 1).Select
PVPUNIT = ActiveCell.Value
ActiveCell.Offset(0, 1).Select
ITEM1 = ActiveCell.Value
ActiveCell.Offset(1, -11).Select

Range("A2").Select

While ActiveCell.Value <> ""
ActiveCell.Offset(1, 0).Select
Wend

ActiveCell.Value = NROREMITO
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = FECHA
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = CTIPOREMITO
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = TIPOREMITO
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = CPROVEEDOR
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = PROVEEDOR
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = CODART
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = DESCRIPCION
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = CODCLR
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = CLR
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = TALLE
ActiveCell.Offset(0, 1).Select
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = CONCATENAR
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = CMVUNIT
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = CMVCANT
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = PVPUNIT
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = ITEM1
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = ITEMTOT
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = CMVTOT
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = TOTART
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = CRESPONSABLE
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = RESPONSABLE
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = OBSERVACIONES
Sheets("Remito").Select
Range(DIREC1).Select

Sheets("CONCAT").Select
Range("A2").Select

While ActiveCell.Value <> ""
ActiveCell.Offset(1, 0).Select
Wend

ActiveCell.Value = CONCATENAR
ActiveCell.Offset(0, 1).Select
ActiveCell.Formula = "=VLOOKUP(INDIRECT(ADDRESS(ROW(),COLUMN()+1)),ARTICULOS!$A:$D,4,FALSE)"
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = CODART
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = CODCLR
ActiveCell.Offset(0, 1).Select
ActiveCell.Value = TALLE
ActiveCell.Offset(0, 1).Select
ActiveCell.Formula = "=IFERROR(VLOOKUP(INDIRECT(ADDRESS(ROW(),COLUMN()-2)),COLORES!$A:$B,2,FALSE),"""")"
ActiveCell.Offset(0, 1).Select
ActiveCell.Formula = "=VLOOKUP(INDIRECT(ADDRESS(ROW(),COLUMN()-4)),ARTICULOS!$A:$F,6,FALSE)"
ActiveCell.Offset(0, 1).Select
ActiveCell.Formula = "=VLOOKUP(INDIRECT(ADDRESS(ROW(),COLUMN()-5)),ARTICULOS!$A:$G,7,FALSE)"
ActiveCell.Offset(0, 1).Select
ActiveCell.Formula = 1
ActiveCell.Offset(0, 1).Select
ActiveCell.Formula = "=VLOOKUP(INDIRECT(ADDRESS(ROW(),COLUMN()-7)),ARTICULOS!$A:$H,8,FALSE)"
ActiveCell.Offset(0, 1).Select
ActiveCell.Formula = "=VLOOKUP(INDIRECT(ADDRESS(ROW(),COLUMN()-8)),ARTICULOS!$A:$I,9,FALSE)"
ActiveCell.Offset(0, 1).Select
ActiveCell.Formula = "=VLOOKUP(INDIRECT(ADDRESS(ROW(),COLUMN()-9)),ARTICULOS!$A:$B,2,FALSE)"
ActiveCell.Offset(0, 1).Select
ActiveCell.Formula = "=VLOOKUP(INDIRECT(ADDRESS(ROW(),COLUMN()-10)),ARTICULOS!$A:$C,3,FALSE)"
Sheets("Remito").Select
Range(DIREC1).Select

With Sheets("CONCAT")

numFilas = .Cells(.Rows.Count, 1).End(xlUp).Row

For i = numFilas To 1 Step -1
If WorksheetFunction.CountIf(.Range("A:A"), .Cells(i, 1)) > 1 Then
.Rows(i).Delete
End If
Next i

End With

Wend

Sheets("Remito").Select
Range("B13").Select

[B6] = Val([B6]) + 1

End Sub


First things first - I assume you used the macro recorder to create this. Maybe you recorded one thing and then wrote the rest based on that. I want to congratulate you on trying this out! It's pretty much how we all start. I also want to welcome you to code review and give you more congrats on wanting to get better at this! But, you came here and want advice, so I'm going to write it. I hope it doesn't come off as condescending or mean, and if it does - I apologize, that's not my intent.

### Variables

You haven't defined any of your variables -

code inspection courtesy of VBA Rubberduck - a VBE add-in maintained by the code review community

It's great that you're using variables, but you need to define them and give them a type!

When you don't define your variable, VBA will declare it as a Variant type that can hold any type of data. While this may be more flexible, it adds processing time to your macro as VBA decides or tests for the type. Additionally, since a Variant can be any type of data, you may miss out on valuable troubleshooting information on Type Mismatch

So if FECHA is a text value, you need

Dim fecha As String


If it's a date or time, pick the right type. One thing to remember is integers are obsolete. According to msdn VBA silently converts all integers to long.

One way to ensure you've declared all your variables is using Option Explicit. 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.

That being said, I don't know my (Spanish?) too well - obviously FECHA is probably date and RESPONSABLE might be a person or department. But then, what's CRESPONSABLE? How is it different than RESPONSABLE? Always give your variables meaningful names - and use entire words. I'll make the assumption that maybe this is an employee that does a task and the other is that employee's supervisor. Maybe better names for your variables would be -

Dim responsibleParty As String
Dim responsiblePartySupervisor As String


Then when you're assigning these variables values in the code, you (and I) will know what type of data that should be as well as what type of information is held in that variables. "Oh that's Joe and Steve from Shipping."

You also have all your variables in ALLCAPS - Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names. Your ALLCAPS come off as constants to me.

### Speed

This is slow you say? Well, even if you didn't, I would know it is because of your usage of .Select. Be sure to avoid things like .Select - it just slows the code down by needing to fiddle with the spreadsheet while doing everything else behind the scenes. There's a good question on StackOverflow addressing this.

Essentially there's no reason to write a macro that uses sheet commands like select and offset. For instance this little block here -

Range("B5").Select
FECHA = ActiveCell.Value
ActiveCell.Offset(1, 0).Select
NROREMITO = ActiveCell.Value
ActiveCell.Offset(1, 0).Select
CTIPOREMITO = ActiveCell.Value
ActiveCell.Offset(0, 1).Select
TIPOREMITO = ActiveCell.Value
ActiveCell.Offset(1, -1).Select
CPROVEEDOR = ActiveCell.Value
ActiveCell.Offset(0, 1).Select
PROVEEDOR = ActiveCell.Value
ActiveCell.Offset(1, -1).Select
CRESPONSABLE = ActiveCell.Value
ActiveCell.Offset(0, 1).Select
RESPONSABLE = ActiveCell.Value
ActiveCell.Offset(34, -1).Select
OBSERVACIONES = ActiveCell.Value
ActiveCell.Offset(-2, 3).Select
TOTART = ActiveCell.Value
ActiveCell.Offset(0, 6).Select
CMVTOT = ActiveCell.Value
ActiveCell.Offset(-30, 11).Select
ITEMTOT = ActiveCell.Value


could be rewritten

Dim startColumn As Long
startColumn = 2
Dim startRow As Long
startRow = 5
With Sheet1
FECHA = .Cells(startRow, startColumn).Value
NROREMITO = .Cells(startRow + 1, startColumn).Value
CTIPOREMITO = .Cells(startRow + 2, startColumn).Value
TIPOREMITO = .Cells(startRow + 2, startColumn + 1).Value
CPROVEEDOR = .Cells(startRow + 3, startColumn).Value
PROVEEDOR = .Cells(startRow + 3, startColumn + 1).Value
CRESPONSABLE = .Cells(startRow + 4, startColumn).Value
RESPONSABLE = .Cells(startRow + 4, startColumn + 1).Value
OBSERVACIONES = .Cells(startRow + 38, startColumn).Value
TOTART = .Cells(startRow + 36, startColumn + 3).Value
CMVTOT = .Cells(startRow + 36, startColumn + 9).Value
ITEMTOT = .Cells(startRow + 6, startColumn + 2).Value
End With


Right, but that's not very clear. If my calculations are correct (which they may not be) you want cells B5:B9, C7:C9, D11, B43, E41, K41. Which you could put in there instead of all that row+i, column+j.

But more likely, you'd benefit from an array. Something like

Dim data As Variant
ReDim data(1 To 12)
Dim dataRange As Range
Set dataRange = Union(Sheet1.Range("B5:B9"), Sheet1.Range("C7:c9"), Sheet1.Range("D11"), Sheet1.Range("B43"), Sheet1.Range("E41"), Sheet1.Range("K41"))
Dim dataCell As Range
Dim index As Long
index = 1
For Each dataCell In dataRange
data(index) = dataCell.Value
index = index + 1
Next


This isn't the best way to do it, but I'm kind of going through this process in full to make it more clear, I would skip all the intermediate steps, personally, but that might be too much to bite off at once.

### Formulas

I see you're writing formulas. Those are okay if you need formulas at the end, but if you don't need the formulas, just write the values.

But let's talk about formulas - some formulas are *volatile - which is to say they are not stable. They recalculate everytime something on the sheet changes. Something like =Rand() will recalculate everytime anything changes. That's slow, isn't it? RAND is very volatile - but others are volatile too - especially OFFSET and INDIRECT. These formulas slow worksheets down like crazy! Avoid them, always, if possible.

If you cannot avoid them (which I bet you can), then you need to make it so they aren't always recalculated, especially not during your macro. Do this by sandwiching your macro with

Application.Calculate = xlManual
'code
Application.Calculate = xlAutomatic


Now nothing will recalculate while you're running your code.

### Misc

Worksheets have a CodeName property - View Properties window (F4) and the (Name) field (the one at the top) can be used as the worksheet name. This way you can avoid Sheets("CONCAT") and instead just use concatSheet.

Instead of hard-coding ranges (like I did), if they are static, assign them a named range property. e.g. instead of Sheets("mySheet").Range("A1:A10") you can have mysheet.Range("MyNamedRange").

 While ActiveCell.Value <> ""


Instead of "" use the built-in constant vbNullString.

Speaking of this -

While ActiveCell.Value <> ""
ActiveCell.Offset(1, 0).Select
Wend


You can find the last row like this

Dim lastRow As Long
lastRow = concatSheet.Cells(concatSheet.Rows.Count, 1).End(xlUp).Row


There is a standard way to find lastRow and lastColumn. That post explains why.

Always be explicit in your references - don't take what sheet or book you're in for granted

Lastly, this syntactic sugar -

[B6] = Val([B6]) + 1


Is, in general, bad practice. Be explicit!

• Nice job of walking through the basics and explaining why. – FreeMan Sep 9 '18 at 13:46
• Thank you, man. But what should I do with this part ActiveCell.Offset(1, -11).Select DIREC1 = ActiveCell.Address – JUAN CARLOS PERON Sep 10 '18 at 16:10
• @JUANCARLOSPERON well if you need the address, just assign the cell to a range variable? – Raystafarian Sep 10 '18 at 20:35