Andrew Thompson wrote:
I have written a few scripts to parse the
URL arguments and either list them or allow
access to the value of any parameter by name.
<snip> Before I go offering it in public (and writing it
into any number of the 'development kits' I deliver
through my site), I would like to get it reviewed
by the people who know Javascript.
Is this code as good as it can be?
<snip>
No, it is pedestrian at best.
Starting with your - QueryParameter - object:-
| /** Provide the name of this URL parameter. */
| function getName() {
| return this.sName;
| }
|
| /** Provide the value of this URL parameter. */
| function getValue() {
| return this.sValue;
| }
|
| /** Check if this is the 'paramName' URL parameter. */
| function isParam( paramName ) {
| return ( this.sName==paramName );
| }
|
| /** Construct an URL parameter object, and attach the functions. */
| function QueryParameter( paramName, paramValue ) {
| this.sName = paramName;
| this.sValue = paramValue;
|
| this.getName = getName;
| this.getValue = getValue;
| this.isParam = isParam;
| }
- you are creating 4 global function to support one object. Generally,
code should have as little impact on the global namespace as possible.
For a globally available object constructor that impact does not need to
be more than just the constructor name. The more items added to the
global namespace the more there is a risk of naming collisions and
resulting unexpected interactions.
This is of most significance when creating generalised components (as
appears to be your intention) because the users of a generalised
component should not be expected to be intimate with its implementation
and so should not need to concern themselves with avoiding naming
collisions. You have chosen to create global functions called "getName",
"getValue" and "isParam", which are nice self-explanatory names to use,
but equally they are names that are likely to be well suited elsewhere.
When the those names are just the properties of a type of object then it
doesn't matter if another type of object has the same property names,
but habitually place them in the global namespace and you are asking for
trouble (and individuals who can only employ other people's components
are going to be the least capable of handling any manifest
consequences).
But javascript constructors to have a prototype which allows objets
created with the constructor to inherit common properties, and also
allows those properties to be define without any impact on the global
namespace. Using the prototype pattern to define your object would
result in:-
function QueryParameter(paramName, paramValue){
this.sName = paramName;
this.sValue = paramValue;
}
QueryParameter.prototype.getName = function(){
return this.sName;
};
QueryParameter.prototype.getValue = function(){
return this.sValue;
};
QueryParameter.prototype.isParam = function(paramName){
return (this.sName==paramName);
};
With which all objects created with the constructor automatically
acquire all of the methods defined on the prototype without any need to
explicitly assign function object to object properties, and without the
creation of the methods needing to have any impact on the global
namespace.
But this namespace problem caries over in to the rest of the system,
which defines another 3 functions and 3 global variables, 2 of which are
only temporarily used and so would seem to be performing roles that
would be better suited to function local variables. The configuration
also uses two inline function calls, neither will work without the
other, and the functions being called are unlikely to be applicable in
other situations (especially as they employ global variables and have no
parameters).
I notice that, on the web page, you say; "I considered adding the
objects to a hash (associative array) as suggested by Grant , but
unfortunately could not then figure how to iterate the elements. For me
exploiting the hashtable-like aspects of javascript objects is the
obvious way to store name/value pairs. Doing so allows named property
references to be handled by native code instead of javascript, and
particularly javascript loops.
You use:-
| /** Provides the value for the parameter named by 'key'. */
| function getQueryString(key) {
| var value = null;
| for (var ii=0; ii<parameters.length; ii++) {
| if ( parameters[ii].isParam(key) ) {
| value = parameters[ii].getValue();
| break;
| }
| }
| return value;
| }
- which loops through (potentially) all of the created objects, and
makes two method calls to determine which value to return, and still
will only return the value corresponding with the first name/value pair
encountered on the query string (a general method should be able to
handle multiple occurrences of the same name in the query string.
Using javascript object to store name/value pairs allows much more
efficient retrieval of named properties. Being able to iterate through
the names used is just a matter of recording the names used in a
retrievable form.
The drawback with using a javascript objects to store name/value pairs
is that they have some properties of their own (constructor, toString)
which need some special consideration when the intention is to create
truly general code. But then creating truly general code is not
necessarily a good idea as all code will actually be used in a specific
context, and downloading code to handle possibilities that will never
arise in the specific situation is wasteful. For example a general query
string interface might go:-
/* This function call creates an interface to the name value pairs
from the query string associated with the URL of the current page
in the browsers. The interface is available as a global variable
called - queryStrings - and has two methods:-
queryStrings.getCountFor(name); - Returns the number of values found
in the query string under the - name - provided as a parameter,
or numeric zero if no elements exist under the name.
queryStrings.getValueFor(name, index); - Returns the value found on
the query string associated with the - name - provided as the
first parameter. The second parameter is optional and represents
an index into an array of the values found for the name. The
index defaults to zero if no value is provided. If no value is
found under the index, or no values exist under the name, then
undefined is returned. Otherwise a string value is returned for
each value provided on the query string associated with the
name, or, if a name was found on the query string with no
associated value this method returns boolean true;
queryStrings.getNames(); - Returns a reference to an Array of all
of the names found on the query string. Any modifications made
to the array will persist and be present in the Array that will
be returned by later calls, but will alter the name value pairs
returned by the other methods.
*/
var queryStrings = (function(out){
var list = {};
var keyList = [];
var global = this;
var unsc = (global.decodeURIComponent)?'decodeURIComponent':
'unescape';
var nameSafe = [' ','']; //first element is the space character
function addItem(name, value){
nameSafe[1] = name;
var sName = nameSafe.join('');
if(list[sName]){
list[sName][out[sName].length] = value;
}else{
list[sName] = [value];
keyList[keyList.length] = name;
}
}
if(typeof location != 'undefined'){
var nvp,ofSet, temp = location.search||location.href||'';
if((ofSet = temp.indexOf('?')) > -1){
temp = temp.split("#")[0];
temp = temp.substring((ofSet+1), temp.length);
var workAr = temp.split('&');
for(var c = workAr.length;c--;){
nvp = workAr[c].split('=');
if(nvp.length > 1){
addItem(global[unsc](nvp[0]),global[unsc](nvp[1]));
}else if(nvp.length == 1){
addItem(global[unsc](nvp[0]), true);
}
}
}
}
return ({
getCountFor:function(name){
nameSafe[1] = name;
var sName = nameSafe.join('');
return ((list[sName] && list[sName].length)|0);
},
getValueFor:function(name, index){
nameSafe[1] = name;
var sName = nameSafe.join('');
if(list[sName]){
return list[sName][(index|0)];
} //else return undefined (by default)
},
getNames:function(){
return keyList;
}
});
})(); //inline function expression call.
- and cover most possibilities in query string use. But in a real
situation it would be possible to avoid names in the query string that
would coincide with javascript object properties, know that name/value
pairs would be distinct, know that names would always have accompanying
values, know which values where expected to be used and avoid names and
values that would be modified in URL encoding.
Knowing these things allows much simpler code to be used, but to still
achieve the desired results. e.g.:-
var queryStrings = (function(out){
if(typeof location != 'undefined'){
var temp = location.search||location.href||'';
var nvp, ofSet;
if((ofSet = temp.indexOf('?')) > -1){
temp = temp.split("#")[0];
temp = temp.substring((ofSet+1), temp.length);
var workAr = temp.split('&');
for(var c = workAr.length;c--;){
nvp = workAr[c].split('=');
if(nvp.length > 1){
out[nvp[0]] = nvp[1];
}
}
}
}
return out;
}({})); //inline function expression call passed an object
// literal - {} - as its parameter (out).
I notice your page includes the line; "It is designed to degrade
'gracefully' by telling the user they require JS for this page.". That
is not a definition of "graceful degradation" that I would recognise (it
is never 'graceful' to be bothering the user with this sort of detail).
Though it is not really the role of such a low-level component to
gracefully degrade itself. That is a task for the code that wishes to
use it; to decide how it is going to respond to not being able to
extract the query sting information that it requires/expects. The
responsibility for the component designer is only to ensure that the
code does not actually error when it fails. and to have well defined
behaviour when that failure occurs, for which it is probably only
necessary, in this context, not to return values when they are requested
by name.
Richard.
