Problem
I am working on a JavaScript exercise. I am running into issues with arrays.
NOTE: I posted this code in this forum because the code works. But, I would appreciate people reviewing it, and letting me know there are better ways to do this.
I am creating two arrays.
I am putting items into the first array. These are the separate parts of a string that represent a time: hours:minutes:seconds:milliseconds. You will see the times in the input string in my code.
I am putting those time segments into the first array, and then I am pushing that array into the second array (muti dimensional). I am doing this because I don’t know how many times will be in an input string. I am also doing this because I need to eventually add up these times in the input string, and then add another set of time segments from a second input string that I did not include here in this code. (I realize that I have to convert these to numbers eventually.)
I tried the code below without the commented out line:
// arrSubtitlesSeparate = [];
I don’t get what I expected.
When I include this line of code, I get the multi dimensional array I want.
Here is the results of the console logs. You will see them if you run the code.
You can see that the times are being found with the regex. But, the multi dimensional array is being filled with the last set of time segments that is found. You can easily see this just by looking at the last three digits which are the milliseconds in the time strings that are found.
found time: 00:13:37,243
arrSubtitlesSepMultiDim: 00,13,37,243
found time: 00:13:39,744
arrSubtitlesSepMultiDim: 00,13,39,744,00,13,39,744
found time: 00:13:39,779
arrSubtitlesSepMultiDim: 00,13,39,779,00,13,39,779,00,13,39,779
found time: 00:13:43,548
arrSubtitlesSepMultiDim: 00,13,43,548,00,13,43,548,00,13,43,548,00,13,43,548
If I add/uncomment that line of code, I get what I want.
The times are all added to the multi dimensional array.
My questions about coding this:
What is the proper way of creating the multidimensional array in this case?
Why is this happening when I don’t include that line of code?
Does this have something to do with the fact that the first set of time segment values found in the input string are the first set of values being put into the array?
Would I be better off initializing the sub array with the first set of values, and then using the splice method to replace the values?
Here is my code. You should be able to copy/paste into an HTML doc.
<script>
var subtitles = "188 00:13:37,243 --> 00:13:39,744 30 minutes. Everyone's ready. 189 00:13:39,779 --> 00:13:43,548 ♪ ";
var arrSubtitles = subtitles.split(" ");
console.log("arrSubtitles: " + arrSubtitles);
var arrSubtitlesSeparate = [];
var arrSubtitlesSepMultiDim = [];
for(var i = 0; i < arrSubtitles.length; i++){
if(/[0-9]{2}:[0-5][0-9]:[0-5][0-9],[0-9]{3}/.test(arrSubtitles[i]) === true){
console.log("found time: " + arrSubtitles[i]);
arrSubtitlesSeparate[0] = arrSubtitles[i].charAt(0) + arrSubtitles[i].charAt(1);
arrSubtitlesSeparate[1] = arrSubtitles[i].charAt(3) + arrSubtitles[i].charAt(4);
arrSubtitlesSeparate[2] = arrSubtitles[i].charAt(6) + arrSubtitles[i].charAt(7);
arrSubtitlesSeparate[3] = arrSubtitles[i].charAt(9) + arrSubtitles[i].charAt(10) + arrSubtitles[i].charAt(11);
arrSubtitlesSepMultiDim.push(arrSubtitlesSeparate);
// arrSubtitlesSeparate = [];
console.log("arrSubtitlesSepMultiDim: " + arrSubtitlesSepMultiDim);
}
}
</script>
Solution
First of all, when presenting a piece of code to fellow developers, you help them the most by letting them know what your code does and what the inputs and expected outputs are. You immediately start by describing how your code does something, which is a bit confusing.
Regarding your code, I suggest the following:
-
Both
arrSubtitles
andsubtitles
are arrays, but only one has the ‘arr’ prefix which is confusing. I find it more helpful to name variables according to their role instead of their type. You are splitting subtitles into some arbitrary whitespace delimited tokens, so how abouttokens
instead ofarrSubtitles
? -
arrSubtitlesSeparate
andarrSubtitlesSepMultiDim
are bulky, nondescript names. Following above advice, how abouttime
andtimes
? -
There is no need to perform a strict boolean comparison
=== true
within your if-clause, your code looks neater by simply omitting that. -
Within the loop body, you access
arrSubtitles[i]
10 times. To avoid those many lookups and improve readability, you could assign that array element to a temporary variable. Or you use afor ... of
loop instead, which iterates over array values instead of array indices. -
Replace successive
string.charAt
concatenations with a single call tostring.substr
. -
Leverage the block scope of
const
andlet
instead of using the function scopedvar
.
Applying those suggestions to your code would result in e.g.
const regex = /[0-9]{2}:[0-5][0-9]:[0-5][0-9],[0-9]{3}/;
const times = [];
for (const token of subtitles.split(" ")) {
if (regex.test(token)) {
const time = [
token.substr(0, 2),
token.substr(3, 2),
token.substr(6, 2),
token.substr(9, 3)
];
times.push(time);
}
}
console.log(times);
You asked what happens when you don’t initialize arrSubtitlesSeparate = []
in the loop body?
Initially you create a single array instance arrSubtitlesSeparate
. Then, during each loop iteration, you modify the content of this array and push it onto arrSubtitlesSepMultiDim
. So in the end, each index of arrSubtitlesSepMultiDim
contains a reference to the same arrSubtitlesSeparate
array, similar to this:
arrSubtitlesSepMultiDim = [arrSubtitlesSeparate, arrSubtitlesSeparate, ...];
See also Is JavaScript a pass-by-reference or pass-by-value language?
Now, if we look again at your implementation, we note that you already perform a regex test. We could perform a regex matching instead and get rid of all those redundant string.substr
calls. I wrapped this new implementation in a documented function and ended up with:
/**
* Find and parse all timestamps of type "hours:minutes:seconds,milliseconds"
* within a given subtitles string.
* @param {string} subtitles
* @return {Array} Array of times [hours, minutes, seconds, milliseconds]
*/
function findTimes(subtitles) {
const regex = /([0-9]{2}):([0-5][0-9]):([0-5][0-9]),([0-9]{3})/g;
const times = [];
let matches;
while (matches = regex.exec(subtitles)) {
times.push(matches.slice(1));
}
return times;
}