The Stack Overflow podcast is back! Listen to an interview with our new CEO.

Hot answers tagged

10

Range.CopyFromRecordset only addresses the [massive] performance issue of traversing an entire recordset row by agonizing row and writing it to a worksheet cell by agonizing cell - all while Excel painstakingly repaints itself every time, fires Worksheet.Change events, and evaluates whether or not recalculations should be happening... between every single ...


8

Without Option Explicit, the msoGraphic identifier in that property scope is a Variant/Empty; there's an implicit type conversion happening when you do this: If msoGraphic = 0 Then Sure vbEmpty will equate to 0, or even vbNullString or "", but that's after converting to a comparable type (Integer, or String). There's a better way. If IsEmpty(msoGraphic) ...


7

Amount of boilerplate code As it is, there are lot of boilerplate that we must provide to set up everything. At a minimum, we need the following code: <Procedure> SomeProcedure On Error GoTo ErrTrap 'Actual Code ExitProcedure: On Error Resume Next Exit Function ErrTrap: Select Case Err.number <specific Case handlers as ...


7

Before diving into the code itself, I'm going to go over a couple "structural things". First, I'm not entirely sure what the utility of a tool like this actually is. When I'm writing code in the Visual Basic Editor, I get all of this great help like IntelliSense, syntax highlighting, an Object Browser, etc., etc. (and this is before using custom add-ins ...


7

To enforce a collection-like functionality would I implement a custom made ICollection interface in each container class. While an ICollection interface is nice in theory, in practice it doesn't really play nicely with how VBA implements enumerables. The single main advantage of using an object collection is its ability to leverage [_NewEnum] and be ...


7

There are 5 steps you could take after which you could repost your code here. Use 'Option Explicit' in each module and address the errors that this will show. If you are able, install the RubberDuck addin. Use the 'RubberDuck' addin to do a code inspection and then address all the issues you find. Split your code into simple functions/subs. At the moment ...


6

Option Explicit Always nice to see. Public Function RemoveBlanksFromArray(ByVal TheArray As Variant) As Variant The usual convention for parameters is camelCase - i.e. theArray. Otherwise looks fine, maybe I'd call it inputArray so it's obvious to the rest of your code what it contains (more on naming). One thing to watch out for when passing arrays ...


6

This looks like a decent recursive implementation. So, this is mainly about style and readability. First, I would like to talk about two things which are really hard and could use some improvement here, naming and consistency of semantic levels. Then I have some more miscellaneous comments. Naming Generally, the naming is not too bad, but some of the ...


5

A couple quick house-keeping issues first: Get rid of your old commented out code - it's simply adding noise. Your indentation is inconsistent. I had to run this through an indenter before I could tell what this section was supposed to be doing: End With tabSh.Select s.Offset(1).Select ActiveSheet.Paste Application.CutCopyMode = False ...


5

I could take this as a not-so-subtle reminder that I need to finish my Excel Tetris implementation... :-P I am a little curious why you seem to have abandoned the OOP approach since your last game - this code is completely procedural (the presence of classes doesn't mean that it's object oriented). A discussion of the architecture would basically entail a ...


5

Well that's great progress. I now have some more comments. Brute force rather than precision Your search for highlighted words is using a 'brute force' approach as you are examining every word rather than using the Word search function to search for words with a specific highlight. 2 Multiple variables rather than grouped data Your are using multiple ...


5

As this is a code review request, some of my comments may be considered "best practices" by me and not by others (though most of my habits I've picked up from sites and reviews such as this one). Your code is successful already because it accomplishes the task for which you have designed. Most of the improvements I can suggest are in terms of software design ...


5

In this review, I am only looking at options for addressing the loops. With the number of rows you have described, finding efficiencies in the loops should have a big payoff. Don't collapse lines of code using ":", it is not necessary, breaks indenting and makes it harder to find some lines. Loop 1 'get count of fields (columns) with data Dim colCount ...


5

No time for a full-blown review, but glancing over the code, a few points stick out: Enthusiastic Abstractions In a few places, you extracted a trivial one-liner function that is only used in a single place. IMO that's the wrong abstraction to use - for example: Private Function IsUserform(ByRef Obj As Object) As Boolean If TypeOf Obj Is MSForms....


5

I often use the Index Match function in daily reporting tasks. I was looking for a quicker way to utilize Index Match, as I find the formula cumbersome to enter. However, a quick look at the form, implementing it into any workbook/application will be cumbersome and any user has to perform many steps - most accomplished Excel users will complete and auto-...


4

The line Dim LRow, LRow2, i, n, serviceID As Long isn't doing what you think it is. When you declare multiple variables on the same line, you need to individually assign types to all of them - otherwise the default to Variant. I'm assuming that line was meant to be Dim LRow As Long, LRow2 As Long, i As Long, n As Long, serviceID As Long. You could probably ...


4

Yup, it's wrong1 :) Forms (and literally everything in Windows) already run their own message loop: whenever you feel the need to wrap a form with a no-op "wait until user does something" DoEvents loop, you are essentially forcing your form into a procedural dialog paradigm, keeping the VBA runtime much, much more busy than it needs to be. Embrace the ...


4

If you're going to do any developing in VBA then you need to check out Rubberduck . Open disclosure, I'm a contributor. I've used it for long enough now that the IDE doesn't feel complete without it. Rubberduck will help you write better code and teach you along the way with code inspections and other goodies. Dim white, red, ... , black as Long is only ...


4

It's not super-clear to me what's going on here but this stands out as cell-by-cell data transfer, which is pretty slow: For s = 1 To nrows Range("Check_Location") = check_nodes(s) PR_row = Range("A9").Row - 1 '<< why not just =8? PR_col = Range("A9").Column - 1 '<< and = 0 ? Cells(PR_row + s, PR_col ...


4

Depending on what you want to return you can actually leverage just one API method from Bing to retrieve the data you are after, the Driving Route API. Also, as others have pointed out what's killing performance is the synchronous code. What I've done below is limited the pull to a single request, and made the code asynchronous. I've also changed this ...


4

Don't be afraid to trash me or offend me. Ok, I'll bite ;-) But this isn't about you, it's all about the code. It's always about the code. Reviewers are not here to judge, they're here to help you grow, and improve your programming! I obviously am new to this, and there has to be a better way to do it Oh, yes, absolutely. But first, I need to know what ...


4

I do have a few suggestions: First of all, you don't need to reactivate your main workbook every loop, since you hold it's reference in a variable anyway. Second, instead of creating new workbook and copying the sheet over, just make a copy to new workbook (i.e. wks.Copy creates copy in new workbook, you can then assign it to a variable since, as you said ...


3

Rubberduck advises against overwriting variables passed ByVal - e.g in the Init method - why? It's a) coding style; keeping the original value in the parameter is supposed to be cleaner code because you can spot all the places where the exact passed in value is used and know where it is not used (but instead a modified/derived/sanitized version of it) ...


3

There are a few optimizations that can be made to the code to speed it up, mainly to do with how some mathematical operations are performed in VBA. Not sure if I have it implemented 100% accurately so please review for accuracy. Also, I'm sure there are more optimizations to be had, but this is hopefully a starting point for further conversation. A lot of ...


3

Use any method that make a "directly-interation" with worksheet like copy, paste, range.value = something (...) intensifies the runtime of the code. To solve this you need to create an array to store the SourceSh.Rows(r) values. Before the loop change spreadsheet you parse all values stored in the array to cells using the method that I will show in the ...


3

OnStart and OnEnd should be called from Main. Although, I am a proponent of using single letter iterates in simple For loops, I think that i should have a more descriptive name like pageIndex. Navigate pageIndex, appIE, keyword Sleep - A common subroutine used across many programming languages. I see no reason to wrap it in WaitSomeMilliseconds(). You ...


3

Formatting individual characters in a string is relatively slow. My solution convert the exponents to superscript using Ascii and Unicode characters. This allows me to work with the values in an array and then write it back over the original range. Using characters code took 0.79 seconds to process 10,000 cells compared to 42.05 using formatting ...


3

You've not provided a fully functional set of code with data, so I can only make untested suggestions as an example for you to incorporate into your application. Without seeing the data that you are working with, it's impossible to know if a Dictionary is the best approach. Using a Dictionary is a good idea, but only if all of the data in the table is ...


3

This post is tagged with object-oriented, but what I'm seeing is very ironically procedural code. It doesn't matter that this is "just a quick tool": we're here to write professional, quality code that is easy to read and maintain, performs well, and correctly. Comintern has given great feedback and highlighted a number of bugs and edge cases - which is ...


3

First off, nice work overall. It's apparent from the way you set up your interfaces and implementations that you "get it". Given that, most? of this can probably be classified as "nitpicking". I'm also not going to specifically address your second question, but the answers should be apparent based on the review itself (if not, feel free to ask in the ...


Only top voted, non community-wiki answers of a minimum length are eligible