Re: svn commit: r1183065 - in /jakarta/jmeter/trunk: src/core/org/apache/jmeter/engine/ src/core/org/apache/jmeter/gui/action/ src/core/org/apache/jmeter/gui/util/ src/core/org/apache/jmeter/resources/ xdocs/

On 13 October 2011 21:44, Philippe Mouawad <[email protected]> wro=
te:
> 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 frequentl=
y
> 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 <
> [email protected]> wrote:
>
>>
>>
>> On Thu, Oct 13, 2011 at 10:26 PM, sebb <[email protected]> wrote:
>>
>>> On 13 October 2011 21:12, =A0<[email protected]> wrote:
>>> > Author: pmouawad
>>> > Date: Thu Oct 13 20:12:23 2011
>>> > New Revision: 1183065
>>> >
>>> > URL: http://svn.apache.org/viewvc?rev=3D1183065&view=3Drev
>>> > Log:
>>> > Bug 52019 - Add menu option to Start a test ignoring Pause Timers
>>> >
>>> > Added:
>>> >
>>> =A0jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeClonerNoT=
imer.java
>>> =A0 (with props)
>>> > Modified:
>>> >
>>> =A0jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeCloner.ja=
va
>>> >
>>> =A0jakarta/jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionNam=
es.java
>>> > =A0 =A0jakarta/jmeter/trunk/src/core/org/apache/jmeter/gui/action/Sta=
rt.java
>>> >
>>> =A0jakarta/jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuB=
ar.java
>>> >
>>> =A0jakarta/jmeter/trunk/src/core/org/apache/jmeter/resources/messages.p=
roperties
>>> >
>>> =A0jakarta/jmeter/trunk/src/core/org/apache/jmeter/resources/messages_e=
s.properties
>>> >
>>> =A0jakarta/jmeter/trunk/src/core/org/apache/jmeter/resources/messages_f=
r.properties
>>> >
>>> =A0jakarta/jmeter/trunk/src/core/org/apache/jmeter/resources/messages_p=
t_BR.properties
>>> > =A0 =A0jakarta/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/j=
meter/engine/TreeCloner.java?rev=3D1183065&r1=3D1183064&r2=3D1183065&view=
=3Ddiff
>>> >
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
>>> > ---
>>> 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
>>> > =A0 =A0 }
>>> >
>>> > =A0 =A0 public void addNode(Object node, HashTree subTree) {
>>> > -
>>> > =A0 =A0 =A0 =A0 if ( (node instanceof TestElement) // Check can cast =
for clone
>>> > =A0 =A0 =A0 =A0 =A0 =A0// Don't clone NoThreadClone unless honourNoTh=
readClone =3D=3D
>>> false
>>> > =A0 =A0 =A0 =A0 =A0 && (!honourNoThreadClone || !(node instanceof NoT=
hreadClone))
>>> > @@ -66,6 +65,14 @@ public class TreeCloner implements HashT
>>> > =A0 =A0 =A0 =A0 } else {
>>> > =A0 =A0 =A0 =A0 =A0 =A0 newTree.add(objects, node);
>>> > =A0 =A0 =A0 =A0 }
>>> > + =A0 =A0 =A0 =A0addLast(node);
>>> > + =A0 =A0}
>>> > +
>>> > + =A0 =A0/**
>>> > + =A0 =A0 * add node to objects LinkedList
>>> > + =A0 =A0 * @param node Object
>>> > + =A0 =A0 */
>>> > + =A0 =A0protected final void addLast(Object node) {
>>> > =A0 =A0 =A0 =A0 objects.addLast(node);
>>> > =A0 =A0 }
>>>
>>> OK, so subclasses can override this to skip adding a node.
>>>
>>> >
>>> > Added:
>>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeClonerNoTime=
r.java
>>> > URL:
>>> http://svn.apache.org/viewvc/jakarta/jmeter/trunk/src/core/org/apache/j=
meter/engine/TreeClonerNoTimer.java?rev=3D1183065&view=3Dauto
>>> >
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
>>> > ---
>>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeClonerNoTime=
r.java
>>> (added)
>>> > +++
>>> jakarta/jmeter/trunk/src/core/org/apache/jmeter/engine/TreeClonerNoTime=
r.java
>>> Thu Oct 13 20:12:23 2011
>>> > @@ -0,0 +1,59 @@
>>> > +/*
>>> > + * Licensed to the Apache Software Foundation (ASF) under one or mor=
e
>>> > + * contributor license agreements. =A0See the NOTICE file distribute=
d
>>> with
>>> > + * this work for additional information regarding copyright ownershi=
p.
>>> > + * The ASF licenses this file to You under the Apache License, Versi=
on
>>> 2.0
>>> > + * (the "License"); you may not use this file except in compliance w=
ith
>>> > + * the License. =A0You may obtain a copy of the License at
>>> > + *
>>> > + * =A0 http://www.apache.org/licenses/LICENSE-2.0
>>> > + *
>>> > + * Unless required by applicable law or agreed to in writing, softwa=
re
>>> > + * 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 a=
nd
>>> > + * 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, =A0skipping test elements that implement {@=
link
>>> Timer} by default.
>>> > + */
>>> > +public class TreeClonerNoTimer extends TreeCloner{
>>> > + =A0 =A0private Logger logger =3D LoggingManager.getLoggerForClass()=
;
>>> > +
>>> > + =A0 =A0/**
>>> > + =A0 =A0 * {@inheritDoc}
>>> > + =A0 =A0 */
>>> > + =A0 =A0public TreeClonerNoTimer() {
>>> > + =A0 =A0 =A0 =A0super();
>>> > + =A0 =A0}
>>> > +
>>> > + =A0 =A0/**
>>> > + =A0 =A0 * {@inheritDoc}
>>> > + =A0 =A0 */
>>> > + =A0 =A0public TreeClonerNoTimer(boolean honourNoThreadClone) {
>>> > + =A0 =A0 =A0 =A0super(honourNoThreadClone);
>>> > + =A0 =A0}
>>> > +
>>> > + =A0 =A0/**
>>> > + =A0 =A0 * {@inheritDoc}
>>> > + =A0 =A0 */
>>>
>>> @Override?
>>>
>>> > + =A0 =A0public void addNode(Object node, HashTree subTree) {
>>> > + =A0 =A0 =A0 =A0if(!(node instanceof Timer)) {
>>>
>>> It's confusing to use negative conditions
>>>
>>
>> It's for performance, in non GUI mode negative conditiion will be the mo=
st
>> frequent
>>
>>>
>>> > + =A0 =A0 =A0 =A0 =A0 =A0super.addNode(node, subTree);
>>> > + =A0 =A0 =A0 =A0} else {
>>> > + =A0 =A0 =A0 =A0 =A0 =A0if(logger.isDebugEnabled()) {
>>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0logger.debug("Ignoring timer node:"+=
 node);
>>> > + =A0 =A0 =A0 =A0 =A0 =A0}
>>> > + =A0 =A0 =A0 =A0 =A0 =A0addLast(node);
>>>
>>> This looks wrong, surely you don't want to add the node?
>>>
>>> No otherwise you will get NoSuchElementException
>>
>>> > + =A0 =A0 =A0 =A0}
>>> > + =A0 =A0}
>>> > +}
>>>
>>> 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: [email protected]
>>> For additional commands, e-mail: [email protected]
>>>
>>>
>>
>>
>> --
>> Cordialement.
>> Philippe Mouawad.
>>
>>
>>
>>
>
>
> --
> Cordialement.
> Philippe Mouawad.
>