HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Accelerate the process of filling the quality control table

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
qualitytheprocessacceleratecontrolfillingtable

Problem

I'm working on a report of quality control. I've got a table, with all the points of control inside, and each time, when I get a new test result, Excel should fill this table. My code already works, but slowly (I have 15000 lines in this table, and it takes me 4 minutes to fill it), so I wish I can accelerate a little bit.

Here is my general idea:

The results of one test is composted by nearly 3000 .txt files, and I have a list L which contains the names of all these files that I should open and import the data into Excel. I open the list L, and open the .txt file one by one, to import the results into a sheet "Brouillon" all at once, then fill the table with its help. I think it could be faster.

'to chose the list L
Private Sub Button_Parcourir_Click()

With Application.FileDialog(msoFileDialogOpen)
.AllowMultiSelect = False
.InitialFileName = "import_excel.txt"
.Show
listPath = .SelectedItems(1)
End With
TextBox1.Text = listPath
End Sub


```
Private Sub Button_Importer_Click()

'create a sheet named "Brouillon":
Sheets.Add After:=Sheets(Sheets.Count)
Sheets(Sheets.Count).Name = "Brouillon"
Range("A1").Select

'open the list L and import the data into "Brouillon":
list_de_Controle = "TEXT;" & listPath
Open listPath For Input As #1
Do While Not EOF(1)
Line Input #1, nom_de_Fich
mfile = Dir(nom_de_Fich & ".")

If mfile <> "" Then
Open nom_de_Fich For Input As #2
Fich_dansList = Fich_dansList & nom_de_Fich & "|"
Inserer_contenu
Close #2
End If
Loop
Close #1

'count how many lines and columns we have in "Data":
Worksheets(Array(1)).Select
Range("A3").Select
ActiveCell.End(xlDown).Select
ActiveCell.End(xlToRight).Select
ligne_Data = Selection.Row
ma_Colonne = Selection.Column + 1

'count how many lines and columns we have in "Brouillon":
Count_Brouillon
'put a flag to reduce the loop
marque_ligneBrouillon = 1

'for each line in "Data":
For i = 4071 To ligne_Data
'find which file I should open (mon_objet):

Solution

A few notes on code-quality and what you can do to make this easier to read and maintain. I'm sure someone else will address the low-hanging performance fruit that I'll leave.
Methods are a thing

I find that your code has many sections extemely similar to the following:

'create a sheet named "Brouillon":
Sheets.Add After:=Sheets(Sheets.Count)
Sheets(Sheets.Count).Name = "Brouillon"
Range("A1").Select


They always follow the same "pattern"

'explanation what happens
DoTheThing
ButMakeItHappenOnTheLowestLevelOfAbstraction


These sections you have there are actually methods

You should seriously consider extracting them into subs:

Private Sub AppendSheet(Optional ByVal sheetName As String = "Brouillon")
    Sheets.Add After := Sheets(Sheets.Count)
    Sheets(Sheets.Count).Name = sheetName
    Range("A1").Select
End Sub


You can rinse and repeat the extraction of these methods to make your Button_Importer_Click() sub much easier to grasp as a whole. After doing all extractions into methods, the method could look like the following:

Private Sub Button_Importer_Click()
    AppendSheet("Brouillon")
    ImportControlSheetTo("Brouillon")
    Dim rows As Long
    rows = CountRows()
    Dim columns As Long
    columns = CountColumns(rows)

    ImportData(rows, columns)
    RemoveDuplicateRows("Brouillon")
    TransposeResults("Brouillon")
    RemoveDuplicateColumns("Brouillon")
    Dim total As Long
    total = CountSpecialCells("Brouillon")
End Sub


Now we can see what that Sub does on a single screen without scrolling. This is extremely important because it makes reasoning about the code significantly easier.

But now that we did that, there's another Problem that becomes apparent. You're using "Brouillon" in a lot of places. What if you mistyped it somewhere? What if you were asked to change the name?
Extract semantically useful constants

As I see it "Brouillon" (jeebus that's the 15th time or so I almost mistyped it) is a magic string. It carries no semantic meaning, because if you fail to type it correctly, the code stops working.

You should consider extracting it into a constant instead:

Private Const SHEET_NAME As String = "Brouillon"


Same goes for "Data" as well as 18, 15, 2 and all the other magic numbers you're using in your actual import code. I have no clue what those numbers stand for, so I'll leave it to you to find a name.
Further nitpicks & standard advice

-
Use Option Explicit to avoid spending time on hunting typos.

-
Avoid Working on the Workbook directly. It's slow as heck. Instead work with Arrays.

-
Use Worksheets instead of Sheets. It's guaranteed to only contain Worksheets. Sheets may contain more than just that.

-
Avoid implicit usage of Range and Worksheets. When the user interacts with Excel while your macro runs, they change, which is bad.

-
Don't work with Select and Active*. In general those are slower than explicitly accessing Cells, Ranges and so on.

-
Check whether you can invert if-conditions to reduce nesting. It's hard to read code that starts 20 columns in, because it makes horizontal scrolling a necessity. Scrolling in general is a bad thing, because it requires you to expend additional mental resources.

Code Snippets

'create a sheet named "Brouillon":
Sheets.Add After:=Sheets(Sheets.Count)
Sheets(Sheets.Count).Name = "Brouillon"
Range("A1").Select
'explanation what happens
DoTheThing
ButMakeItHappenOnTheLowestLevelOfAbstraction
Private Sub AppendSheet(Optional ByVal sheetName As String = "Brouillon")
    Sheets.Add After := Sheets(Sheets.Count)
    Sheets(Sheets.Count).Name = sheetName
    Range("A1").Select
End Sub
Private Sub Button_Importer_Click()
    AppendSheet("Brouillon")
    ImportControlSheetTo("Brouillon")
    Dim rows As Long
    rows = CountRows()
    Dim columns As Long
    columns = CountColumns(rows)

    ImportData(rows, columns)
    RemoveDuplicateRows("Brouillon")
    TransposeResults("Brouillon")
    RemoveDuplicateColumns("Brouillon")
    Dim total As Long
    total = CountSpecialCells("Brouillon")
End Sub
Private Const SHEET_NAME As String = "Brouillon"

Context

StackExchange Code Review Q#133820, answer score: 3

Revisions (0)

No revisions yet.