On 13 October 2011 21:44, Philippe Mouawad <philippe.moua...@gmail.com> wrote: > I changed implementation a little.
That's much clearer. However the method private final void addLast(Object node) is no longer needed and could be inlined. Sorry, I was confused by the addLast(); thought it was adding to the new tree, rather than keeping track of the current one. > Added @Override (as I didn't see it in other classes, didn't add it) OK > About negative condition, Start should generally be called more frequently > that the new option so put it first. There is only one condition check. Surely the following have the same performance. Positive: if (node instanceof Timer) { return node; } else { return super.addNodeToTree(node); } Negative: if (!(node instanceof Timer)) { return super.addNodeToTree(node); } else { return node; } I agree where there are multiple conditions, best to check the cheapest first, but here there is only one check. It's important for code to be readable. > Regards > Philippe > > On Thu, Oct 13, 2011 at 10:29 PM, Philippe Mouawad < > philippe.moua...@gmail.com> wrote: > >> >> >> On Thu, Oct 13, 2011 at 10:26 PM, sebb <seb...@gmail.com> wrote: >> >>> On 13 October 2011 21:12, <pmoua...@apache.org> wrote: >>> > Author: pmouawad >>> > Date: Thu Oct 13 20:12:23 2011 >>> > New Revision: 1183065 >>> > >>> > URL: http://svn.apache.org/viewvc?rev=1183065&view=rev >>> > Log: >>> > Bug 52019 - Add menu option to Start a test ignoring Pause Timers >>> > >>> > Added: >>> > >>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeClonerNoTimer.java >>> (with props) >>> > Modified: >>> > >>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeCloner.java >>> > >>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionNames.java >>> > jakarta/jmeter/trunk/src/core/org/apache/jmeter/gui/action/Start.java >>> > >>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java >>> > >>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/resources/messages.properties >>> > >>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/resources/messages_es.properties >>> > >>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/resources/messages_fr.properties >>> > >>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/resources/messages_pt_BR.properties >>> > jakarta/jmeter/trunk/xdocs/changes.xml >>> > >>> > Modified: >>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeCloner.java >>> > URL: >>> http://svn.apache.org/viewvc/jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeCloner.java?rev=1183065&r1=1183064&r2=1183065&view=diff >>> > >>> ============================================================================== >>> > --- >>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeCloner.java >>> (original) >>> > +++ >>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeCloner.java Thu >>> Oct 13 20:12:23 2011 >>> > @@ -56,7 +56,6 @@ public class TreeCloner implements HashT >>> > } >>> > >>> > public void addNode(Object node, HashTree subTree) { >>> > - >>> > if ( (node instanceof TestElement) // Check can cast for clone >>> > // Don't clone NoThreadClone unless honourNoThreadClone == >>> false >>> > && (!honourNoThreadClone || !(node instanceof NoThreadClone)) >>> > @@ -66,6 +65,14 @@ public class TreeCloner implements HashT >>> > } else { >>> > newTree.add(objects, node); >>> > } >>> > + addLast(node); >>> > + } >>> > + >>> > + /** >>> > + * add node to objects LinkedList >>> > + * @param node Object >>> > + */ >>> > + protected final void addLast(Object node) { >>> > objects.addLast(node); >>> > } >>> >>> OK, so subclasses can override this to skip adding a node. >>> >>> > >>> > Added: >>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeClonerNoTimer.java >>> > URL: >>> http://svn.apache.org/viewvc/jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeClonerNoTimer.java?rev=1183065&view=auto >>> > >>> ============================================================================== >>> > --- >>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeClonerNoTimer.java >>> (added) >>> > +++ >>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeClonerNoTimer.java >>> Thu Oct 13 20:12:23 2011 >>> > @@ -0,0 +1,59 @@ >>> > +/* >>> > + * Licensed to the Apache Software Foundation (ASF) under one or more >>> > + * contributor license agreements. See the NOTICE file distributed >>> with >>> > + * this work for additional information regarding copyright ownership. >>> > + * The ASF licenses this file to You under the Apache License, Version >>> 2.0 >>> > + * (the "License"); you may not use this file except in compliance with >>> > + * the License. You may obtain a copy of the License at >>> > + * >>> > + * http://www.apache.org/licenses/LICENSE-2.0 >>> > + * >>> > + * Unless required by applicable law or agreed to in writing, software >>> > + * distributed under the License is distributed on an "AS IS" BASIS, >>> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >>> implied. >>> > + * See the License for the specific language governing permissions and >>> > + * limitations under the License. >>> > + * >>> > + */ >>> > + >>> > +package org.apache.jmeter.engine; >>> > + >>> > +import org.apache.jmeter.timers.Timer; >>> > +import org.apache.jorphan.collections.HashTree; >>> > +import org.apache.jorphan.logging.LoggingManager; >>> > +import org.apache.log.Logger; >>> > + >>> > +/** >>> > + * Clones the test tree, skipping test elements that implement {@link >>> Timer} by default. >>> > + */ >>> > +public class TreeClonerNoTimer extends TreeCloner{ >>> > + private Logger logger = LoggingManager.getLoggerForClass(); >>> > + >>> > + /** >>> > + * {@inheritDoc} >>> > + */ >>> > + public TreeClonerNoTimer() { >>> > + super(); >>> > + } >>> > + >>> > + /** >>> > + * {@inheritDoc} >>> > + */ >>> > + public TreeClonerNoTimer(boolean honourNoThreadClone) { >>> > + super(honourNoThreadClone); >>> > + } >>> > + >>> > + /** >>> > + * {@inheritDoc} >>> > + */ >>> >>> @Override? >>> >>> > + public void addNode(Object node, HashTree subTree) { >>> > + if(!(node instanceof Timer)) { >>> >>> It's confusing to use negative conditions >>> >> >> It's for performance, in non GUI mode negative conditiion will be the most >> frequent >> >>> >>> > + super.addNode(node, subTree); >>> > + } else { >>> > + if(logger.isDebugEnabled()) { >>> > + logger.debug("Ignoring timer node:"+ node); >>> > + } >>> > + addLast(node); >>> >>> This looks wrong, surely you don't want to add the node? >>> >>> No otherwise you will get NoSuchElementException >> >>> > + } >>> > + } >>> > +} >>> >>> However, the whole approach looks wrong. >>> >>> Why not just override addLast(), and skip super.addLast() if processing a >>> Timer? >>> >>> Don't understand why >> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: dev-unsubscr...@jakarta.apache.org >>> For additional commands, e-mail: dev-h...@jakarta.apache.org >>> >>> >> >> >> -- >> Cordialement. >> Philippe Mouawad. >> >> >> >> > > > -- > Cordialement. > Philippe Mouawad. > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@jakarta.apache.org For additional commands, e-mail: dev-h...@jakarta.apache.org