Hot answers tagged

14

You include Option Explicit :+1 for that alone. Range without any explicit worksheet implicitly is working off of whatever worksheet happens-to-be-active-at-the-time-that-code-is-executed. This will eventually blow up on you. Always qualify your Ranges with a worksheet IE Sheet1.Range("...") so there is no doubt as to what sheet you're referring to. You ...


9

The function is implicitly public, takes an implicitly ByRef parameter that has no reason to not be passed ByVal, and returns an implicit Variant that should be explicit. It's also side-effecting, which makes it unusable as an actual UDF. ThisWorkbook.Worksheets("Scripting Worksheet") suggests the procedure is using a purposely-made bogus (hidden?) sheet ...


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) ...


8

You have no idea how excited I am about this. Years after learning about the Xor operator and I finally get to use it!! VBA - Logical Operators: Xor Operator Called Logical Exclusion. It is the combination of NOT and OR Operator. If one, and only one, of the expressions evaluates to be True, the result is True. The Xor seems like a great fit because ...


8

All your arguments can/should be passed ByVal. As they are presently written they are implicitly ByRef, the default when neither is specified. You want them written ByVal since you're accessing them and not changing them. Use of "" can be rewritten as vbNullString. "" leaves doubt as possibly the string had contents previously but were possibly removed ...


8

Let me start with a few quick things to enhance readability and then give some pointers how one could make this more maintainable, if one wanted to. Magic Numbers One thing that makes your code hard to follow is that you use magic numbers instead of named constants at multiple places. You already use vbYesNo for the MsgBox options most of the time. I ...


7

There is no reason to convert arr to a collection. Join() can quickly build a tag list from a 1 Dimensional array. The trick is to have Join() insert a closing tag + open tag between each element. "<li>" & Join(arr, "</li><li>") & "</li>" Join Demo Dim arr As Variant Dim n As Long ReDim arr(6) For n = 1 To 7 arr(n - ...


7

If I understand correctly, you are testing to see if there are any duplicate entries in Text30 and if you find a duplicate setting flag 5 to 1. In this case you are duplicating at least half of the tests that you are conducting because Task(1).Text(30) = Task(2).Text30 is the same comparison as Tasks(2).Text30 = Task(1).Text30. So you can cut down on the ...


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 ...


6

Disclaimer: Rubberduck is a free, open-source VBIDE add-in project I manage and cheerlead, that's proudly & actively maintained by members of this community. I have zero personal interests in your clicks, but I do own the project's website, contributed and/or reviewed the content, and I've written the articles on the project's blog - covers all links you'...


6

Option Explicit is missing from your code. From the menu at the top under Tools>Options>Editor tab>Code Settings group>Require Variable Declaration <-- toggle that check box on. It will thereafter add the Option Explicit statement to all modules. You'll have to add this manually to any currently existing modules. This mandates you have Dim foo as Range to ...


6

FetchMonthNumber Get is a standard across most programming languages, where as, Fetch is a standard when playing with a dog. But neither GetMonthNumber or FetchMonthNumber provide any context to the return value of the function. FetchMonthNumber = "" The code above is unnecessary. FetchMonthNumber is typed as a string and has a default value of "". ...


6

Turn on Option Explicit. From the VBIDE menu at the top Tools>Options to display the Options dialog>Editor tab>Code Settings group>Require Variable Declaration. Tick that check box. From that point on Option Explicit will always be added to the top of every new (Standard, Form, Class) module you create. Future-you will thank you. This mandates that all your ...


5

Amazing code as always, Mathieu and Comintern; many thanks. I'm here only two add two cents: Probably you tested it in a 32 bits setup; to compile in a 64 bits you need to (only differences): #If Win64 Then Private Declare PtrSafe Sub CopyMemory Lib "kernel32.dll" Alias "RtlMoveMemory" (hpvDest As Any, hpvSource As Any, ByVal cbCopy As LongPtr) ...


5

MoveFilesToFolder() MoveFilesToFolder() is doing too much. Testing filePath and moveToPath in a separate sub would greatly reduce MoveFilesToFolder() size making it easier to read, test and modify. Private Const DirctoryBron As String = "Bron directory" Private Const DirctoryDoel As String = "Doel directory" Private Const WorksheetImages As String = "...


5

First I'd like to say that this is impressive work, overall pretty squeaky clean... despite the adjustments needed to make it build on x64 :) One enhancement I can see in terms of readability, would be to use PascalCase rather than camelCase for member names: inconsistent casing is distracting, because parameters and locals are usually camelCase, so a camel-...


5

CodeReview Why use the Scripting.FileSystemObject? MyFolder is just returning the folder name MyFolder = Worksheets("Config").Range("D4").Value & ActiveSheet.Range("C21").Value I would also write a function to return the folder path and a second function to create the icacls commands. Fun Part: My Own Solution The class below will create a self-...


5

I think this is what, in rugby, they would call a 'hospital pass'. As someone who has fixed much code like this, I am going to hit some highlights only. If you manage to fix these highlights, I would love to see the revised code in another question for the second round. Because the job you have undertaken will take many passes to get right (but it will be ...


5

Sub Button1_Click() I know it's tempting to just double-click the ActiveX control and code everything in the auto-generated event handler procedure, but that's a very bad habit you will need to lose early on. The handler was generated with an explicit Private access modifier, too - it being absent means it was removed, leaving the procedure implicitly ...


5

Declarations My first comment is Option explicit. Every. Single. Time. Your first line of code is : Set objShell = CreateObject("WScript.Shell") Why? objshell is not declared or used. And while on the matter of declarations: Dim WDR, WDCheck, ShipmentID As Object declares WDR and WDCheck as Variant, not Object. Youi are writing a utility tool - ...


5

On first sight, your code looks reasonably short. Nevertheless, no matter how short the code is, there's always something to improve. The function name and signature are perfect. I though about passing the string as ByVal, but that might be less efficient since the string might have to be copied then. Initializing the return value of the function to False ...


5

When writing any kind of event method, I always try to minimize the code that executes because you don't want the user to be aware of processing that is happening between the keystrokes. In your case, your code is firing each time ANY cell on ANY sheet is changed. So in the spirit of keeping things streamlined, don't create, initialize, or perform any logic ...


5

Unfortunately the exercise is a bit abstract to me, because I cannot visualize the data you are manipulating. A data sample would have been welcome to better comprehend your purpose. A few comments anyway I don't see the code for FindLastRow You have multiple references to C17, so it should be defined as a constant variable. It could be named START_CELL or ...


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

75K Non-Contiguous Areas, No Problem My FastUnion class was able to crack the 75K non-contiguous areas goal by expanding on Ahmed AU answer using Union() with multiple parameters. Although, this class excels at smaller numbers of areas, my UnionCollection class far out performs it by working with smaller groups of cells at a time. Results Immediate ...


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 ...


4

Code is generally very clean, although I do have a number of reservations with some of the naming: c prefix for class modules, M for standard ones, is pure noise; Cell as a suffix for something that isn't a cell, is confusing. That kind of thing. I would have named cStyles as Styles, or perhaps StyleProcessor since we don't want to hide Excel.Styles; ...


4

An observation on your class constructors. At the moment you are using public properties to allow you to set the value of properties of a class after it has been created. You can take this to the next step which allows you to delete the setters for the public class members by passing the create parameters to the Self function. In this way you can create ...


4

Although you think that the second one is closer to your desired syntax, it is actually somewhat confusing. If LetSet was a function with only one parameter, this would cause a Let coercion on the RHS, if an object is supplied. E.g. if the RHS is a range, the usual behaviour of a Let assignment is that the values in the range are assigned instead of the ...


4

I'd name the procedure Assign: Dim foo As Long Assign foo, 42 But then, this is involving a stack frame for an otherwise very straightforward operation - the method intending to be generic, it cannot bring any additional logic to the table, and ultimately comes off as redundant. A Property Let procedure is more problematic though, because such procedures ...


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