-1

Below is a JS function:

I'd like to update this so it uses jQuery but I am having difficulty figuring out how to translate it.

function pathContents(fileList) {
    var list = document.createElement('ul');
    for (file in fileList) {
        var item = document.createElement('li');
        item.appendChild(document.createTextNode(fileList[file]));
        list.appendChild(item);
    }
    return list;
}

UPDATE: Removing the call to pathContents() and replacing it with Malvolio's code - this is the browser output -

enter image description here

Why isn't it displaying the data as list items in an unordered list?

knot22
  • 2,648
  • 5
  • 31
  • 51
  • 1
    Why converting it into jQuery? What's wrong with that code snippet? If you want to create a jQuery object, pass the returned value to jQuery: `$( pathContents(fileList) )` and done :) – Ram Jul 24 '16 at 21:35
  • Don't fix anything that is not broken – trincot Jul 24 '16 at 21:37
  • 1
    Why? Vanilla JS is more performant than jQuery. – ryanlutgen Jul 24 '16 at 21:40
  • Because I'm trying to learn jQuery and doing a translation is a useful exercise in the learning process. – knot22 Jul 24 '16 at 21:41
  • JQuery isn't something "else". It's still Javascript. So translating "from JS to JQuery" doesn't make much sense. You're just rewriting it to use a Javascript library. – Ingo Bürk Jul 24 '16 at 21:42
  • 1
    @ryanlutgen - well, you could write AJAX in js without jQuery but it wouldn't be as short, maintainable, readable and configurable easily as jQuery. but generally it's true, but not even 90%. much less, jquery is very very performant and in many cases performance isn't as important as readability – vsync Jul 24 '16 at 21:59
  • 1
    Note that your update exposes a misunderstanding of what the code @Malvollio posted is doing - this will not work - you're not going to return a list but a function.. That code is meant to **replace** your `pathContents` function, not go inside it. – George Mauer Jul 24 '16 at 23:00

4 Answers4

2

The jQuery equivalent "all-the-way" would be:

function pathContents(fileList) {
    var $list = $('<ul/>');
    $.each(fileList, function () {
        $('<li/>').text(this).appendTo($list);
    });
    return $list;
}

The use of this inside $.each is often seen in jQuery code, but it is cleaner to use the function arguments instead:

    $.each(fileList, function (_, file) {
        $('<li/>').text(file).appendTo($list);
    });

But the pure JavaScript way really is not that bad. For one, you cannot beat the performance of it.

trincot
  • 317,000
  • 35
  • 244
  • 286
  • is the `text(fileList[file])` will eliminate problems that I'm fixing with regex? – spirit Jul 24 '16 at 21:49
  • The Console shows ReferenceError: file is not defined. – knot22 Jul 24 '16 at 21:51
  • btw, in your code `fileList[file]` will be `undefined`... You've forgot to add function parameter, like this: `function (file) { ... }` when you passing callback to the `$.each()` – spirit Jul 24 '16 at 21:51
  • @spirit: Ah yes, the best way is to use `this`: that really is how jQuery works. Thanks. Now the code really looks nice – trincot Jul 24 '16 at 21:53
  • 1
    @spirit, I missed your first comment: yes the text method takes care of this, as it comes down to assigning to the `textContent` DOM property, which does not interpret text as HTML, but as .... plain text. – trincot Jul 24 '16 at 22:07
  • I disagree with using `this` in the `$.each`. Essentially you just opt to use a variable that you can't name instead of one that you can. jQuery doesn't "work that way", in fact in 90% of cases in jQuery, you can use a named parameter instead of `this` - the fact that people insist on using the context parameter is just bad culture. – George Mauer Jul 24 '16 at 22:08
  • Also, you don't need closing `/` in the element factories - that's another case of people just seeing bad code in blogs and going with it. Check the jQuery source, it doesn't care if there's a slash there or not. – George Mauer Jul 24 '16 at 22:10
  • @GeorgeMauer, closing html tag is not bad code. it just **valid xml**. – spirit Jul 24 '16 at 22:11
  • The jQuery documentation seems to give [no preference](http://api.jquery.com/jQuery/#creating-new-elements) for adding the slash or even the complete closing tag, while SO shows that [everyone has their opinion](http://stackoverflow.com/a/268520/5459839) on it. ;-) – trincot Jul 24 '16 at 22:15
  • I see your point about `this`, @GeorgeMauer, although the function argument of interest is the second one, so it would look like `function (_, file)`, which ... well, ... is a bit of a pity. – trincot Jul 24 '16 at 22:20
  • @trincot yes, you're correct, there's no preference in the docs - that's why I said [check the jquery source](https://github.com/jquery/jquery/blob/master/src/core/var/rsingleTag.js). It basically does a regex that ignores terminating slashes. There's no preference expressed over not doing `$('
      `) either, its just ignored.
      – George Mauer Jul 24 '16 at 22:31
    • Why is function argument of interest the second one? I got lost on that part. What is the first argument? I see in the code the first argument is represented as _ but don't understand why. – knot22 Jul 24 '16 at 22:34
    • 1
      The first argument is de index in the array, and the second is the value of the array element at that index. So if you would name the first argument `i` you could write `fileList[i]`, but with the second argument, the same is written by just using that argument: `file`. – trincot Jul 24 '16 at 22:36
    • 1
      @knot22 so basically a long time ago, the jQuery team screwed up - when you're using `$.each` it would make sense to have it be `$.each(array, function(value, index) { })` which is how it works everywhere else, but they screwed up and inverted the two parameters. `_` is just a valid variable name that by convention we know to indicate "this isn't used". (Technically btw the jQuery team didn't screw up - they did this before it was conventional everywhere else). – George Mauer Jul 24 '16 at 22:37
    • 1
      Yep, I suppose you could phrase it like that ;-) – trincot Jul 24 '16 at 22:38
    1

    you can try following :):

    function pathContents(fileList) {
        var list = $('<ul />');
        for (file in fileList) {
            list.append($('<li />').text(fileList[file]));
        }
        return list;
    }
    

    or you can return list.html();

    And yes, as many users mentioned, you will have some performance loss compared to pure javascript

    spirit
    • 3,265
    • 2
    • 14
    • 29
    • Let's just hope there is no ampersand or less-than sign somewhere in that file list. – trincot Jul 24 '16 at 21:39
    • Or a file named `` – Michael Lorton Jul 24 '16 at 21:43
    • Nah, that way lies madness. You may _think_ you have caught every case, but you'll find out when someone hacks your site. Use the library-provided tools, like `.text()`. – Michael Lorton Jul 24 '16 at 22:01
    • @Malvolio, and you really think that giving me -1 is a good idea??? So why you didn't give -1 to *vsync* than? he is not using anything at all =) – spirit Jul 24 '16 at 22:05
    • Just because I didn't see vsync's answer. Any answer that encourages _security flaws_ is a bad answer. Your answer, as amended, isn't terrible, but much better to use proper libraries. – Michael Lorton Jul 24 '16 at 22:44
    • @Malvolio, ok. Now what's the problem with my code? Is that that I'm using `for` and not an `$.each()`? – spirit Jul 25 '16 at 07:32
    • _Personally_ I prefer `.map` or `$.each`, but `in` is not a security hole (the way constructing HTML from input would be). Upvoted. – Michael Lorton Jul 25 '16 at 20:06
    0

    The modern version of your function would be

    let pathContents =
        fileList => 
             $('<ul/>').append(fileList.map(file => $('<li/>').text(file)));
    

    This version uses jQuery to create and append to DOM objects, the fat-arrow notation for functions, and the .map method on lists. It doesn't rely on the unreliable in operator and avoids the Bobby Tables exploit.

    I kept your function and variable names, but I think you should consider more descriptive names.

    Michael Lorton
    • 43,060
    • 26
    • 103
    • 144
    • In the console it says TypeError: fileList.map is not a function. – knot22 Jul 24 '16 at 22:04
    • @knot22 then either you're using a [really really old browser](http://caniuse.com/#search=map) or `fileList` is not an array as your code implies. – George Mauer Jul 24 '16 at 22:12
    • Browser is Firefox 47.0. – knot22 Jul 24 '16 at 22:13
    • @knot22 so your code implied that `fileList` as an array, but if you're on 47 it is not. That is why Malvolio said to use `map`. You can instead do `$.map(fileList, file => $('
    • ').text(file))` which will operate just on the values of `fileList` which I assume is an object. If it still doesn't do what you want it to do, you might want to specify what type `fileList` is
    • – George Mauer Jul 24 '16 at 22:15
  • Strange...I was sure it was an array. I will go look. – knot22 Jul 24 '16 at 22:17
  • The output from the parsed JSON is {"fileList":["Queries","Scripts","Sprocs","Tables","Views"]} – knot22 Jul 24 '16 at 22:22
  • @knot22 open up the console and check what `Array.isArray(fileList)` gives you – George Mauer Jul 24 '16 at 22:33
  • I added that as the first line of the pathContents() function but nothing appeared in the Console. Also tried putting it in the function that calls pathContents() and nothing showed in the Console either. Evidently I am doing something wrong. – knot22 Jul 24 '16 at 22:40
  • You need to add `console.log(Array.isArray(fileList))` if it's in the program itself. – Michael Lorton Jul 24 '16 at 22:42
  • Yeah, sorry for not being very specific - you might want to take a minute and watch a video on how to use the debugger to halt code execution at a certain point so that you can look around (though fair warning - in my experience the Firefox debugger is one of the worst out there and frequently just plain doesn't work) – George Mauer Jul 24 '16 at 22:46
  • Made the update recommended by Malvolio but the Console still shows nothing. – knot22 Jul 24 '16 at 22:46
  • @GeorgeMauer -- are you talking about Firebug? My experience with it has been entirely positive. – Michael Lorton Jul 24 '16 at 22:47
  • I switched over to Chrome and then `console.log(Array.isArray(fileList)` worked fine. It is returning true. – knot22 Jul 24 '16 at 22:50
  • @Malvolio both Firebug and the built-in one. Nearly threw my computer out the window a few years ago after the 50th time it failed to stop on a breakpoint or expose a variable in console, or swallowed a console.log. It's happened to several coworkers too. Might depend on the Operating System though. I like Firefox in theory, but they are by far the most frustrating experience with browser bugs I've had since IE6 days (try doing anything with building up custom iframes). Anyways, waay off topic. – George Mauer Jul 24 '16 at 22:51
  • @knot22 then `fileList.map` should definitely exist and should definitely be a function - are you sure that you copied the original code correctly? – George Mauer Jul 24 '16 at 22:52
  • I will edit my original post so it's visible. I was not sure if this code was supposed to be placed in the pathContents() function or if it was meant to be implemented some other way. – knot22 Jul 24 '16 at 22:54
  • @knot22 this code *is* the `pathContents` function - note the `let pathContents = ..` - that creates a function and puts it into a `pathContents` variable. Everything else is the function body using es6 arrow functions. Which by the way won't work in Firefox 47 or Chrome < 51. You're coming to Javascript at a weird time. The language saw its largest update of syntax ever last year and many people still don't know how to use the new sytnax, what it means, nor which browsers natively support it (and how to get around that with transpilers). – George Mauer Jul 24 '16 at 22:57
  • Ah ha! OK, I replaced the line that called pathContents() to the code exactly posted by @Malvolio above and the browser output is nearly there. It's just not being displayed as list items. I'll edit my original post with a screen shot so you can see the output now. – knot22 Jul 24 '16 at 23:02