Adding a bookmark, or changing an existing one

I have a very problematic nested if/else/for that I'm sure it could use some love. Here's how it looks originally:

function addBookmark(obj) {
    var isFound = false;
    var posFound;
    if (bookmarks.length > 0) {
        for (var j = 0; j < bookmarks.length; j++) {
            if (obj.mirror == bookmarks[j].mirror &&
                obj.url == bookmarks[j].url &&
                obj.chapUrl == bookmarks[j].chapUrl &&
                obj.type == bookmarks[j].type) {
                if (obj.type == "chapter") {
                    isFound = true;
                    posFound = j;
                    break;
                } else {
                    if (obj.scanUrl == bookmarks[j].scanUrl) {
                        isFound = true;
                        posFound = j;
                        break;
                    }
                }
            }
        }
    }
    if (!isFound) {
        bookmarks[bookmarks.length] = {
            mirror : obj.mirror,
            url : obj.url,
            chapUrl : obj.chapUrl,
            type : obj.type,
            name : obj.name,
            chapName : obj.chapName,
            scanUrl : obj.scanUrl,
            scanName : obj.scanName,
            note : obj.note
        };
    } else {
        bookmarks[posFound].note = obj.note;
    }
    localStorage["bookmarks"] = JSON.stringify(bookmarks);
}

Is supposed to retrieve an object with bookmark information to be added/changed, loop through the array of bookmarks and check if the one we want to add exist/match, if exist then just add/change the note, if it doesn't then append.

I tried to remove chunks of it, trying to remove unnecessary nesting and this is what I achieved so far:

```
function addBookmark(obj) {
var isFound = false;
var posFound;
if (bookmarks.length > 0) {
for (var j = 0; j < bookmarks.length; j++) {
if (obj.mirror == bookmarks[j].mirror &&
obj.url == bookmarks[j].url &&
obj.

It would be more natural to call isFound simply found.

If I understand correctly,
after the loop, there is more code in your application.
In which case, the function does too much.
There should be at least 3 functions:

• A function with the loop, returning the position found, or else -1, as is pretty common with functions that search for something in a container

• A function that takes a position found, and does something with it (the code you didn't post)

• A function that calls the previous two

This way, you could get rid of the found and posFound state variables.
And in any case, no need to check bookmarks.length,
the iterating logic automatically takes care of the empty case.

The function then becomes:

function addBookmark(obj) {
    for (var j = 0; j < bookmarks.length; j++) {
        if (obj.mirror == bookmarks[j].mirror &&
            obj.url == bookmarks[j].url &&
            obj.chapUrl == bookmarks[j].chapUrl &&
            obj.type == bookmarks[j].type) {
            if (obj.type == "chapter" ||
                obj.scanUrl == bookmarks[j].scanUrl) {
                return j;
            }
        }
    }
    return -1;
}

For readability, I would also extract the complex conditions to a helper function with a descriptive name.
The name of the function would describe the meaning of those conditions.

StackExchange Code Review Q#91338, answer score: 6