Pairwise code review help

Pairwise code review help
0.0 0

#1

If you have a lot of time to spend, please look at this post and my code. It is not simple. But, I am hoping someone with experience would be kind enough to take the time to help.

I have enjoyed working on this project. But, I am a bit stumped about a small part of my code that didn’t work.

In my design and code I went through the given array and added the numbers. When I found a pair where the sum equaled the given number, I put the indexes of those numbers into a new array. I then continued going through the given array of numbers to find more pairs. During this continuation, I would check the numbers against the new array to make sure they weren’t already used. I used the “indexOf” method to do so. But, this didn’t work for the second to last test case.

I eventually found a different way to solve this issue. I replaced the numbers in the given array with “X” so they couldn’t be used again. While that worked with all the test cases and completed the project, it didn’t sit well with me. I think it is a valid solution. In some ways, I like it because it totally eliminates the possibility to reuse a number. But, it is a bit peculiar. And, I would like to find out why my use of “indexOf” didn’t work which would help me as a coder.

Here is a link to my JS that didn’t quite work. It has a couple of alert windows that will give you the final new array with all the indexes and the final sum. (The final sum code works. But, the data fed into it is not correct.)

You can see by comparing the numbers in first alert with the test case that there are indexes/numbers in the given array that are being used more than once, which results in an incorrect final set in the new array.

The problem was discovered while using the second to last test case.
I created some test cases of my own to try to figure out what was going on. You will see them in my code.

Note: The code sections that don’t work are within comments:
////////////////////////////// THIS CODE DOES NOT WORK ////////////////////////////////////
It should be pretty easy to find.
Obviously, there is code that interacts with this section. But, this is where there is a comparison made using the indexOf method which looks in the new array.

Additional Note: I use Notepad++ for my coding. So, what is in Code Pen is copied/pasted from my html file created in Notepad++.
.

Please let me know if I can provide any additional information. I think an experienced coder will be able to sort through my code. I have tried to make the code listing readable and have comments…maybe too many comments. I put a lot of my design in my code as comments.

Thanks.

Ken


#2

Hi there,

To be honest, I couldn’t make my way through this. Given enough time, i could, but it’d be a lot of work, so I don’t think you’ll get the code review you’re hoping for (and that I wish I could provide). However, there are a number of suggestions I can offer based on this code that will help you in the future:

  1. Don’t use alert() for debugging (or ever). Stick with console.log() for now and check out your browser’s dev console. You’ll find this much more usable!

  2. There are way too many comments. I totally appreciate what you’re trying to do by writing them, but if you feel you have to write this many, you should assume no one will read your code.

  3. Focus on writing readable code. Don’t worry so much about how fast or elegant or impressive it is, just write code that you can sit down and read without comments. If you’re writing comments next to variable declarations, that’s a good hint as to what you could name your variable. For instance:

    var ct00 = 0; // The index for the new array.

Could be

var newArrayIndex = 0;

If you’re writing comments above, in, or around your functions, that’s also a good sign your variables aren’t well named. When your variables have sensible names, your code can be read almost like a story.

This isn’t just for the benefit of other coders (and it’s often stated that you are the “other coder” when you return to your code 6 months later), but it will benefit you as you are writing your code. You will be able to reason about and organize your code better. You will write fewer bugs and be able to squash the remaining bugs faster. This is absolutely the best way you can improve right now, and you’ll see these benefits immediately.


#3

Thanks for giving it a shot.
And, thanks for the suggestions.

I am just starting to transition from using alerts to the console.log. There is a lot more to see in the Developer’s Console. I like Chrome’s the best.

The way I do a project is to write out a design/pseudo code and then paste it into an HTML document as comments. I code around the comments and delete them as I go. I didn’t get to delete them here. I also left them so people could see what was going on. But, there is a lot to read in this case, I agree. In the end I only leave comments when the code is doing something complex or unusual.

Variable names: Yes. I have not quite settled on a naming schema yet. I have read various blogs and chapters in books about them. I usually use something more descriptive than “ct” for something that is counting. And, I agree that “newArrayIndex” is better than something like “count”. It describes how this particular variable is associated with another part of the code.

My last employer had a naming schema spelled out which everyone followed. That “yn_stop” is a carryover from that era. I do need to decide on some kind of standards.

.


#5

Oh…Please…NO ONE look at this.
I made this WAAAY too complicated.
Had a bad week last week. Overthought this project…Yikes!
Maybe it can serve as an example as how NOT to do a challenge.

I am going to request that the initial post be deleted.

I do appreciate the general coding advice.

Hmm…P1xt…Do you think the genome thing will eventually be a FCC challenge?


#7

If you want me to delete the topic, I can do that. Learning is never a waste of time, though. What you posted isn’t bad in some moral sense, and it’s actually perfectly normal as far as how we progress as programmers. If we were to look at my earlier code - even from just a year ago - we’d see an opaque mess that no one would want to read.

Anyways, it’s not something you’ll be able to do overnight, so I hope you don’t get discouraged. Reading other people’s code can help, too, so take a look at what people submit on the forums and think about what could make it more readable, or why it’s already readable. Offer friendly suggestions to spread what you’ve learned. Github is also great for checking out code style. If you want, check out my respository. I’ve been refactoring my older code as I improve, which I think is a great way to master these concepts, but you’ll still be able to find some instances where I haven’t quite walked the walk.


#9

Thanks. I will look into these.

Well, here is my final version.

I did think about putting in something to eliminate processing values of j that were <= i in the loops.

Maybe I should have done something instead of using the value “X”, too.

If the test cases were the only ones being used with the code, then using <2 could be changed to ===0. But, I used <2 to cover test cases that only had one number in the given array. (Overthinking or being safe?)

function pairwise(arr, nbr){

function sumIndexes(indexTotal, nbrToAdd) {
    return indexTotal + nbrToAdd;
}

if(arr.length <2){
	return 0;
	}
else{ 

	var newArr = [];

	for(i=0; i<arr.length -1; i++){
		for(j=1; j<arr.length; j++){
			if(arr[i] != "X" && arr[j] != "X"){
				if(arr[i] + arr[j] === nbr){
					newArr.push(i);
					newArr.push(j);
					arr[i] =  "X";
					arr[j] =  "X";			
				}
			} 
		} 
	}

	var finalSum = newArr.reduce(sumIndexes);
	return finalSum;

	}

}

#11

My next project is the Tic Tac Toe game. I will probably post something here or Stack Exchange/Code Review for a review.

I did read up on alternatives to global variables. The one solution with object literals I found on the W3 Wiki page seemed similar to the Make a Person project.
https://www.w3.org/wiki/JavaScript_best_practices


#13

I attempted this using recursion. Once it gets to 3 layers of for loops, it gets too unwieldy imo:


#14

P1xt:

Here is a post I made to Code Review at Stack Exchange yesterday for the Tic Tac Toe game.

I found the W3 Wiki Best Practices very helpful.

There is a post/answer to my code already.