0
\$\begingroup\$

Challenge

  1. Create a page with
    • An element for number,
    • An image
    • An unordered list tag for list of images
    • A 'Check result' button
    • A text element to show result
  2. Generate random number and put it in (i)
  3. On click the image it add an li with the image inside UL tag
  4. On click check result button, compare random with number of images inside UL, if both are same show Correct answer else wrong answer in result box.

Codepen URL

Random number and Image

HTML

<div class="container">
  <div class="row">
    <div class="col-md-12">
      <h4>Select <span id="randomNumber"></span> <span id="item"></span>s</h4>
    </div>
  </div>
  <div class="row mb-10">
    <div class="col-md-12">
      <img id="actionImage" src="https://images.pexels.com/photos/145939/pexels-photo-145939.jpeg?h=150&auto=compress&cs=tinysrgb" alt="Tiger" />
    </div>
  </div>
  <div class="row">
    <div class="col-md-12">
      <ul id="imgList"></ul>
    </div>
  </div>

  <div class="row">
    <div class="col-md-12">
      <button id="btnResult" class="btn btn-primary">Check Result</button>
      <b id="output"></b>
    </div>
  </div>
</div>

jQuery

$(function() {
    var elements = {
        random: $("#randomNumber"),
        item: $("#item"),
        sourceImage: $("#actionImage"),
        targetImage: $("<img>"),
        imageList: $("#imgList"),
        list: $("<li>"),
        btnResult: $("#btnResult"),
        output: $("#output")
    },
    data = {
        random: Math.floor(9 * Math.random() + 2),
        image: elements.sourceImage.attr("src"),
        item: elements.sourceImage.attr("alt")
    },
    events = {
        init: function() {
            elements.random.text(data.random), elements.item.text(data.item), elements.targetImage.attr({
                alt: "list image",
                src: data.image
            }), 
            elements.list.append(elements.targetImage), 
            elements.sourceImage.click(events.addImage),
            elements.btnResult.click(events.showResult);
        },
        addImage: function() {
            elements.imageList.append(elements.list.clone());
        },
        showResult: function() {
            var result = "Correct Answer";
            elements.imageList.children().length !== data.random &&
            (result = "Wrong Answer"), elements.output.text(result);
        }
    };
    events.init();
});
\$\endgroup\$
2
  • \$\begingroup\$ Your jQuery code is missing \$\endgroup\$ Commented Sep 12, 2017 at 8:51
  • \$\begingroup\$ Why use CodePen to post the executable example, when you could use Stack Snippets to post it here? \$\endgroup\$
    – Barmar
    Commented Sep 12, 2017 at 19:44

1 Answer 1

1
\$\begingroup\$

The code looks clean and well readable and with one exception I would say there is nothing wrong with it per se, however you are using several unusual approaches.


The task asks for

A text element to show result

I'm not sure what a "text element" means here, but it could be an <input type="text">. However that would be wrong, since there are for input and not output. Your choice of a <b> element is better, but HTML 5 provides the <output> element, that would be even more suitable.


Your choice to collect your variables in objects (elements, data, events) seems somewhat arbitrary. It's not wrong, and after getting used to it, it actually helped readability somewhat (except that now almost all lines begin with elements...., which seems a bit repetitive).

Just be aware that it's an unusual thing to do, because people usually expect objects like that to have sort of technical use, instead of just grouping variables.


The use of the comma operator to separate statements (or to be exact expressions) instead of semicolons (or just line breaks) is very unusual. Since the comma operator is very rarely used, it can be confusing for beginners and experts may try to find deeper meaning to it, even when there is none. Especially the first line of init() irritated me for a moment, because there is no line break after the commas, so that it looks like some special is happening, which isn't.


The only point that I consider wrong: The use of && instead of a regular if. This (and the use of the comma operator) looks like you are trying to emulate the kind of "optimizations" that code compressors do. Don't do this. It compromises readablily. If you want to serve compact code, then use a code compressor.

\$\endgroup\$
1
  • \$\begingroup\$ Your point about && is quite appropriate, because he uses it along with the comma operator, and I'll bet many programmers aren't sure about their relative precedences. \$\endgroup\$
    – Barmar
    Commented Sep 12, 2017 at 19:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.