Skip to content

Improve between implementation#3

Open
tshemsedinov wants to merge 1 commit into
masterfrom
refactor-between
Open

Improve between implementation#3
tshemsedinov wants to merge 1 commit into
masterfrom
refactor-between

Conversation

@tshemsedinov

Copy link
Copy Markdown
Member

No description provided.

@tshemsedinov tshemsedinov added the enhancement New feature or request label Oct 22, 2019
@tshemsedinov tshemsedinov requested a review from nechaido October 22, 2019 14:45
@tshemsedinov tshemsedinov self-assigned this Oct 22, 2019
Comment thread JavaScript/2-between.js
i = line.indexOf(end);
if (i === -1) return '';
str = str.substring(0, i);
return line.substring(0, i);

@nechaido nechaido Oct 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to implement this function in a way, that we only create a substring once in each case.
Current implementation extracts substring twice if we provide end quote.

Comment thread JavaScript/2-between.js
@@ -3,13 +3,15 @@
// Implementation

const between = (str, quotes) => {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we can make in more flexible

Suggested change
const between = (str, quotes) => {
const between = (str, start, end) => {

Comment thread JavaScript/2-between.js
if (quotes[1]) {
i = str.indexOf(quotes[1]);
const line = str.substring(i + 1);
if (end) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expect than end argument always present, so if is not needed

@HowProgrammingWorks HowProgrammingWorks locked and limited conversation to collaborators Oct 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants