Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solution #2755

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

marcinzrodlowski
Copy link

No description provided.

Comment on lines 13 to 18
arrayFromString = arrayFromString.map(
line => line.replace('.2s', ' .2s '));

arrayFromString = arrayFromString.map(
line => line.replace('solid', ' solid '));
arrayFromString = arrayFromString.map(line => line.replace('!', ' !'));
Copy link

Choose a reason for hiding this comment

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

What if the styles have different values than .2s or solid? Will your code still work? It is not necessary to do it this way.

Btw. you are iterating through the array way more times than it's necessary. Try to find a simpler solution. I'm sure you are gonna solve with way less steps ;)

@marcinzrodlowski
Copy link
Author

That's my best shot, I hope it's enough 😁

Copy link

@mbruhler mbruhler left a comment

Choose a reason for hiding this comment

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

Hi,
This is a hardcore and very slow solution (regex'es are slow).

What I would do is:

  • replace newlines and split over ';'
  • for every part I would trim it, split it on ':' and destructure like [key, value] = trimmed
  • If i have key and value, just append it to the returning object.

Your solution won't be accepted using regexes because it is unnecessary slow.

@mbruhler
Copy link

Due to the fact that Marcin raised a complaint to my review I'm forced to revoke my assessment and add a new one with following comment:

I based my assessment on the fact that regexes have varying computational complexity and, depending on the syntax, they can have either linear or exponential (if we use backtracking). Your regexes just happen to have a linear one.
The for loop and map will increase the complexity because O(n) we have for each string and we have m strings - final O(m * n)
This complexity is not bad - in fact, my solution would have a similar one, because the split operation would have O(n) and I also wanted to iterate over the split string fragments.
So from this point I withdraw my review which says that the solution is slow - I wrongly assumed that all regexes are slow. This task is based on the length of the input anyway, and it won't get better even with other solutions. Regexes are slow when using backtracking, then they can even reach exponential complexity. The ones you used all have O(n) complexity.
Your solution, on the other hand, has another problem as I dug into it - it is non-generic. If the basis there is another CSS code, badly formatted it will crash / not format it correctly - and I guess this is the bigger problem as I see it. Here I request a correction to generalize this solution.
The conclusion of my analysis will be attached to the task review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants