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

Question

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
 MsgBox "FALTAN INGRESAR CANTIDADES"
 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
 CANTIDAD = ActiveCell.Value
 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
 DIREC1 = ActiveCell.Address
 Sheets("MOV MERCADERIA").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.Value = CANTIDAD
 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

Answer 1

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 -

enter image description here _{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 enter image description here

Lastly, this syntactic sugar -

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

Is, in general, bad practice. Be explicit!

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