3
\$\begingroup\$

I have written a little Paint-Program in HTML. You can also save and load your wonderful paintings. My code works, but my heart says that my code is not so beautiful. I dont use JS/HTML/CSS very often and thats why I dont know how to write clean JS-Code, but I want to know that. Javascript seems to provide a thousand ways to express your ideas sometimes. I dont know that from Java (I usually write Java Code). How would you improve the code? I am very interested!

/**
 * user can choose color by hisself
 **/

// global color variable
let currentColor = "black";

function createColorTable(colorStringArray) {
	let colorTable = document.getElementById("colorTable");
	
	for (i = 0; i < colorStringArray.length; i++) {
		let colorRow = document.createElement("tr");
		let colorBox = document.createElement("td");
		colorBox.value = colorStringArray[i];
		colorBox.style.backgroundColor = colorStringArray[i];
		colorBox.onclick = function () {
			window.currentColor = colorBox.style.backgroundColor;
		};
		
		colorRow.appendChild(colorBox);
		colorTable.appendChild(colorBox);
	}
}

let colors = [
	'black',
	'brown',
	'red',
	'grey', 'lightgrey',
	'blue', 'lightblue',
	'yellow',
	'green', 'lightgreen'
];

createColorTable(colors);

/**
 * function for drawing table
 **/

function drawPaintingArea(height, width) {
	let paintingArea = document.getElementById("paintingArea");
	// clear table for redrawing
	paintingArea.innerHTML = "";
	for (i = 0; i < width; i++) {
		let row = document.createElement("tr");
		for (j = 0; j < height; j++) {
			let column = document.createElement("td");
			column.style.backgroundColor = "white";
			column.setAttribute("onclick", "tableClickHandler(this)");
			
			row.appendChild(column);
		}
		paintingArea.appendChild(row);
	}
}

function tableClickHandler(box) {
	if (box.style.backgroundColor == "white")
		box.style.backgroundColor = window.currentColor;
	else
		box.style.backgroundColor = "white";
}

drawPaintingArea(20, 20);

/**
 * draw table by button click
 **/
 
let heightTextField = document.getElementById("height");
let widthTextField = document.getElementById("width");
let drawingButton = document.getElementById("drawingButton");
drawingButton.onclick = function() {
	drawPaintingArea(heightTextField.value, widthTextField.value); 
};

/**
 * saving and loading (via buttons)
 **/

let saveButton = document.getElementById("saveButton");
saveButton.onclick = function() {
	let paintintArea = document.getElementById("paintingArea");
	download("save.txt", paintingArea.innerHTML);
}

function download(filename, text) {
	var element = document.createElement('a');
	element.setAttribute('href', 'data:text/plain;charset=utf-8,' + encodeURIComponent(text));
	element.setAttribute('download', filename);

	element.style.display = 'none';
	document.body.appendChild(element);

	element.click();

	document.body.removeChild(element);
}

document.getElementById('fileInput').addEventListener('change', readFileAsString)
function readFileAsString() {
    var files = this.files;
    if (files.length === 0) {
        console.log('No file is selected');
        return;
    }

    var reader = new FileReader();
    reader.onload = function(event) {
        let paintintArea = document.getElementById("paintingArea");
		paintingArea.innerHTML = event.target.result;
    };
    reader.readAsText(files[0]);
}
table {
	border: 1px solid black;
	border-collapse: collapse;
}

td {
	height: 20px;
	width: 20px;
} 
<html>
	<head>
		<title>Malprogramm Web</title>
		<link rel="stylesheet" href="design.css">
	</head>
	
	<body>
		<h1>Web Painting Program</h1>
	
		<div>
			<input id=height value="20" /> Height <br /><br />
			<input id=width value="20" /> Width <br /><br />
			<button id=drawingButton>Draw Field</button></br></br>
		</div>
	
		<h2>Choose Color</h2>
		<table id=colorTable>
		</table>
	
		<br /><br />
	
		<table border id=paintingArea>
		</table>
	
		<br /><br />
	
		<button id=saveButton>Save to file</button><br /><br />
		
		Load: <input type='file' id=fileInput />
		
		<script type="text/javascript" src="main.js"></script>
	</body>
</html>

\$\endgroup\$
2
  • \$\begingroup\$ Tip: Java and JavaScript are not related. \$\endgroup\$
    – Mast ♦
    Commented Apr 15, 2020 at 13:58
  • \$\begingroup\$ Yes, but I also did not say that :D \$\endgroup\$
    – Dexter Thorn
    Commented Apr 15, 2020 at 14:00

1 Answer 1

Reset to default
3
\$\begingroup\$

Some possible improvements:

Inputs are self-closing, as are <br>s. No need for />. (even in the snippet, you can see how the syntax highlighting is off due to this)

When you want to vertically separate elements from each other, usually it's more elegant to put containers (like <div>s, block-level elements) around the elements than to use <br>. You can use <br>s, but I wouldn't recommend it in most cases.

Tables should usually be used only for tabular data. It's not forbidden, but if you're not trying to display tabular data, I don't think it's exactly semantically appropriate, even if it makes writing the styles easier.

You're mixing let and var, and you're never using const. If you're writing modern Javascript, you should never be using var (it has too many gotchas, like function scope instead of block scope, and automatically creating properties on the global object), and preferably, you should almost always use const, not let. Variables declared with let may be reassigned, so when you use them, you're sending a warning to readers of the code that you may reassign the variable name in the future. If you use const instead, a variable name will not be reassignable, resulting in less cognitive overhead required.

You have

for (i = 0; i < colorStringArray.length; i++) {
  // reference colorStringArray[i] here

You have this same pattern in a few other places in the code. There are two issues with all of these:

  • You're implicitly creating a global i variable because you didn't declare it first. This is not only inelegant, but it'll throw an error in strict mode. (Consider enabling strict mode, it'll help you catch bugs earlier)
  • You're having to manually manage the indicies while iterating over the array. This is a bit verbose, has no abstraction benefits, and can occasionally result in off-by-one errors (it's more common than you think). Consider using for..of instead:
for (const color of colorStringArray) {
  // reference color here
}

You have

let colorBox = document.createElement("td");
colorBox.value = colorStringArray[i];

but <td>s should not have .values - only input-like elements should have .values. You can just remove that line.

With

colorBox.onclick = function() {
  window.currentColor = colorBox.style.backgroundColor;
};

It's not an issue here, but I think it's a good idea to get into the habit of using addEventListener instead of on properties. The problem with on properties is that when there are scripts doing separate things, if they want to listen for the same event on an element and they both use on syntax, only the latest listener will actually run:

button.onclick = () => console.log('click 1');
button.onclick = () => console.log('click 2');
<button id="button">click</button>

It's an easy source of bugs. Consider getting into the habit of using addEventListener instead, which can attach any number of listeners.

button.addEventListener('click', () => console.log('click 1'));
button.addEventListener('click', () => console.log('click 2'));
<button id="button">click</button>

You should also never use inline handlers, like with:

column.setAttribute("onclick", "tableClickHandler(this)");

Inline handlers have too many problems. Not only will setAttribute remove any previous attribute handler, inline handlers also have a demented scope chain, require global pollution, and have quote escaping issues. Use addEventListener instead.

You have

colorRow.appendChild(colorBox);
colorTable.appendChild(colorBox);

You're appending the colorBox <td> to the row, then removing it from the row and attaching it to the table instead, and the row never gets used again. You probably wanted to create just a single row which all the <td>s get appended to.

If you wish to dynamically create elements concisely, a possible shortcut is to use appendChild's return value, which is the element that was appended. For example, your createColorTable can be turned into:

function createColorTable(colorStringArray) {
  const colorTable = document.getElementById("colorTable");
  const colorRow = colorTable.appendChild(document.createElement("tr"));
  for (const color of colorStringArray) {
    const colorBox = colorRow.appendChild(document.createElement("td"));
    colorBox.style.backgroundColor = color;
    colorBox.addEventListener('click', () => {
      currentColor = color;
    });
  }
}

This is somewhat opinion-based, but you might consider using classes instead of IDs - any element with an ID automatically creates a global variable with the same name, which can result in strange bugs occasionally.

You might consider making an indicator of which color the user has currently selected, like Paint does, so they know what color's going to be painted without them having to remember it themselves.

You may consider using the conditional operator, it's perfectly suited for cases like your tableClickHandler function when you'd like to create an expression conditionally, then use it:

function tableClickHandler(box) {
  box.style.backgroundColor = box.style.backgroundColor === "white"
    ? window.currentColor
    : 'white';
}

(remember to use strict equality with ===, good to avoid == loose equality)

\$\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.