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

Code concerning Issue #38 #39

Merged
merged 7 commits into from
Feb 9, 2016
Merged

Code concerning Issue #38 #39

merged 7 commits into from
Feb 9, 2016

Conversation

hashhar
Copy link
Contributor

@hashhar hashhar commented Oct 29, 2015

Improve compression by appending count only if count exceeds 2 ( #38 )

Changed Files:

  • arrays_strings/compress/compress_challenge.ipynb
  • arrays_strings/compress/compress_solution.ipynb
  • arrays_strings/compress/test_compress.py

Chagenlog:

  • The solution notebook and challenge notebook have updates titles and test cases.
  • The solution notebook has changes made to the compress_string function with some helpful comments for explaining the changes

Even in case it is not pulled, do comment on how I did and what I could have done to do it better.

@hashhar
Copy link
Contributor Author

hashhar commented Oct 29, 2015

Sorry for the three commits, it could have been done in one. (But I'm a commit hungry and streak hungry person 😛 )

@muatik
Copy link

muatik commented Oct 29, 2015

the time and space complexity of your implementation are O(n) and O(n), as you specified?

@hashhar
Copy link
Contributor Author

hashhar commented Oct 29, 2015

No, I haven't checked those. Let me check. Space complexity is O(n) as I haven't used any other variable or storage space. But time complexity may be affected by the nested if's I have added.

@donnemartin
Copy link
Owner

@hashhar thanks for the pull request! I'll try to review this in the next few days.

@muatik appreciate the review 👍

@donnemartin
Copy link
Owner

@hashhar just a quick update...I haven't forgotten about this, I hope to get to it by the weekend.

@hashhar
Copy link
Contributor Author

hashhar commented Nov 11, 2015

Thanks to let me know. Take your time. I have exams coming up anyway.

@donnemartin
Copy link
Owner

@hashhar thanks again for the PR, here are some thoughts:

The requirement change of "Improve compression by appending count only if count exceeds 2" makes a lot of sense, although it does make the solution a bit more complex. Perhaps we should make it a 'follow-up' type question and add another solution as opposed to modifying the existing solution?

Problem: Compress a string such that 'AAABCCDDDD' becomes 'A3B1C2D4'. Only compress the string if it saves space. What if you don't want to display 'B1' and instead want the output to be 'A3BC2D4'?

Here's a notebook with multiple solutions as a template:

http://nbviewer.ipython.org/github/donnemartin/interactive-coding-challenges/blob/master/arrays_strings/unique_chars/unique_chars_solution.ipynb

Additional comments:

  • The use of tabs instead of spaces is making it hard for me to diff the solutions. Can you use spaces instead? Please check out the style guide.
  • I wonder if there's a way to reduce the number of times compressed_string.append is called from 6x.
  • There's a FIXME comment left in the submission.

@hashhar
Copy link
Contributor Author

hashhar commented Dec 2, 2015

Thanks for the feedback. I'll get back onto it.
It may take me some more time because I have my exams going on.

@donnemartin
Copy link
Owner

@hashhar no rush!

@hashhar
Copy link
Contributor Author

hashhar commented Dec 18, 2015

Fixed all of the issues you mentioned. Also, the append is only being called twice (only any one of the cases will be run) in any one of the 3 branch conditionals.

@donnemartin
Copy link
Owner

@hashhar thanks for the revisions! I'll try to check it out within the next few days.

@donnemartin
Copy link
Owner

@hashhar sorry, fell behind on this from the holidays. Just want to let you know it's still on my TODO list.

@hashhar
Copy link
Contributor Author

hashhar commented Feb 4, 2016

@donnemartin Seems you are really busy. Just wanted to remind you.

@donnemartin
Copy link
Owner

@hashhar thanks for the ping, sorry it's taking awhile, this is still on my short-term radar. I plan to circle back to this project very soon and invest a lot of time beefing it up, including clearing out pull requests. Thanks!

donnemartin added a commit that referenced this pull request Feb 9, 2016
@donnemartin donnemartin merged commit 06dbb2e into donnemartin:master Feb 9, 2016
@donnemartin
Copy link
Owner

Merged, thanks @hashhar!

@hashhar hashhar deleted the better-compression branch February 9, 2016 09:35
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.

None yet

3 participants