1
\$\begingroup\$

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
\$\endgroup\$
1
\$\begingroup\$

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!

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.