Adding a bookmark, or changing an existing one

Question

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.chapUrl == bookmarks[j].chapUrl &&
 obj.type == bookmarks[j].type) {
 if (obj.type == "chapter" ||
 obj.scanUrl == bookmarks[j].scanUrl) {
 isFound = true;
 posFound = j;
 break;
 }
 }
 }
 }
// more runs here, specifically the posFound/isFound variables
}

I'm thinking on replacing the break with a return statement instead and abstract the whole ifelse. Should I? Or there's a way to simplify this further?

• Simon Forsberg: To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question
• Braiam: @SimonAndréForsberg is there a part that isn't straight-forward? It just 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. Is a self-documented piece of code which doesn't need much explanation (this part isn't even commented).
• Simon Forsberg: It's not about straight-forwardness, it's about being helpful to other reviewers, and also a sign that you are yourself aware what the code does.

Answer 1

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:

1. 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
2. A function that takes a position found, and does something with it (the code you didn't post)
3. 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.

• Braiam: That's exactly what I'm trying to do (the numbered list). I posted the rest of the function anyways if you want to have a look. Thanks anyways.

Answer 2

I personally prefer this flatter pattern, I also eliminated repeated array indexing:

function findBookmark(obj) {
 for (var j = 0; j < bookmarks.length; j++) {
 var bookmark = bookmarks[j];
 if (obj.type != "chapter" && obj.scanUrl != bookmark.scanUrl) continue;
 if (obj.mirror != bookmark.mirror) continue;
 if (obj.url != bookmark.url) continue;
 if (obj.chapUrl != bookmark.chapUrl) continue;
 if (obj.type != bookmark.type) continue;
 return bookmark;
 }
 return null;
}

Make sure to order the if's so the most common eliminations happen first.

Then your code would be:

var bookmark = findBookmark(obj);
if(bookmark) updateBookmark(bookmark, obj);
else addBookmark(obj);

To add a bookmark, depending on your implementation you could just do something like:

bookmarks.push( _.extend({}, obj) );

• Andreas Louv: The problem is that your findBookmark will return the index of the bookmark rater than the bookmark object itself, this will resolve in an if statement like so if ( -1 ) updateBookmark(-1, obj) which is properly not intended.