patternjavascriptMinor
ASCII flow chart drawer
Viewed 0 times
chartdrawerasciiflow
Problem
After some free ASCII flowchart drawer started charging money, I decided to write my own. Salient features are that you can draw a box (mouse down, mouse move, mouse up), then ctrlB will draw a box. Copy/Paste, Undo, Redo, click anywhere and type are all working features. I pasted the main code here, and it relies on two other minor files to provide a cursor and box object, but this code should still be very reviewable.
I did run the code through JSHint and made a judgement call on the few remaining ickies. This is my first project with canvas that's not just a prototype, so any insights there are welcome. Finally, this will become a Chrome extension, so I only care about this working on Chrome. Except when I don't of course (
```
/ Unidraw, because we can/
//Documentation:
// http://en.wikipedia.org/wiki/Box-drawing_character
//Competition:
// http://www.asciidraw.com/#Draw
// http://asciiflow.com/
(function IIFE(){
"use strict";
var canvas,
context,
clipboard;
var model = (function()
{
//Privates
var cells = [],
tabSize = 4;
//Exposed
var cursor = new Cursor();
function write( x, y, s )
{
//Make sure that we have an array for y
//Always assume overwrite mode
var originalX = x;
cells[y] = cells[y] || [];
for( var i = 0; i 31 )
{
cells[y][x++] = s[i];
}
else if ( c == "\n" )
{
y++;
cells[y] = cells[y] || [];
x = originalX;
}
else if ( c == '\t' )
{
x += tabSize;
}
}
return new Cursor( x, y );
}
function setCell( cursor , c )
{
return write( cursor.x , cursor.y , c );
}
function getCell( cursor )
{
return cells[cursor.y] ? cells[cursor.y][cursor.x] || " " : " ";
}
function stringify()
{
var s = '', x, y;
for( y = 0 ; y <'.indexOf( getCell( cursor ) ) ? returnValue : 0;
}
//Modulify
return {
write: write,
stringify: stringify,
setCell: setCell
I did run the code through JSHint and made a judgement call on the few remaining ickies. This is my first project with canvas that's not just a prototype, so any insights there are welcome. Finally, this will become a Chrome extension, so I only care about this working on Chrome. Except when I don't of course (
which).```
/ Unidraw, because we can/
//Documentation:
// http://en.wikipedia.org/wiki/Box-drawing_character
//Competition:
// http://www.asciidraw.com/#Draw
// http://asciiflow.com/
(function IIFE(){
"use strict";
var canvas,
context,
clipboard;
var model = (function()
{
//Privates
var cells = [],
tabSize = 4;
//Exposed
var cursor = new Cursor();
function write( x, y, s )
{
//Make sure that we have an array for y
//Always assume overwrite mode
var originalX = x;
cells[y] = cells[y] || [];
for( var i = 0; i 31 )
{
cells[y][x++] = s[i];
}
else if ( c == "\n" )
{
y++;
cells[y] = cells[y] || [];
x = originalX;
}
else if ( c == '\t' )
{
x += tabSize;
}
}
return new Cursor( x, y );
}
function setCell( cursor , c )
{
return write( cursor.x , cursor.y , c );
}
function getCell( cursor )
{
return cells[cursor.y] ? cells[cursor.y][cursor.x] || " " : " ";
}
function stringify()
{
var s = '', x, y;
for( y = 0 ; y <'.indexOf( getCell( cursor ) ) ? returnValue : 0;
}
//Modulify
return {
write: write,
stringify: stringify,
setCell: setCell
Solution
I like it! I've got some quibbles about the style, but that's personal opinion, and the code works, so I won't go into that. I haven't gone through the code line-by-line (there's a lot!), but I've tried looking at the overall structure.
If I were to suggest something, it might be a more declarative way to handle events, and keyboard event in particular. It's a minor thing, but the sort of thing I find more readable/direct.
You've got a lot of functions that are simply named for the event they handle, but then you have to repeat that name when attaching them to events. I'd consider defining them as properties on an object, and then loop through them, e.g.
Again, it's a minor thing, but defining the event handlers in one place and have them automatically attached would keep things nicely contained, I think.
And you could do a similar thing sort of thing for keyboard events, to avoid the large
(note that you'll have to figure out a way to pass the local var
Aside: It might be a good use-case for the
Alternatively, you could also make an
Basically, you can go more or less in-depth with this, but it'd make it (hopefully) easier to set up keyboard-handling, and (with some modification) use different key combos depending on platform.
For instance, on the Mac, cmd is used instead of ctrl, but checking
You've got a nice separation of model, ui, controller and so forth, but perhaps a bit more encapsulation within each of those would be nice, such as abstracting/encapsulating the key-combo matching.
Semi-related: A long time ago, I wrote something to handle keyboard shortcuts. Maybe you can use it for something. I'm linking it mostly because for some complex combinations, key events start to get weird. I don't think the code quite works right anymore, but the technique might still be viable.
Oh, and one thing I noticed is that making a 1-column vertical selection and drawing it, gave me something like:
═
║
═
which seems a little off. I expected it to only draw vertical pipe glyphs, or, if the top and bottom should be "flat", use the 3-way pipe glyphs for the ends:
║ ╦
║ or ║
║ ╩
If I were to suggest something, it might be a more declarative way to handle events, and keyboard event in particular. It's a minor thing, but the sort of thing I find more readable/direct.
You've got a lot of functions that are simply named for the event they handle, but then you have to repeat that name when attaching them to events. I'd consider defining them as properties on an object, and then loop through them, e.g.
var canvasEvents = {
mouseover: function () { ... }
mousemove: function () { ... }
mousedown: function () { ... }
mouseup: function () { ... }
click: function () { ... }
};
for(var event in canvasEvents) {
canvas.addEventListener(event, canvasEvents[event]);
}Again, it's a minor thing, but defining the event handlers in one place and have them automatically attached would keep things nicely contained, I think.
And you could do a similar thing sort of thing for keyboard events, to avoid the large
else if... else if... structure. For instance,// Not the complete list - just a sampling
// Order still matters of course, so ctrl+Home gets matches before Home
var keyCommands = [
{
mask: { which: BACKSPACE },
preventDefault: true,
handler: model.backspace
},
{
mask: { which: ARROW_UP },
handler: model.cursor.up
},
{
mask: { keyIdentifier: 'Home', ctrlKey: true },
handler: function () { model.cursor = new Cursor( 0, 0 ) }
},
{
mask: { keyIdentifier: 'Home' },
handler: function () { model.cursor.x ? model.cursor.x = 0 : model.cursor.y = 0 }
},
{
mask: { which: KEY_C, ctrlKey: true },
handler: function () { /*... etc ...*/ }
},
....
];
// ....
function handleKeyCombo(event) {
normalizeEvent(event);
function matchMask(mask) {
for( var property in mask ) {
if(event[property] != mask[property]) { // maybe use a strict comparison; your call
return false;
}
}
return true;
}
for( var i = 0, l = keyCommands.length ; i < l ; i++ ) {
var command = keyCommands[i];
if( matchMask(command.mask) ) {
command.handler(event); // or use call/apply if necessary
if(command.preventDefault) {
event.preventDefault();
}
break;
}
}
}(note that you'll have to figure out a way to pass the local var
box to the handlers, now that they're defined in a different scope)Aside: It might be a good use-case for the
Map object, if available - masks as keys, handlers as values?Alternatively, you could also make an
addKeyboardShortcutListener function that works similar to addEventListener, but accepts the key-combo mask as well as a handler.Basically, you can go more or less in-depth with this, but it'd make it (hopefully) easier to set up keyboard-handling, and (with some modification) use different key combos depending on platform.
For instance, on the Mac, cmd is used instead of ctrl, but checking
metaKey instead of ctrlKey isn't always enough. Something like "undo" is cmd+shift+Z by convention, and cmd + arrows is used for most navigation (though Home/End works too, and sometimes emacs-style combos too). Not that I'm demanding Mac support, but in general it might be nice to make the key-combo mapping more flexible.You've got a nice separation of model, ui, controller and so forth, but perhaps a bit more encapsulation within each of those would be nice, such as abstracting/encapsulating the key-combo matching.
Semi-related: A long time ago, I wrote something to handle keyboard shortcuts. Maybe you can use it for something. I'm linking it mostly because for some complex combinations, key events start to get weird. I don't think the code quite works right anymore, but the technique might still be viable.
Oh, and one thing I noticed is that making a 1-column vertical selection and drawing it, gave me something like:
═
║
═
which seems a little off. I expected it to only draw vertical pipe glyphs, or, if the top and bottom should be "flat", use the 3-way pipe glyphs for the ends:
║ ╦
║ or ║
║ ╩
Code Snippets
var canvasEvents = {
mouseover: function () { ... }
mousemove: function () { ... }
mousedown: function () { ... }
mouseup: function () { ... }
click: function () { ... }
};
for(var event in canvasEvents) {
canvas.addEventListener(event, canvasEvents[event]);
}// Not the complete list - just a sampling
// Order still matters of course, so ctrl+Home gets matches before Home
var keyCommands = [
{
mask: { which: BACKSPACE },
preventDefault: true,
handler: model.backspace
},
{
mask: { which: ARROW_UP },
handler: model.cursor.up
},
{
mask: { keyIdentifier: 'Home', ctrlKey: true },
handler: function () { model.cursor = new Cursor( 0, 0 ) }
},
{
mask: { keyIdentifier: 'Home' },
handler: function () { model.cursor.x ? model.cursor.x = 0 : model.cursor.y = 0 }
},
{
mask: { which: KEY_C, ctrlKey: true },
handler: function () { /*... etc ...*/ }
},
....
];
// ....
function handleKeyCombo(event) {
normalizeEvent(event);
function matchMask(mask) {
for( var property in mask ) {
if(event[property] != mask[property]) { // maybe use a strict comparison; your call
return false;
}
}
return true;
}
for( var i = 0, l = keyCommands.length ; i < l ; i++ ) {
var command = keyCommands[i];
if( matchMask(command.mask) ) {
command.handler(event); // or use call/apply if necessary
if(command.preventDefault) {
event.preventDefault();
}
break;
}
}
}Context
StackExchange Code Review Q#55375, answer score: 6
Revisions (0)
No revisions yet.