Ex1 #2

zyahav · 2016-10-15T22:52:19Z

This is my first exercise.

myok12 · 2016-10-15T23:08:13Z

ex1/ex1.js

+
+/////////////////////////////////////////////////////////////////////////////
+// getThirdCommaPosition accepts a string and returns the index of the third comma in that string /  no comma return -1
+function getThirdCommaPosition(str,searcVal) {


searchVal was not requested in the question, but fine.
BTW - searcVal --> searchVal (with h).

Still not fixed

myok12 · 2016-10-15T23:08:53Z

ex1/ex1.js

+
+/////////////////////////////////////////////////////////////////////////////
+// getThirdCommaPosition accepts a string and returns the index of the third comma in that string /  no comma return -1
+function getThirdCommaPosition(str,searcVal) {


Position is fine, but in JavaScript we normally use the word index (like in indexOf)

Done (next time you add this comment so that I know this is fine now).

myok12 · 2016-10-15T23:09:40Z

ex1/ex1.js

+// getThirdCommaPosition accepts a string and returns the index of the third comma in that string /  no comma return -1
+function getThirdCommaPosition(str,searcVal) {
+
+	var i, position = -1;


The -1 assignment is not needed, but it's fine

need more info

myok12 · 2016-10-15T23:10:26Z

ex1/ex1.js

+
+	var i, position = -1;
+
+	for (i = 0; i < 3; i++) {


if you are writing in ES6, I would move the var statement into the for loop and change it to a let, as in for (let i = 0, ....

position = -1; בגלל שצריך להתחיל מ 0
position = str.indexOf(searcVal, position+1);

myok12 · 2016-10-15T23:12:06Z

ex1/ex1.js

+//var str = ',,,';// === 2
+var str = 'hel,lowo,rld';// === -1
+
+console.log(getThirdCommaPosition(str, ','));


Please make a function that tests using console log, e.g.:

function testFunction(str) {
console.log(getThirdCommaPosition(str));
}

and call it as many times as your examples

replace val to searchFor add test function

myok12 · 2016-11-07T21:41:19Z

ex1/ex1.js

+
+/////////////////////////////////////////////////////////////////////////////
+// getThirdCommaIndex accepts a string and returns the index of the third comma in that string if no comma return -1
+function getThirdCommaIndex(str,searcFor) {


myok12 · 2016-11-07T21:41:34Z

ex1/ex1.js

+// getThirdCommaIndex accepts a string and returns the index of the third comma in that string if no comma return -1
+function getThirdCommaIndex(str,searcFor) {
+
+        var index = -1;


The initial value of -1 isn't being used

myok12 · 2016-11-07T21:44:14Z

ex1/ex1.js

+
+function testFunction(str) {
+
+	console.log(getThirdCommaIndex(str, ','));


Please switch to console.assert.
Then, read https://jasmine.github.io/ and switch to a real test framework (after which, running npm test should test your code and report if anything is wrong. Good luck

myok12 · 2016-11-15T18:10:14Z

ex1/ex1.js

-function getThirdCommaIndex(str,searcFor) {
+function getThirdCommaIndex(str,searchFor) {

        var index = -1;


Please put a comment about why the value -1 is important.

myok12 · 2016-11-15T18:12:40Z

ex1/ex1Spec.js

+describe("getThirdCommaIndex", function() {
+
+
+	it("hel,lowo,rld", function() {


The it name should reflect what you are testing for. It should be some sentence, such as: "it returns -1 when no third comma", and should be then written as it("returns -1 when no third comma", function() {

myok12 · 2016-11-15T18:13:50Z

ex1/ex1Spec.js

+		expect(getThirdCommaIndex('hel,lowo,rld', ',')).toBe(-1);
+	});
+
+	it("hello,world,this,is,a,great,day", function() {


it("finds a comma in a regular sentence", function() {

myok12 · 2016-11-15T18:14:12Z

ex1/ex1Spec.js

+		expect(getThirdCommaIndex('hello,world,this,is,a,great,day', ',')).toBe(16);
+	});
+
+	it(",,,", function() {


it("works even when there are no characters beside commas", function() {

myok12 · 2016-11-15T19:50:12Z

ex1/ex1Spec.js

@@ -0,0 +1,46 @@
+/*


Did you not get the require working (or maybe you just did it for another question) and instead you have to copy the function?

myok12 · 2016-11-22T18:34:55Z

ex1/ex1.js


+function getThirdCommaIndex(str,searchFor) {
+
+	//  i do not know how to explain the use of  -1


This should have been in a github comment and not in code. (You don't really want this to remain in code for the next time you'll go read this, right?) Please delete.

myok12 · 2016-11-22T18:36:47Z

ex1/ex1.js

-testFunction('hel,lowo,rld');// === -1
-testFunction('hello,world,this,is,a,great,day');// === 16
-testFunction(',,,');// === 2
+module.exports = { getThirdCommaIndex }


This is equivalent to:
module.exports = { getThirdCommaIndex: getThirdCommaIndex }
This exports an object with one property named getThirdCommaIndex.

Since you don't need to export anything else, you can just export the function directly:
module.exports = getThirdCommaIndex
The test file will have to change too, obviously.

myok12 · 2016-11-22T18:38:03Z

ex1/ex1Spec.js

-	it("hello,world,this,is,a,great,day", function() {
-		expect(getThirdCommaIndex('hello,world,this,is,a,great,day', ',')).toBe(16);
+	it("finds a comma in a regular sentenc", function() {
+		expect(ex1.getThirdCommaIndex('hello,world,this,is,a,great,day', ',')).toBe(16);


Try to break lines in higher-level constructs:

expect(ex1.getThirdCommaIndex('hello,world,this,is,a,great,day', ','))
.toBe(16);
It makes it easier to read (the pause of the line break comes in a better place).

myok12 · 2016-12-01T17:48:49Z

ex1/ex1Spec.js

+	});
+
+	it("finds a comma in a regular sentence", function() {
+		expect(getThirdCommaIndex('hello,world,this,is,a,great,day', ','))


The maximum line length on github.com is 80 character, so this line gets too long. You could break this into two lines, but I think it might be easier to use a shorter tab (maybe two spaces instead of an entire tab? See https://www.google.com/search?sourceid=chrome-psyapi2&ion=1&espv=2&ie=UTF-8&q=vim%20tabs%20to%20spaces&oq=vim%20tab&aqs=chrome.1.69i57j0l5.3624j0j7

myok12 · 2016-12-01T17:49:27Z

ex1/ex1Spec.js

+
+	it("works even when there are no characters beside commas", function() {
+		expect(getThirdCommaIndex(',,,', ','))
+		.toBe(2);


When you do break a line because it is long, you need to push the continuation line in: +tab.

updating from ohad

Merge pull request #2 from myok12/master

Manually copied old function

037b865

myok12 requested changes Oct 15, 2016

View reviewed changes

zyahav added 2 commits October 18, 2016 14:02

replace function name from pointer to index

5ef068f

replace val to searchFor add test function

Fixed all initial comments from Ohad

f347958

myok12 reviewed Nov 7, 2016

View reviewed changes

zyahav added 2 commits November 15, 2016 14:07

1

3d9da97

add jasmine test

dad124b

myok12 reviewed Nov 15, 2016

View reviewed changes

zyahav added 2 commits November 22, 2016 07:59

redid test using require

f3a43dd

redid ex1.ja using requirde

4a472c1

myok12 reviewed Nov 22, 2016

View reviewed changes

myok12 changed the title ~~exercise 1~~ Ex1 Nov 24, 2016

zyahav added 2 commits December 1, 2016 12:53

typo and break lines

1ac33f0

test breack lines

90d5d26

myok12 approved these changes Dec 1, 2016

View reviewed changes

add function to test

e6095af

myok12 pushed a commit that referenced this pull request Dec 23, 2016

Merge pull request #2 from myok12/master

85316b4

updating from ohad

myok12 pushed a commit that referenced this pull request Dec 23, 2016

Merge pull request #15 from zyahav/master

546ee68

Merge pull request #2 from myok12/master

myok12 mentioned this pull request Jan 5, 2017

employee filter.asc.desc.load #18

Open


		function testFunction(str) {

		console.log(getThirdCommaIndex(str, ','));

		describe("getThirdCommaIndex", function() {


		it("hel,lowo,rld", function() {


		function getThirdCommaIndex(str,searchFor) {

		// i do not know how to explain the use of -1

Ex1 #2

Are you sure you want to change the base?

Ex1 #2

Uh oh!

Conversation

zyahav commented Oct 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone