adobe-webplatform/Snap.svg — pull request #216 by tshinnic (2014-03-17 02:30:34+00:00)
Title: cleanup duplicate code on group Element creation  (not a bug)

I found that some code was duplicated when creating group Elements. More specifically, when creating a <g> Snap Element, code first calls make("g",...).
```javascript
proto.group = proto.g = function (first) {
var el = make("g", this.node);
el.add = add2group;
for (var method in proto) if (proto[has](method)) {
el[method] = proto[method];
}
```
make() calls wrap() ...
```javascript
function make(name, parent) {
var res = $(name);
parent.appendChild(res);
var el = wrap(res);
```
which calls new Element()
```javascript
function wrap(dom) {
: : : :
return new Element(dom);
```
which itself has special code for <g> Element creation.
```javascript
function Element(el) {
: : : :
this.type = el.tagName;
: : : :
if (this.type == "g") {
this.add = add2group;
for (var method in Paper.prototype) if (Paper.prototype[has](method)) {
this[method] = Paper.prototype[method];
}
}
```
Once Element() has returned, this duplicate code in ```elproto.group()``` is not needed, as all these have already been set:
```javascript
el.add = add2group;
for (var method in proto) if (proto[has](method)) {
el[method] = proto[method];
}
```
nor is the single duplicate line of code in make() needed:
```javascript
function make(name, parent) {
: : : :
var el = wrap(res);
el.type = name;
```
This was tested first by inserting 'throw's into the code, specifically testing that the asserted behavior was true.
```
C:\Toms\Study\Javascript\Snap.svg\github\Snap.svg\src>git diff svg.js
diff --git a/src/svg.js b/src/svg.js
index d347a7b..98dd52f 100644
--- a/src/svg.js
+++ b/src/svg.js
@@ -2507,6 +2507,8 @@ function make(name, parent) {
var res = $(name);
parent.appendChild(res);
var el = wrap(res);
+ if (!el.type) throw new Error("make() el.type not already set");
+ if (el.type !== name) throw new Error("make() el.type not set correctly");
el.type = name;
return el;
}
@@ -2909,6 +2911,10 @@ function gradientRadial(defs, cx, cy, r, fx, fy) {
\*/
proto.group = proto.g = function (first) {
var el = make("g", this.node);
+ if (!el.add) throw new Error("elproto.group() el.a
+ if (el.add !== add2group) throw new Error("elproto.group() el.a
+ if (!el.el) throw new Error("elproto.group() el.e
+ if (el.el !== Paper.prototype.el) throw new Error("elproto.group() el.e
el.add = add2group;
for (var method in proto) if (proto[has](method)) {
el[method] = proto[method];
```
This code did not throw during the tests. Note that a one point the checks were coded wrong and did throw.

The duplicate code was then removed and tests re-run.
```
C:\Toms\Study\Javascript\Snap.svg\github\Snap.svg\src>diff -u svg.js.original svg.js.deduped
--- svg.js.original 2014-03-16 20:22:37.409811000 -0500
+++ svg.js.deduped 2014-03-16 20:54:50.497553500 -0500
@@ -2507,7 +2507,6 @@
var res = $(name);
parent.appendChild(res);
var el = wrap(res);
- el.type = name;
return el;
}
function Paper(w, h) {
@@ -2909,10 +2908,6 @@
\*/
proto.group = proto.g = function (first) {
var el = make("g", this.node);
- el.add = add2group;
- for (var method in proto) if (proto[has](method)) {
- el[method] = proto[method];
- }
if (arguments.length == 1 && first && !first.type) {
el.attr(first);
} else if (arguments.length) {
```
While somewhat difficult to see because of the number of other errors due to bugs or browser boo-boos, neither the checking throws nor the removal of the duplicate code caused any (more) errors in the test/test.html runs.
```
FireFox 27.0.1
no changes
passes: 203 failures: 10 duration: 2.60s
path2curve fix
passes: 208 failures: 5 duration: 3.12s
dedupe_checks
passes: 208 failures: 5 duration: 2.76s
deduped
passes: 208 failures: 5 duration: 2.60s

Opera 20.0
gets several errors due to local file:// ajax failures
no changes
passes: 199 failures: 14 duration: 2.03s
path2curve fix
passes: 202 failures: 11 duration: 2.20s
dedupe_checks
passes: 202 failures: 11 duration: 2.26s
deduped
passes: 202 failures: 11 duration: 2.18s

Chrome 33.0
gets several errors due to local file:// ajax failures
no changes
passes: 199 failures: 14 duration: 1.99s
path2curve fix
passes: 202 failures: 11 duration: 2.19s
dedupe_checks
passes: 202 failures: 11 duration: 2.25s
deduped
passes: 202 failures: 11 duration: 2.08s

IE 10
gets several errors due to local file:// ajax failures
no changes
passes: 189 failures: 24 duration: 1.87s
path2curve fix
passes: 195 failures: 18 duration: 2.45s
(lovely IE)
Error: expected 'M 10 10 L 50 60' to equal 'M10,10,50,60'
Error: expected '10,15 20,25 30,35' to equal '10,15,20,25,30,35'
TypeError: Unable to get property 'nodeName' of undefined or null reference
dedupe_checks
passes: 195 failures: 18 duration: 1.92s
deduped
passes: 195 failures: 18 duration: 1.81s
```

This is obviously not a high priority change, but would help de-clutter the code.