5
\$\begingroup\$

I personally find it helpful to see what aspects of a program I already implemented and what features I need to add. Just for fun, I decided to write up a program to do that. This is the first program for which I tried to write clean looking code. Sorry for the long, overcomplicated code.

The way the program stores/retrieves data is weird and inefficient mainly because I don't know a better way.

The program is html and javascript communicating to a Python webserver. I also hosted the program on the webserver to make cross origin stuff easier to deal with. Unfortunately all the filenames are hardcoded into the program.

webserver.py:

from BaseHTTPServer import BaseHTTPRequestHandler, HTTPServer
import SocketServer

startData = "###DATAGOESHERE###"
endData = "###ENDDATAGOESHERE###"

def readFile(fileName):
    with open(fileName,'r') as f:
        data = f.read()
        f.close()
    return data

def replaceWithData(string, data):
    data = data.strip()
    if data.strip() == "":
        return string.replace(startData, "").replace(endData, "")
    else:
        start = string.find(startData)
        end = string.find(endData)+len(endData)
        string = string[:start]+data+string[end:]
        return string

def writeFile(fileName, data):
    with open(fileName,'w') as f:
        f.write(data)
        f.close()

def serveStaticApp(request, url):
    data = readFile('data.html')
    html = readFile('main.html')
    html = replaceWithData(html, data)
    request.wfile.write(html)

def serverStaticFile(request, url):
    data = readFile(url)
    request.wfile.write(data)

def saveData(request, url):
    length = int(request.headers.getheader('content-length'))
    data = request.rfile.read(length).strip()
    writeFile('data.html', data)

funcMap = {
    "app": serveStaticApp,
    "main.js":serverStaticFile,
    "save": saveData,
    "main.css": serverStaticFile,
}

def handleRequest(request):
    request._set_headers()
    for i in funcMap.keys():
        if request.path[-len(i):] == i:
            funcMap[i](request, i)
            return
    request.wfile.write("<html><body><h1>REQUEST!</h1></body></html>")

class S(BaseHTTPRequestHandler):
    def _set_headers(self):
        self.send_response(200)
        self.send_header('Content-type', 'text/html')
        self.end_headers()

    def do_GET(self):
        handleRequest(self)

    def do_HEAD(self):
        self._set_headers()

    def do_POST(self):
        handleRequest(self)


def run(server_class=HTTPServer, handler_class=S, port=8080):
    server_address = ('localhost', port)
    httpd = server_class(server_address, handler_class)
    print 'Starting...'
    httpd.serve_forever()

if __name__ == "__main__":
    from sys import argv

    if len(argv) == 2:
        run(port=int(argv[1]))
    else:
        run()

main.html

<!DOCTYPE>
<html>
<head>
    <link rel="stylesheet" href="main.css" type="text/css">
</head>
<body>
    <div>
        <span id="not-implemented-label">Not Implemented:</span>
        <span id="implemented-label">Implemented:</span>
    </div>
    <span id="save">
        ###DATAGOESHERE###
        <select id="not-implemented-select" size="2" multiple="true"></select>
        <span id="controls">
            <input type="button" value="<-" id="left-arrow-button">
            <input type="button" value="LOAD" id="load-button">
            <input type="button" value="->" id="right-arrow-button"><br>
            <input type="button" value="DELETE" id="delete-button">
        </span>
        <select id="implemented-select" size="2" multiple="true"></select>
        ###ENDDATAGOESHERE###
    </span>
    <br>
    <br>
    <input type="text" id="title-input" placeholder="Title"><br>
    <textarea id="description-input" placeholder="Description"></textarea>
    <span id="save-controls">
        <input type="button" value="SAVE OVER" id="save-over-button"><br>
        <input type="button" value="SAVE NEW" id="save-new-button"><br>
        <input type="button" value="CLEAR" id="clear-button">
    </span>
    <script src="functions.js"></script>
    <script src="main.js"></script>
</body>
</html>

main.css

select{
    width:40vw;
    height:60vh;

}
textarea{
    width:80vw;
    height:30vh;
}
#title-input{
    width:80vw;
}
#controls{
    display:inline-block;
    width:8em;
    text-align: center;
}
#save-controls{
    display:inline-block;
}
#not-implemented-label{
    display:inline-block;
    width:calc(40vw + 8em);
}
.description{
    display:none;
}

main.js

"use strict";
var mostRecentlySelectedOptions = [];
var allowTextOverrride = true;
var saveServerURL = "/save"

var cachedElements = {};
function gebi(id){
    var cachedElement = cachedElements[id];
    if(cachedElement === undefined){
        cachedElement = document.getElementById(id);
        cachedElements[id] = cachedElement;
        return cachedElement;
    }else{
        return cachedElement;
    }
}

function slice(collection){
    return [].slice.call(collection);
}

function moveOptions(options, toSelect){
    options = options.slice();
    for(var i = 0; i < options.length; i++){
        toSelect.appendChild(options[i]);
    }
    saveToServer();
}

function setInputs(title, description){
    allowTextOverrride = true;
    gebi("title-input").value = title;
    gebi("description-input").value = description;
}

function getInputs(){
    return {
        "title": gebi("title-input").value, 
        "description": gebi("description-input").value
    };
}

function getSafeInputs(){
    var inputs = getInputs();
    if(inputs.title.trim() == ""){
        inputs.title = "<untitled>";
    }
    inputs.title = inputs.title.trim();
    inputs.description = inputs.description.trim();
    return inputs;
}

function clearInputs(){
    setInputs("", "");
}

function moveSelectedOptions(toSelect){
    moveOptions(mostRecentlySelectedOptions, toSelect);
}

function onSelectionChange(target){
    mostRecentlySelectedOptions = slice(target.selectedOptions);
    if(allowTextOverrride){
        loadSelectedOption();
    }
}

function deselectAllInSelect(selectElement){
    var selected = selectElement.selectedOptions;
    for(var i = 0; i < selected.length; i++){
        selected[i].selected = false;
    }
}

function saveText(){
    allowTextOverrride = true;
    saveToServer();
}

function saveToServer(){
    var dataToSave = gebi("save").innerHTML;
    var xhttp = new XMLHttpRequest();
    xhttp.open("POST", saveServerURL, true);
    xhttp.setRequestHeader("Content-type", "text/plain");
    xhttp.send(dataToSave);
}

function removeOptions(options){
    options = options.slice();
    for(var i = 0; i < options.length; i++){
        options[i].parentNode.removeChild(options[i]);
    }
    saveToServer();
}

function removeSelectedOptions(){
    removeOptions(mostRecentlySelectedOptions);
}

function setOptionData(option, title, description){
    option.text = title;
    option.setAttribute("data-description", description);
}

function getOptionData(option){
    return {
        "title": option.text, 
        "description": option.getAttribute("data-description")
    };
}

function createOption(title, description){
    var option = document.createElement("option");
    setOptionData(option, title, description);
    return option;
}

function addOption(title, description, parentSelect){
    var option = createOption(title, description);
    parentSelect.add(option);
    deselectAllInSelect(parentSelect);
    option.selected = true;
}

function saveNew(){
    var inputData = getSafeInputs();
    var parentSelect = gebi("not-implemented-select");
    addOption(inputData.title, inputData.description, parentSelect);
    saveText();
}

function loadSelectedOption(){
    if(mostRecentlySelectedOptions.length == 1){
        var selectedOption = mostRecentlySelectedOptions[0];
        var data = getOptionData(selectedOption);
        setInputs(data.title, data.description);
    }
}

function saveOverwrite(){
    if(mostRecentlySelectedOptions.length == 1){
        var selectedOption = mostRecentlySelectedOptions[0];
        var inputData = getSafeInputs();
        setOptionData(selectedOption, inputData.title, inputData.description);
        saveText();
    }else{
        saveNew();
    }
}

function addEventListeners(){
    var notImplementedSelect = gebi("not-implemented-select");
    var implementedSelect = gebi("implemented-select");

    gebi("right-arrow-button").addEventListener("click", function(){
        moveSelectedOptions(implementedSelect);
    });

    gebi("left-arrow-button").addEventListener("click", function(){
        moveSelectedOptions(notImplementedSelect);
    });

    gebi("load-button").addEventListener("click", function(){
        loadSelectedOption();
        allowTextOverrride = false;
    });

    gebi("delete-button").addEventListener("click", function(){
        removeSelectedOptions();
    });

    gebi("save-over-button").addEventListener("click", function(){
        saveOverwrite();
    });

    gebi("save-new-button").addEventListener("click", function(){
        saveNew();
    });

    gebi("clear-button").addEventListener("click", function(){
        clearInputs();
    });

    implementedSelect.addEventListener("change", function(event){
        onSelectionChange(event.target);
    });

    implementedSelect.addEventListener("focus", function(event){
        onSelectionChange(event.target);
    });

    notImplementedSelect.addEventListener("change", function(event){
        onSelectionChange(event.target);
    });

    notImplementedSelect.addEventListener("focus", function(event){
        onSelectionChange(event.target);
    });

    gebi("title-input").addEventListener("keypress", function(){
        allowTextOverrride = false;
    });

    gebi("description-input").addEventListener("keypress", function(){
        allowTextOverrride = false;
    });

    window.addEventListener("beforeunload", function(){
        //alert();
    });
}

addEventListeners();

To run it, put all the files into the same directory and run the Python file. You also need a blank file named data.html in the same directory. Then visit http://localhost:8080/app to access the app.

You can put a small title for the task and a longer description of the task and click SAVE NEW. SAVE OVER will save over whatever one you have selected. The two arrow buttons move the task between columns. It is important to note that the webserver should not be run over the internet.

I'm particularly interesting in the following:

  • Are my variables/functions well named?
  • Are my functions doing too much or too little?
  • Are there cases that you can get unexpected behavior?
  • Do you feel that the code is easily extendable/maintainable?
  • What can I do about the long list of addEventListeners?
\$\endgroup\$
0

1 Answer 1

Reset to default
3
\$\begingroup\$

HTML

Two instances of the same problem. You can actually see it in the markup but using < and > makes the syntax highlight weird. This is because you shouldn't be using these characters - some tools can (incorrectly) interpret them as tags. Instead use their HTML entity codes

  • &lt; for < - "lt" stands for "less than"
  • &gt; for > - "gt" stands for "greater than"

It's not a huge issue, as any browser would still work with that but you can see how syntax highlighters can be tripped.

JavaScript

Small Things

  • Typo in your variable name: allowTextOverrride has too many rs - the correct would be allowTextOverride
  • Check for missing semicolons - the line var saveServerURL = "/save" doesn't have one in the end. I don't think I saw any others but still. It's a minor inconsistency in the code.
  • Avoid names like gebi. I had to guess that it's short for "getElementById` but that was after seeing what it does. It's not completely clear when looking at when it's used
  • You don't actually need gebi() at all. I understand the idea - it's for looking up items and caching them so subsequent lookups would be faster. But the browser already does that. In fact, it does it better - all lookups by ID would be optimised from before you make them. When the DOM is built up all IDs will be cached, so when invoking document.getElementById you'd get a constant time lookup. At least that's the behaviour in most browsers. I suppose some might do a search in the page with every invocation that but I don't think it's worth optimising for those.
  • Related to you not needing gebi() - there is very little that function does to be useful. You call it with the ID every time, for example gebi("title-input"), so if the ID changes, that means you have to find all invocations and correct them. You could instead have something like function getTitleInput { return document.getElementById("title-input"); } which would actually save you repeated code.
  • Minor redundancy in getSafeInputs you do a .trim() on inputs.title twice. You can clean it up by first doing inputs.title = inputs.title.trim(); and then checking if it's empty and setting a default value if so. You can also do it in one line, if you really want to inputs.title = inputs.title.trim() || "<untitled>"; is idiomatic JavaScript assignment that will set the value to the fallback string "<untitled>" if the first option evaluates as falsey. And since that value is always going to be a string, then the only string that evaluates as falsey is the empty string "". With that said, you are allowed to prefer an explicit if check instead of a one line assignment using a boolean operator.
  • Don't use alert() for debugging. I know it's commented out but it's worth mentioning - alert() is only useful for extremely rough debugging. But it has a lot of problems like pausing other scripts execution. This can give you an incorrect information about what's going on when using alert() in some cases. Use console.log - it's unobtrusive and even if you leave it in, it's not a problem for the users. It's also easy to silence, if needed - you can overwrite the console.log function in production, if you find it's too chatty.
  • Don't leave code in comments - it's almost inevitable when developing to quickly enable/disable some functionality with comments. That's why all editors have a very easy shortcut for commenting out lines. Still, if you are ready with development, all code should be enabled and all comments should be whatever is relevant.

Larger Problems

  • Don't use global variables. Your mostRecentlySelectedOptions, allowTextOverrride, and saveServerURL = "/save", along with all the functions are in the global namespace. You can use the module pattern to enclose your code and insulate it from the global scope. There are many different implementations of it for various purposes but in essence, all you need to do is wrap your code in a function expression like so (function() { /* all the code goes here */ })(). Here are the basics - JS only really has two scopes - global and functional. Once you put your code inside a function (in this case an anonymous one), it's no longer in the global scope. The last () executes the function and this the code inside it is live. The only difference is that anything inside the function doesn't immediately go to the global scope. The visibility inside is still exactly the same. On a related note the construct (function() { })() is called an immediately invoked function expression. Or shortened as IIFE. In case you come across it in the future, it's that - a function that's defined and executed at the same time.
  • Related to the global scope above but I wanted to have a separate point - don't enable strict mode in the global scope. Don't get me wrong - it's good to use strict mode but if you define it in the global scope then all scripts on the page would use it. This includes third party libraries, which can break them if they use some features from "loose mode" but suddenly those throw an error. You don't have third party code now but you may in the future - either in this project or another one. So, this is a good practice.

To answer your questions

Are my variables/functions well named?

Yes, for the most part, they are. As mentioned gebi is too short but also it's not needed. Some could be improved - like setInputs - it sets two specific inputs and only their values. So the name can be a bit misleading. However, I'm not very good with names - I'm not sure how to name them better (I'm not good with names...). Overall, the names serve but can be made a bit more descriptive.

Are my functions doing too much or too little?

As mentioned gebi does too much and is redundant as it does what the browser already does.

I'm also a bit wary of saveOverwrite. It has two completely different branches and so it's actually to (code-wise) unrelated functions in one. Only one of them would run and it's governed by an external attribute. It probably needs to be cleaned up.

Some of the others probably fall under "too little" but I am not sure how to improve them. So, I'd let it fly. I don't think it's a huge issue. Or probably any.

Are there cases that you can get unexpected behavior?

The biggest problem is that global "use strict". To a lesser extent, the global functions. I can't think of anything else right now that could be an unexpected behaviour. With that said, there is a "special case" here of what happens if an ID changes? All the code assumes the IDs exist, which is fine, but if an ID is different, then you'd get a bunch of errors. Having a getter function for each ID (e.g., the getTitleInput I mentioned earlier) can allow you to have a slightly better error handling. Even if it's nothing more than doing a console.error("ID doesn't exist"). It would also be easier to fix by just changing the function.

Still, that might be a bit of an extreme. A lot of times you put an ID in the HTML because you want to use it and you aren't going to just change it. So it's also OK to leave the code as it is looking stuff up without trying to handle the very unlikely scenario of the HTML changing and the JS code being left as-is.

Do you feel that the code is easily extendable/maintainable?

Perhaps. Depends on what you need to do. But one big problem I neglected to mention earlier (since it is rather specific to this) is the setInputs function. It takes two parameters, which are the two inputs you want to set the value of. But what happens if you now have three inputs? What if you only want to set some of them? I'd suggest passing in an object where the key is the input you want (perhaps the ID or just some identifier you map yourself) and the value is what you want the element's value set to. For example

function setInputs(inputs){
    allowTextOverride = true;
    if (inputs.hasOwnProperty("title")) {
        document.getElementById("title-input").value = inputs.title;
    }
    if (inputs.hasOwnProperty(description) {
        document.getElementById("description-input").value = inputs.description;
    }
}

This way, if you have to care about more elements, you don't have to have a monolith and hard to read signature of the function.

However, if that expands a lot more, I'd advise you to get some frontend library that can handle the logic and view separation. It need not be something giant like Angular or React - those might be an overkill, but you might look at, for example, KnockoutJS or something else that allows databinding between HTML and a JavaScript model.

What can I do about the long list of addEventListeners?

You can consolidate some. For example, if you are binding the same event with the same handler but on multiple elements, there is very little reason to just do it separately. For example, you can change 

gebi("title-input").addEventListener("keypress", function(){
    allowTextOverrride = false;
});

gebi("description-input").addEventListener("keypress", function(){
    allowTextOverrride = false;
});

to this

document.querySelectorAll("#title-input, #description-input") //select both elements by their IDs
    .addEventListener("keypress", function(){ //add the same handler
        allowTextOverride = false;
    });

So, you can group them to save space.

Another thing that can save you a few lines is what in Lambda calculus is called Beta reduction. In general if you have function a() { b() } then a() is a bit useless, you can just call b() and get the same effect. So, you can "unwrap" them, if you will. In your case things like

gebi("delete-button").addEventListener("click", function(){
    removeSelectedOptions();
});

have a useless "wrapper" function. You can directly assign the handler to be the removeSelectedOptions function: .addEventListener("click", removeSelectedOptions); note that there aren't () at the end. Because the name is the function reference. If you add (), that executes the function immediately, instead of setting it as the handler here.

Further suggestion outside of a code reviewing the main thing

Consider using ES6 in your code. It's not really new any more and it should be getting more usage. Even if you end up transpiling it into ES5, it's better to have "new code". There is little here that can be straight up improved by ES6 but it's better to practice putting let and const instead of var all the time.

\$\endgroup\$

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.