Suggestions for improvement? (Not stuck/urgent)


#1

JavaScript -> Search Text for Your Name


I've tried to improve the program in this exercise to make it work with any word so that no matter the length or characters in the "myName" variable it'll be able to find it if it's within the "text" variable.

The code is at the bottom of the message.

I was just curious if anyone could see any more improvements/if I could do something better to my program as I've taken interest to the topic, especially after using features like this in web browsers and text editors.

I apologise if this is the wrong section to post this in as I'm unfamiliar with the website's forums


var text = "ppppookawiojwiojwegojretgpokwqfpoiqfdegojretgpokwqfpoiqfddoijggitqwiojwegojretgpokwqfpoiqfdoiOPKDWQOKFePQOLS";
var myName = "iojwegojretgpokwqfpoiqfd";
var hits = [];
var counter = 0;
var timesDetected = 0;

for(var i = 0; i < text.length; i++){ 
    if(text[i] === myName[0]){
        counter = 0;
        for(var j = i; j < i + myName.length; j++){
            if(text[j] === myName[counter]){
                counter++;
                if(text[j] === myName[myName.length - 1] && counter === myName.length){
                    timesDetected++;
                    counter = 0;
                    for(var k = i; k < i + myName.length; k++){
                        if(text[k] === myName[counter]){
                            hits.push(text[k]);
                            counter++;
                        }
                    }
                }
            }
        }
    }
}

if(hits.length === 0){
    console.log("Your name wasn't found");
}else{
    if(timesDetected === 1){
        console.log("The result was found 1 time");
    }else{
        console.log("The result was found " + timesDetected + " times");
    }
    console.log(hits);
}


#2

One optimization that can be done is that to observe that hits.length and myName.length in some way that gives the value for timesDetected directly :wink:


#3

If code runs off to the right (indentation) like that without really good reason, then it should probably be refactored.

You really only need two loops. One for the starting index of each substring and one for comparing the characters.

Keeping the matches in an array is a little pointless, the name is already known in myName. The indexes of where they are might be of value though.

var text = "ppppookawiojwiojwegojretgpokwqfpoiqfdegojretgpokwqfpoiqfddoijggitqwiojwegojretgpokwqfpoiqfdoiOPKDWQOKFePQOLS";
var myName = "iojwegojretgpokwqfpoiqfd";
hits = []
for (var i = 0; i < text.length; ++i) {
    for (var j = 0; j < myName.length; ++j) {
        if (text[i + j] !== myName[j]) {
            break;
        }
    }
    if (j == myName.length) {
        hits.push(i);
    }
}

console.log('number of hits: ' + hits.length)
console.log('hits found at indexes: ' + hits.join(', '))

Perhaps var j should be placed above the loop, I'm abusing that var doesn't make the variable local to the loop, it's local to the whole current function.
Alternatively, the inner loop could be replaced with string's substring method and compare to myName, the code would be a little nicer that way.. But it would be doing more operations .. But it might be doing them faster .. I really don't know which is faster, should probably just use substring because the code is nicer. I went with what uses less built-in functionality, but I'd rather write it this way:

var text = "ppppookawiojwiojwegojretgpokwqfpoiqfdegojretgpokwqfpoiqfddoijggitqwiojwegojretgpokwqfpoiqfdoiOPKDWQOKFePQOLS";
var myName = "iojwegojretgpokwqfpoiqfd";
hits = []
for (var i = 0; i < text.length; ++i) {
    if (text.substring(i, i + myName.length) === myName) {
        hits.push(i);
    }
}

console.log('number of hits: ' + hits.length)
console.log('hits found at indexes: ' + hits.join(', '))

If the purpose isn't to find each occurrence, then there are string methods like includes, indexOf, lastIndexOf
There's also the match method which finds each matching occurrence, but it uses regex so myName would need to be escaped first


#4

Thanks a lot for that! A lot of very useful information to work with. You're right about the array not being needed, I had that in there from an exercise as I adapted that off of the JavaScript course.

Cheers for showing me how much the program can be simplified, great reply :slight_smile:


#6

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.