patternjavascriptMinor
printf-style string formatter
Viewed 0 times
formatterstyleprintfstring
Problem
As part of a larger project I'm working on, I've made a function that aims to replicate, as best as I can, the placeholder part of the
It replaces
My main questions are:
```
function replacePlaceholders(string) {
var args = Array.prototype.slice.call(arguments).splice(1); //get all the extra arguments (exclude the original string)
var counter = 0; //set a counter
for (var i = 0; i < args.length; i++) { //loop through the extra arguments (should match the number of placeholders in the string)
var match = /%s|%d|%i|%f|%o/.exec(string); //regex
if (match) { //if match found
var index = match.index; //store the index
switch (match[0]) { //check whether the placeholder has a real value with the correct type
case '%s': //for strings
string = (typeof args[counter] == 'string') ? string.substr(0, index) + args[counter] + string.substr(index + 2) : string.substr(0, index) + NaN + string.substr(index + 2);
break;
case '%d': //for numbers....
case '%i':
case '%f':
string = (typeof args[counter] == 'number') ? string.substr(0, index) + args[counter] + string.substr(index + 2) : string.substr(0, index) + NaN + string.substr(index + 2);
break;
case '%o': //for objects
string = (typeof args[counter] == 'object') ? string.substr(0,
console.log() function usable in most browsers.It replaces
%s, %d , %i , %f and ,%o. It works by having 'unlimited' parameters (the replacements), looping through the parameters, checking for any placeholders and replacing them by index with the parameter given. The switch is for checking whether the passed parameter (replacement) is the correct type - if not, it will add NaN, just like the real console would.My main questions are:
- Is my code readable?
- Are there any better ways to do any part of this?
- Can it be cleaned up anymore?
```
function replacePlaceholders(string) {
var args = Array.prototype.slice.call(arguments).splice(1); //get all the extra arguments (exclude the original string)
var counter = 0; //set a counter
for (var i = 0; i < args.length; i++) { //loop through the extra arguments (should match the number of placeholders in the string)
var match = /%s|%d|%i|%f|%o/.exec(string); //regex
if (match) { //if match found
var index = match.index; //store the index
switch (match[0]) { //check whether the placeholder has a real value with the correct type
case '%s': //for strings
string = (typeof args[counter] == 'string') ? string.substr(0, index) + args[counter] + string.substr(index + 2) : string.substr(0, index) + NaN + string.substr(index + 2);
break;
case '%d': //for numbers....
case '%i':
case '%f':
string = (typeof args[counter] == 'number') ? string.substr(0, index) + args[counter] + string.substr(index + 2) : string.substr(0, index) + NaN + string.substr(index + 2);
break;
case '%o': //for objects
string = (typeof args[counter] == 'object') ? string.substr(0,
Solution
Before I start: Thank you so much for making this code easily readable! Thank you from the bottom of my heart!
Second thing that I have noticed was the name. It's wrong! If it is a
You have an argument called
You can do this:
Done! But it won't be needed and you will see why.
You still didn't learn: Store the length in a local variable. Always! Like this:
Accessing a property in an object (in this case, an array) is slower than accessing a loval variable. By setting the local variable with that slow property, we increase performance.
It is really a great idea to have literal regular expressions, but the whole code is quite... shady... Specially the useless validations!
You expect a string to be a real string so you can do something... But why? What about a number? What about an array? What around something else? Why can't I convert anything into a string? Every single object has the
And why that
The
And now, the questions:
I would never do it on this way. It's so frail and picky. It's so hard to change anything without breaking it. You have to deal with things you should, like breaking a string in 3 parts and replacing the middle part with something. That's a bad way to do a
I would do like this:
Come on! It's so easy to understand! You can change it's functionality easily!
If you want, you can remove that loop to fetch the keys and use this literal regular expression:
And this is a "good enough" clone of the
Outside the scope of this review, thanks to you, I finally have a function to write my polyglot questions! Also, you may want to look at http://phpjs.org/functions/sprintf/ if you want a perfect clone of the
To reflect closely the desired implementation, one can change the
```
function sprintf(){
var map = {
s: function(value){
return value.toString();
},
d: function(value){
return (parseInt(value, 10) || 0).toString();
},
i: function(value){
return this.d(value);
}
f: function(value){
return (parseFloat(value, 10) || '0.0').toString();
Second thing that I have noticed was the name. It's wrong! If it is a
sptrinf-like function, give it the right name. sprintf! Done! No overly complicated boring names.You have an argument called
string. Please, don't do that. In fact, remove it's name! You know it is the first element.You can do this:
var text = arguments[0];
for (var i = 1; i < arguments.length; i++) {Done! But it won't be needed and you will see why.
You still didn't learn: Store the length in a local variable. Always! Like this:
for (var i = 1, length = args.length; i < length; i++) {Accessing a property in an object (in this case, an array) is slower than accessing a loval variable. By setting the local variable with that slow property, we increase performance.
It is really a great idea to have literal regular expressions, but the whole code is quite... shady... Specially the useless validations!
You expect a string to be a real string so you can do something... But why? What about a number? What about an array? What around something else? Why can't I convert anything into a string? Every single object has the
.toString() method.And why that
NaN there? What is it for? Is that to say it is NaN?The
%o is doing something somewhat somewhere that I don't get. By the C++ documentation and the PHP documentation specify that this is the octal representation.And now, the questions:
- Why do you check if an argument is a
stringbefore converting it to string?
- Why do you check if an argument is og any type before converting?
- why don't you do any kind of validation?
- Why are you using
exec?
I would never do it on this way. It's so frail and picky. It's so hard to change anything without breaking it. You have to deal with things you should, like breaking a string in 3 parts and replacing the middle part with something. That's a bad way to do a
.replace().I would do like this:
function sprintf(){
var toBase = function(value, base){
//converts to the required bases
return (parseInt(value, 10) || 0).toString(base);
};
var map = {
s: function(value){
return value.toString();
},
d: function(value){
return toBase(value, 10);
},
i: function(value){
return this.d(value);
},
b: function(value){
//binary conversion
return toBase(value, 2);
},
o: function(value){
//octal conversion
return toBase(value, 8);
},
x: function(value){
//hexadecimal conversion
return toBase(value, 16);
},
X: function(value){
//HEXADECIMAL CONVERSION IN CAPITALS
return toBase(value, 16).toUpperCase();
},
e: function(value){
//scientific notation
return (parseFloat(value, 10) || 0).toExponential();
},
E: function(value){
return this.e(value, 10).toUpperCase();
},
f: function(value){
//floating point
return (parseFloat(value, 10) || '0.0').toString();
}
};
//crude way to extract the keys
var keys = '';
for(var k in map)
{
keys += k;
}
var args = Array.prototype.slice.call(arguments).slice();
return args.shift().toString().replace(new RegExp('%([' + keys + '])','g'),function(_, type){
if(!args.length)
{
throw new Error('Too few elements');//appropriate type here?
}
return map[type](args.shift());
});
}Come on! It's so easy to understand! You can change it's functionality easily!
If you want, you can remove that loop to fetch the keys and use this literal regular expression:
/%([diobsxXeEf])/g. Same thing, but faster and less flexible.And this is a "good enough" clone of the
sprintf function. It doesn't support padding, float-number size specifitation and others. But, it works for the bare minimum.Outside the scope of this review, thanks to you, I finally have a function to write my polyglot questions! Also, you may want to look at http://phpjs.org/functions/sprintf/ if you want a perfect clone of the
sprintf function.To reflect closely the desired implementation, one can change the
map variable. I went a bit further and changed all unnecessary things. To be consistent with the MDN documentation and the Chrome console API documentation, you can do this:```
function sprintf(){
var map = {
s: function(value){
return value.toString();
},
d: function(value){
return (parseInt(value, 10) || 0).toString();
},
i: function(value){
return this.d(value);
}
f: function(value){
return (parseFloat(value, 10) || '0.0').toString();
Code Snippets
var text = arguments[0];
for (var i = 1; i < arguments.length; i++) {for (var i = 1, length = args.length; i < length; i++) {function sprintf(){
var toBase = function(value, base){
//converts to the required bases
return (parseInt(value, 10) || 0).toString(base);
};
var map = {
s: function(value){
return value.toString();
},
d: function(value){
return toBase(value, 10);
},
i: function(value){
return this.d(value);
},
b: function(value){
//binary conversion
return toBase(value, 2);
},
o: function(value){
//octal conversion
return toBase(value, 8);
},
x: function(value){
//hexadecimal conversion
return toBase(value, 16);
},
X: function(value){
//HEXADECIMAL CONVERSION IN CAPITALS
return toBase(value, 16).toUpperCase();
},
e: function(value){
//scientific notation
return (parseFloat(value, 10) || 0).toExponential();
},
E: function(value){
return this.e(value, 10).toUpperCase();
},
f: function(value){
//floating point
return (parseFloat(value, 10) || '0.0').toString();
}
};
//crude way to extract the keys
var keys = '';
for(var k in map)
{
keys += k;
}
var args = Array.prototype.slice.call(arguments).slice();
return args.shift().toString().replace(new RegExp('%([' + keys + '])','g'),function(_, type){
if(!args.length)
{
throw new Error('Too few elements');//appropriate type here?
}
return map[type](args.shift());
});
}function sprintf(){
var map = {
s: function(value){
return value.toString();
},
d: function(value){
return (parseInt(value, 10) || 0).toString();
},
i: function(value){
return this.d(value);
}
f: function(value){
return (parseFloat(value, 10) || '0.0').toString();
}
};
var args = Array.prototype.slice.call(arguments).slice();
return args.shift().toString().replace(/%([difs])/g,function(_, type){
if(!args.length)
{
throw new Error('Too few elements');
}
return map[type](args.shift());
});
}Context
StackExchange Code Review Q#101405, answer score: 3
Revisions (0)
No revisions yet.