patternjavascriptModerate
A package for classes and Ajax
Viewed 0 times
ajaxpackageforclassesand
Problem
I've built a framework that primarily provides automated Ajax and a way to emulate classes. I've pasted the framework below and I hope it is well-commented.
I've also posted some application code so you have some idea of how it used.
I'm looking for general feedback on logic and organization.
I'm not looking to add any other features or increase the scope for now. If anything, I need to make it smaller.
Library
```
/*****
** FRAME
** manageGlobal - manages the single global variable that FRAME uses
** time - basic timer
** getDomElements - pull elements from the DOM
** getLibElements - creates library elements ( change name )
** initByProperty - execute class methods by property
** klass - emulates a class system
** machine - runs ajax on the classes among other things
** definePipe - defines the model data for the entire application
** makePipe - makes the model data for a single Ajax call
*****/
(function () {
"use strict";
var $A,
$P = {},
$R = {};
$P.last = {};
$R.Classes = {};
$R.packet_hold = {};
// requires utility and comms module
(function manageGlobal() {
if (window.$A && window.$A.pack && window.$A.pack.utility &&
window.$A.pack.comms) {
$A = window.$A;
$A.pack.frame = true;
} else {
throw "frame requires utility and comms module";
}
}());
// used to time performance by machine()
$R.time = (function () {
var measurements = [];
return function (control) {
var index,
intervals = [],
time_current = new Date().getTime();
if (control === 'start') {
measurements = [];
measurements.push(time_current);
klass() and machine() are the interesting methods.I've also posted some application code so you have some idea of how it used.
I'm looking for general feedback on logic and organization.
I'm not looking to add any other features or increase the scope for now. If anything, I need to make it smaller.
Library
```
/*****
** FRAME
** manageGlobal - manages the single global variable that FRAME uses
** time - basic timer
** getDomElements - pull elements from the DOM
** getLibElements - creates library elements ( change name )
** initByProperty - execute class methods by property
** klass - emulates a class system
** machine - runs ajax on the classes among other things
** definePipe - defines the model data for the entire application
** makePipe - makes the model data for a single Ajax call
*****/
(function () {
"use strict";
var $A,
$P = {},
$R = {};
$P.last = {};
$R.Classes = {};
$R.packet_hold = {};
// requires utility and comms module
(function manageGlobal() {
if (window.$A && window.$A.pack && window.$A.pack.utility &&
window.$A.pack.comms) {
$A = window.$A;
$A.pack.frame = true;
} else {
throw "frame requires utility and comms module";
}
}());
// used to time performance by machine()
$R.time = (function () {
var measurements = [];
return function (control) {
var index,
intervals = [],
time_current = new Date().getTime();
if (control === 'start') {
measurements = [];
measurements.push(time_current);
Solution
I'm polarized by this code.
I see many good practices in use here:
However, if you ask me to look at it at a more conceptual level, I would argue against using such code in production. There are several reasons for this.
Firstly, in my opinion this code violates the principle of single responsibility. It doesn't separate concerns quite well, mixing AJAX, some kind of a state machine, DOM interaction and a pseudo-OOP framework in one module.
It may just be me being silly, but I don't understand what this code does, after reading it fully for several times. The individual components seem deceptively simple but I don't understand how they work together.
The usage example you provided strikes me as rather cryptic too. It looks like a state machine (
Finally, I find naming to be hard to understand and confusing. The comments don't help because they seem to assume the reader already knows the system. This may not be the case for your team members, any new hires, or maybe even for yourself a few months down the road.
A few example that could benefit from better naming:
To sum it up, in my opinion this code is a bit over-engineered and suffers from premature abstraction. I don't think this abstraction will scale well as the website gets more complex.
For inspiration, I advise you to check out Backbone for nice ideas about models, views, and how they interact with each other while being separate.
Don't miss Backbone.Model.extend and how it properly sets up prototype chain, so
Stateful views are a really good idea—check out Facebook React if you're interesting in taking this idea further.
What you're trying to achieve with a custom framework is usually done with a combination of Backbone and jQuery, AngularJS and jQuery, React, or other frameworks. I'd argue the abstractions they provide are more generic and modular, and you might want to take a look at them and maybe adapt some ideas for your code.
Cheers!
An older version of this answer was overly harsh.
While I stay by my opinion that I wouldn't use this code in production, I tried to give more perspective for why it is so instead of just being mean.
I'm grateful to CR community for pointing out I was wrong.
I see many good practices in use here:
- you
use strict, consistently indent your code and apparently lint it too;
- you correctly use scoping to provide encapsulation, split code in modules and don't use globals;
- methods are small and clean, the code is easy to read.
However, if you ask me to look at it at a more conceptual level, I would argue against using such code in production. There are several reasons for this.
Firstly, in my opinion this code violates the principle of single responsibility. It doesn't separate concerns quite well, mixing AJAX, some kind of a state machine, DOM interaction and a pseudo-OOP framework in one module.
It may just be me being silly, but I don't understand what this code does, after reading it fully for several times. The individual components seem deceptively simple but I don't understand how they work together.
The usage example you provided strikes me as rather cryptic too. It looks like a state machine (
machine helps), but what are S, E, J, fm? Is there some asynchronous request being made (pre and post)? Is pipe some kind of shared state? When do machine's states change?Finally, I find naming to be hard to understand and confusing. The comments don't help because they seem to assume the reader already knows the system. This may not be the case for your team members, any new hires, or maybe even for yourself a few months down the road.
A few example that could benefit from better naming:
$R,$P,$A: private, public and... global? I've never seen “R” for “private” anywhere else.
someKey: I'd assume it returns true if some objects in an array have a specified key. Doesn't seem to be the case. Is this actually aforEach?
packet_hold: is it a packet that needs to be held for some time? What is a packet? Why does callingmakePipeset this variable?
getDomElements: this sounds very broad (I thought it's something likequerySelector) but in fact it does something very specific. It looks like it somehow binds “classes” defined through pseudo-OOP framework to DOM elements. So these classes are akin to MVC views? Shouldn't this code belong to a base view class?
getLibElements: Totally no idea what this does. The name is very vague. Even stranger, despite being calledget, it doesn't return anything.
initByProperty: Runs a method on each module? Is this something likeinvoke?
pipe_string_receive: Is this a boolean? A string?
makePipe: When I first saw the “pipes” thing, I thought you're referring to Unix-like or NodeJS-like pipes. Pipe is a quite specific concept in programming, so I'm curious why you're using the same word while apparently meaning something entirely different.
To sum it up, in my opinion this code is a bit over-engineered and suffers from premature abstraction. I don't think this abstraction will scale well as the website gets more complex.
For inspiration, I advise you to check out Backbone for nice ideas about models, views, and how they interact with each other while being separate.
Don't miss Backbone.Model.extend and how it properly sets up prototype chain, so
instanceof works, etc.Stateful views are a really good idea—check out Facebook React if you're interesting in taking this idea further.
What you're trying to achieve with a custom framework is usually done with a combination of Backbone and jQuery, AngularJS and jQuery, React, or other frameworks. I'd argue the abstractions they provide are more generic and modular, and you might want to take a look at them and maybe adapt some ideas for your code.
Cheers!
An older version of this answer was overly harsh.
While I stay by my opinion that I wouldn't use this code in production, I tried to give more perspective for why it is so instead of just being mean.
I'm grateful to CR community for pointing out I was wrong.
Context
StackExchange Code Review Q#38460, answer score: 14
Revisions (0)
No revisions yet.