Looking for feedback on Matching Exercises Maker/ Builder

[lorlarz]

Looking for feedback on Matching Exercises Maker/ Builder:

http://mynichecomputing.com/ReadIt/translateT.html

For one thing, I am concerned about storing the matching kwork (known
word)
as the className of the li which is the answer. I think this may be
problematic.

I also worry about using the ul as the container for the whole thing.

Kind feedback would be appreciated (I am a little tired of harsh
feedback from newsgroups).

Thanks.

 

[Thomas 'PointedEars' Lahn]

Looking for feedback on Matching Exercises Maker/ Builder:

http://mynichecomputing.com/ReadIt/translateT.html

<http://validator.w3.org/check?uri=h...ReadIt/translateT.html&ss=1&group=0&verbose=1>
<http://jigsaw.w3.org/css-validator/...//mynichecomputing.com/ReadIt/translateT.html>

For one thing, I am concerned about storing the matching kwork (known
word) as the className of the li which is the answer. I think this may
be problematic.

It is. CSS class names are restricted to a limited set of characters:

<http://www.w3.org/TR/REC-CSS2/grammar.html#q1>

I suggest you use a mapping object instead:

var kwords = {
items: ["Shark", "Lion", "Dolphin", "Squirrel"],
map: {
Shark: "Tiburón",
Lion: "León",
Dolphin: "Delfín",
Squirrel: "Ardilla"
}
};

You can iterate over the `items' array to build the list (iterating over
`map' would use arbitrary order). Suppose `firstClicked' and `lastClicked'
store the words in the clicked list elements, then you can test whether
there is a match:

if (lastClicked === kwords.map[firstClicked])
{
// ...
}
else
{
// ...
}

I recommend not to bother your users with alert() messages (at least not
always); it indicates an error/problem where there might not be none and
drags the user's attention off from what was clicked, which is disturbing.
Use accessible Web-safe colors and at least one other style to indicate
correctness (or the opposite).

I also worry about using the ul as the container for the whole thing.

It might be better to use two UL elements, indeed.

As for your script:

- Why do you mix `new Array()' and `[]'?
- There are too many global variables.
- Math.random() needs a bugfix if it it returns 1.0.
- Your indentation sucks^W can be improved.
- You are using too many unnecessary parentheses around expressions
for my taste.
- You are not feature-testing anything, which is error-prone.
- You use identifiers for constructors to refer to simple objects.
- var i, item;
for (i = listEvents.length - 1; i >= 0; i = i - 1)
{
item = listEvents;

can be rewritten more efficient as

for (var i = listEvents.length; i--
{
var item = listEvents;

- Your addEvent() is flawed, W3C's addEventListener() and MSHTML's
attachEvent() are not equivalent.
- There are too many lookups for the same reference, which makes this
inefficient and hard to maintain.
- Your getXHR() is error-prone, exception-handling is not supported in
all UAs that support XHR. The method is also needlessly inefficient:
you do not need advanced features of MSXML 3.0 here, so testing for
Microsoft.XMLHTTP suffices.
- Your handleReadyState() makes no sense to me: The callback assigned
to the `onreadystatechange' property is automatically called when the
readyState changes; you don't have to wait for that.
- ISTM you do not need XHR here, see above.
- You use `innerHTML', which is proprietary and error-prone, needlessly.

Search the newsgroup's archives for details. And try applying
<http://www.jslint.com/> to your code. (Some messages are
but a matter of taste, some are based on good recommendations.)

That should be enough homework for today.

Kind feedback would be appreciated (I am a little tired of harsh feedback
from newsgroups).

Well, you get what you paid for.

<http://jibbering.com/faq/>


PointedEars

 

[lorlarz]

I like a lot of your feedback.

<http://validator.w3.org/check?uri=http://mynichecomputing.com%2....>
<http://jigsaw.w3.org/css-validator/validator?profile=css2&warning=2&u...>


It is.  CSS class names are restricted to a limited set of characters:

<http://www.w3.org/TR/REC-CSS2/grammar.html#q1>

I am considering just a little extra code (including one more array),
so this
is not a problem.

I suggest you use a mapping object instead:

  var kwords = {
    items: ["Shark", "Lion", "Dolphin", "Squirrel"],
    map: {
      Shark:    "Tiburón",
      Lion:     "León",
      Dolphin:  "Delfín",
      Squirrel: "Ardilla"
    }
  };

While that seems like a good suggestion, note that the application
takes in
a JSON array to _swap out_ sets. Can you write this to be a valid
part
of such a 5 piece JSON array of objects?

You can iterate over the `items' array to build the list (iterating over
`map' would use arbitrary order).  Suppose `firstClicked' and `lastClicked'
store the words in the clicked list elements, then you can test whether
there is a match:

  if (lastClicked === kwords.map[firstClicked])
  {
    // ...
  }
  else
  {
    // ...
  }

I recommend not to bother your users with alert() messages (at least not
always); it indicates an error/problem where there might not be none and
drags the user's attention off from what was clicked, which is disturbing..
Use accessible Web-safe colors and at least one other style to indicate
correctness (or the opposite).

I also worry about using the ul as the container for the whole thing.

It might be better to use two UL elements, indeed.

Yes, 2 UL elements would certainly provide an easy fix. Will do.

As for your script:

- Why do you mix `new Array()' and `[]'?
- There are too many global variables.
- Math.random() needs a bugfix if it it returns 1.0.
- Your indentation sucks^W can be improved.
- You are using too many unnecessary parentheses around expressions
  for my taste.
- You are not feature-testing anything, which is error-prone.
- You use identifiers for constructors to refer to simple objects.
-   var i, item;
    for (i = listEvents.length - 1; i >= 0; i = i - 1)
    {
      item = listEvents;

  can be rewritten more efficient as

    for (var i = listEvents.length; i--
    {
      var item = listEvents;

- Your addEvent() is flawed, W3C's addEventListener() and MSHTML's
  attachEvent() are not equivalent.

Actually, the code deals with either attachEvent and addEvent (look
more
closely).

- There are too many lookups for the same reference, which makes this
  inefficient and hard to maintain.
- Your getXHR() is error-prone, exception-handling is not supported in
  all UAs that support XHR.  The method is also needlessly inefficient:
  you do not need advanced features of MSXML 3.0 here, so testing for
  Microsoft.XMLHTTP suffices.
- Your handleReadyState() makes no sense to me: The callback assigned
  to the `onreadystatechange' property is automatically called when the
  readyState changes; you don't have to wait for that.

The code retrys if an ajax call fail, so there is a reason for the way
it is.

- ISTM you do not need XHR here, see above.
- You use `innerHTML', which is proprietary and error-prone, needlessly.

All say innerHTML is very, very cross-browser.

Search the newsgroup's archives for details.  And try applying
<http://www.jslint.com/> to your code.  (Some messages are
but a matter of taste, some are based on good recommendations.)

That should be enough homework for today.

You are generally right (when you do not miss a feature or two or
three),
so thanks.

 

[lorlarz]

I like a lot of your feedback.





I am considering just a little extra code (including one more array),
so this
is not a problem.

I did make the change, so now any value for the "known" words (kwords)
should work.
className no longer holds the correct answers values in the lower ul.

I suggest you use a mapping object instead:

var kwords = {
items: ["Shark", "Lion", "Dolphin", "Squirrel"],
map: {
Shark: "Tiburón",
Lion: "León",
Dolphin: "Delfín",
Squirrel: "Ardilla"
}
};

While that seems like a good suggestion, note that the application
takes in
a JSON array to _swap out_ sets. Can you write this to be a valid
part
of such a 5 piece JSON array of objects?

I am still interested about this, but my randomization works fine.

[snip]

Yes, 2 UL elements would certainly provide an easy fix. Will do.

And, indeed I did it. I now use 2 lists.

[snip]

That should be enough homework for today.

You are generally right (when you do not miss a feature or two or
three),
so thanks.

Well, you get what you paid for.

<http://jibbering.com/faq/>

Hey, if you guys are so smart, then why am I the first one to build
both a universal
JS automatic drag and drop maker and the first automatic multi-
featured JS matching
activity maker? This is what I would like to know.

 

[lorlarz]

Looking for feedback on Matching Exercises Maker/ Builder:

http://mynichecomputing.com/ReadIt/translateT.html

<http://validator.w3.org/check?uri=http://mynichecomputing.com%2...>
<http://jigsaw.w3.org/css-validator/validator?profile=css2&warning=2&u...>
<http://www.w3.org/QA/Tips/color> [snip]

Kind feedback would be appreciated (I am a little tired of harsh feedback
from newsgroups).

Well, you get what you paid for.

<http://jibbering.com/faq/>

PointedEars

Dear PointedEars
I took what feedback of yours I could. I took care of the list
problem and have 2 lists.
I took action against the 1.0 problem. I made less global. I made
arrays with [] . I store
correct answers without using classNames. I also added some
flexibility to the program.
And, I took care of an unnoticed bug (which I won't bring up; but it
was stupid).

Your feedback again or that of others would be appreciated. Thanks
already. And,
thanks again in advance.