23

I'd recommend the timeit module when measuring the execution time of functions. AFAIK, timeit disables the garbage collector for the duration of the test, which might give you better results overall. From here: timeit is more accurate, for three reasons: it repeats the tests many times to eliminate the influence of other tasks on your machine, ...


17

public void Refactor() { if (_targetDeclaration == null) { _messageBox.Show(RubberduckUI.PromoteVariable_InvalidSelection); return; } RemoveVariable(); AddField(); } Not sure if anyone other than Rubberduck's architect could point this one out, but it doesn't quite feel right. The parameterless overload of void Refactor()...


13

The only coupling I can see with MSAccess-specific vba is in your exampleCall (why is it Private anyway?): Dim prj As vbProject Set prj = VBE.ActiveVBProject Your code works perfectly fine with Excel vba if you take in a VBProject parameter: Public Sub exampleCall(project As VBProject) If this code lives in a class module called Ext, I can then do this ...


12

Look what I found Dim procedureName As String, lastFound As String Dim procedureBody As String Personally I don't like declaring variables like this, and almost every language allows you to do this in some way or another. I think this is one of those holy war issues though, some programmers like doing this and some programmers say this is bad practice. ...


11

You're a Python native, aren't you? I can tell from the """ multiline strings and the four-space indents and really unidiomatic whitespacing and if/then/else/end instead of ternary and if not instead of unless and use of blocks instead of statements and wow this is really unidiomatic ;-; Each tip assumes that you've already applied the last. Also, I'm ...


11

1. Design The decorator mechanism seems hard to use, unreliable, and counter-productive. It's hard to use because you have to remember to add it to every method that might raise an exception. It's unreliable because it would be easy to forget one or two. And it's counter-productive because in Python, exceptions are not only used for runtime errors, but for ...


11

public IntroduceField(RubberduckParserState parseResult, IActiveCodePaneEditor editor, IMessageBox messageBox) { _declarations = parseResult.AllDeclarations.ToList(); _editor = editor; _messageBox = messageBox; } why don't you pass an IEnumerable<Declaration> instead of a RubberduckParserState object here ? Right now the IntroduceField(...


10

Your GetParentMethodDeclarations() method has a lot of repeated code when it loops over the positionSelection collections. Considering all of them are the same, you can extract them to a helper method and do it once there. At the same time I would rename result as well, not in the least because that would've made it easier for a poor reviewer like me to ...


10

private const string Pattern = @"(?<!\{)\{(?:\{\{)*(\d+(?::.*?)?)\}(?:\}\})*(?!\})"; That pattern is the only reason you need a helper method like this: /// <returns>Returns the placeholder index.</returns> internal static string Normalize(string input) { var temp = input.Trim('{', '}'); var colonIndex = temp.IndexOf(':'); if (...


10

You don't check your entry parameters for null. In FindSelection, this query could be reduced : var target = _declarations .Where(item => !item.IsBuiltIn) .FirstOrDefault(item => item.IsSelected(selection) && item.DeclarationType == DeclarationType.Variable || item.References.Any(r => r.IsSelected(...


9

I've not really dug into everything here, perhaps someone else will give you a fresh perspective on the Unit Testing code. I want to address an algorithm issue in ClassModule.GetMethods. I am partly responsible for this inefficiency because, in previous answer, I pointed you to some code I had written a while back. The algorithm you're currently using is \...


8

Reduce variable span and live time Let me quote an interesting paragraph from Code Complete: The code between references to a variable is a “window of vulnerability.” In the window, new code might be added, inadvertently altering the variable, or someone reading the code might forget the value the variable is supposed to contain. It’s always a good ...


8

This doesn't address your actually question (simplifying the logic/implementation), but I feel like it's important. For the sake of readability, I would recommend replacing some of the nested lambdas with a function reference to a named methods. The assignment to globals in the first section of code has 5 levels of indentation. That is a bit much for an ...


8

While reading my code for the bazillionth time, I noticed a logical error which would make it possible for very specific exotic scenarios to pop this up expectedly. At the very start of the analyzer there is a check to see whether or not we're dealing with a string.Format() call: // Verify we're dealing with a string.Format() call var ...


8

I don't think that what you're doing is good style. I think you're adding a lot of extra complexity and places for things to go wrong, or become hard to understand, or whatever. I don't think you lose anything by just making very small methods that call super and your special method. I talk about those options at the bottom of the post. With that out of the ...


7

Finally: Can you suggest any improvements to my code? Fair warning ahead: I've only tried the following with Rebol 3. That said: going into a different direction than the improvements suggested by @HostileFork, here's an alternative approach: arity: func [ "Returns the arity of a function, disregarding refinements." f [any-function!] ] [ -1 + ...


7

Naming self.retry_times You retry retry_times - 1 times, in all your loops. Consider renaming this or removing the - 1's to make it more understandable. forget This remembers, always. It just doesn't lookup. Consider using a temporary variable or renaming this. RetryTypes This is a constant, not a class. This should be RETRY_TYPES. PEP8 Lines are to be ...


7

First of all, when I start writing decorators of more than moderate complexity, and especially if they take parameters, I usually transition to writing them as classes - I find that easier to reason about and understand. Second of all, in 100% of decorators I've ever written, I've wanted the decorator to look like the wrapped function. To do this, just use ...


7

Here are some issues I noticed in your implementation. Currently you're simply iterating over __slots__ everywhere, but this doesn't handle the case when __slots__ is a string parameter like 'bar'. i.e this shouldn't result in three slots 'b', 'a' and 'r'. Your abstract base class MutableNamedTuple is directly instantiable. The __init__ method only works ...


6

IsTestMethodName is lousy. The function should be IsTestMethod, and could use a few constants: Private Const TestMethodNamePrefix As String = "Test" Private Const TestMethodAttribute As String = "TestMethod" Given a few more properties and a helper function in the Reflection.Method class: Friend Property Let Body(ByVal value As String) this.Body = ...


6

I don't know what's up with the spacing in the docstring, but fix it. Your memoize_all doesn't work with everything: memoize_all(int)(1) #>>> Traceback (most recent call last): #>>> ... #>>> AttributeError: type object 'int' has no attribute 'func_code' I suppose this doesn't bother you much, but it irks me ;). Consider for ...


6

Wow. The idea is nice, but I'll say it clearly: You failed at creating a beautiful, testable and maintainable architecture. Miserably :( But let's start with first things first: import javax.swing.SwingUtilities; This seems to be the first error you've made: Chose your technologies. Swing has officially reached end-of-life status: 6. Is JavaFX ...


6

Your IsSignature relies on IsDeclaration, which doesn't take variable names of functions into account. If I make a function with an argument named maxDimensions, your code fails to identify the function as a function. For example: Public Sub Initialize(maxDimensions as Integer) for some kind of data structure object (multi-dimensional array?). You call ...


6

I'm not a fan of this design. @TheBlackCat is right. Don't do this. If you are to do this, rethink your design. A more sensible one might look like import functools from contextlib import suppress def try_except_response(func): @functools.wraps(func) def wrapper(*args, fail_silently, **kwargs): if fail_silently: with suppress(...


6

It's neat, though I feel like there's a bit too much array manipulation going on. Your friend for this, I'd argue, is the each_cons method, short for "each consecutive". Then you don't need the zip, though you do need to pad the beginning of the array. Here's my take: def brainturing_printer_generator(text) ords = [0] + text.chars.map(&:ord) ords....


6

I don't believe a Semaphore is needed in the first place. Remove it, register an event handler and continue your "act" and "assert" phase inside it. Something like this: parser.State.StateChanged += (o, e) => { var qualifiedSelection = new QualifiedSelection(new QualifiedModuleName(component), selection); var refactoring = new IntroduceParameter(...


6

Some quick notes: I think you can just cast context.Node to SwitchStatementSyntax. The analyzer driver should ensure you should only get called for things you signed up for. If you do want to defensively check, though, I think you should check the node's SyntaxKind. It's more specific and much faster/cheaper. I would avoid LINQ in analyzers. You could ...


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