2
\$\begingroup\$

Currently I'm making a website that has a to-do list that will probably become quite long to write in html. So I decided to make a shorter version for me to write that I find more comfortable to read.

What I am currently doing is using ajax to read the text file from the server. Once the file has been stored into a string, it's sent to a function which formats it correctly for html. After that, it's returned into a variable that is now stored into a container div using jquery div.

This is my javascript code:

var todoListMake = function( reading ){
        var wanted =reading.split("\n");
        var newString = "";
        for( var i = 0; i < wanted.length; i ++ )
        {
            if( wanted[i] === "\n" )
                continue;
            /* Place text formatting code below */
            wanted[i] = wanted[i].replace("<b>","<strong>");
            wanted[i] = wanted[i].replace("</b>","</strong>");
            wanted[i] = wanted[i].replace("<c>","<span class=\"cutWord\">");
            wanted[i] = wanted[i].replace("</c>","</span>");

            if( wanted[i].search("[X]") != -1 )
            {
                wanted[i] = "<span class=\"workDone\">" + wanted[i].replace("[X]","") + "</span>";
            }

            if( wanted[i].search("<ul") === -1 && wanted[i].search("</ul") === -1 )
                wanted[i] = "<li>" + wanted[i]; // Open the list if no ul tags

            if (i === (wanted.length-1) )
            {
                newString += wanted[i];
                break;
            }// Break early because the rest cannot be processed correctly

            if( wanted[i].search("</ul") != -1 )
                wanted[i] += "</li>"; // Close the list at the end of each ul

            if ( wanted[i + 1].search("</ul") != -1 )
                wanted[i] += "</li>";// Close the list of regular data

            //alert(wanted[i]);
            newString += wanted[i];
        }
        return newString;

};

window.onload = function(){
        $.get("textData/todoData.txt", function(data) {


 var compiledData = todoListMake( data );
            $("#todoList").append( compiledData );
        });
    };

And then read from the text file like this:

<ul  id="todoList" class="workList">
    <h2>Release 1:</h2> <span class="important"> Completed </span>

    <h2>Release 2: <span class="important"> Current Version </span></h2>
        <ul>    
            <b>Core Additions</b>
            <ul>
                [X] Save Progress
                [X] <c>14</c> 9 New Levels
            </ul> 

            <b>Programmers Check List</b>
                <ul>
                    [X] Add functionality to Level Restart
                    [X] Add checkpoint floater
                </ul> 

            <b>Bug Fixes</b>
                <ul>
                    [X] Hanging on Slopes teleport you
                    [X] Hanging on Slopes froze the game
                    [X] Fixed Menu overlay problem
                </ul>
        </ul>
</ul>
\$\endgroup\$
  • \$\begingroup\$ Why would you do it like this? Use the DOM, check out innerHTML. \$\endgroup\$ – elclanrs May 1 '14 at 1:50
  • \$\begingroup\$ @elclanrs I used $.append to edit the html inside the div because I read somewhere in the past that innerHTML some times destroys other tags and data. \$\endgroup\$ – Andrew May 1 '14 at 3:34
  • \$\begingroup\$ I'd consider an XSLT transformation much better for this. Of course you'd have to build the HTML beforehand using the transformation, but I consider that cleaner than building the content using javascript (causing the content to not show at all if something goes wrong). \$\endgroup\$ – dancek May 1 '14 at 6:38
  • \$\begingroup\$ Perhaps consider using a static website generator: no Ajax requests, no JS required, and overall it's a cleaner solution. Your current way of doing things is just… ugh. \$\endgroup\$ – slikts May 1 '14 at 12:50
  • 1
    \$\begingroup\$ The readability of which code are you worried about? The code you write, or the finished HTML code on the web site? The latter is irrelevant and all you are doing is sacrificing user experience and over all site quality, because you are lazy - and not the good kind of lazy, because you put a lot of unnecessary work in your custom parsing function. There are plenty of great template engines, which do exactly the same you are doing manually with help of macros/mixins/helper functions. Which programming languages are you most comfortable with? \$\endgroup\$ – RoToRa May 2 '14 at 9:29
1
\$\begingroup\$

What do you mean with the variable name wanted? I don't understand what it has anything to do with what you are doing, so it's a badly chosen name.

if( wanted[i] === "\n" )
    continue;

This won't do anything. Since you split by "\n", there will be no line breaks leftover. I assume you want wanted[i] === "". Also consider .trim()ing all you lines, otherwise a stray white space will break this.

if (i === (wanted.length-1) )
    {
        // [...]
        break;
    }// Break early because the rest cannot be processed correctly

I'm not sure why you are "breaking early" here. Your condition triggers for the last line, so it's not really "early".

newString += wanted[i];

Repeated string concatenation is a slow process and uses a lot of memory. Instead just leave your strings in the array and at the end just return wanted.join();

EDIT: One more thing: Both the original div and the other ul you later add inside have the same id. That's invalid.

EDIT: Regarding the ID: In your script you have $("#todoList").append( compiledData ); so there's obviouly an element with the id todoList, but your template has the code <ul id="todoList" class="workList"> in it, so you are creating a second element with the same ID.

\$\endgroup\$
  • \$\begingroup\$ Alright, I agree with the variable name wanted it's not very appropriate. Also, I like the Idea of trimming so I will use that as well. I'm a bit confused on the ID, I don't remember placing any. I broke early because right after that the code is set to close a <LI> and the last line is the closing <ul> so a </li> would break the html with closing an unopened <li>. And the line after that checks I+1 out of the array size and will cause an error in the javascript. \$\endgroup\$ – Andrew May 2 '14 at 13:01
  • \$\begingroup\$ @Lemony-Andrew See 2nd edit. \$\endgroup\$ – RoToRa May 2 '14 at 13:24
  • \$\begingroup\$ very good catch! I would have never noticed without you. \$\endgroup\$ – Andrew May 2 '14 at 14:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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