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

Ports - Kate #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Ports - Kate #16

wants to merge 1 commit into from

Conversation

goblineer
Copy link

No description provided.

Copy link

@ace-n ace-n left a comment

Choose a reason for hiding this comment

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

Tests pass and code style looks good! Nice work! 👍

The time complexity analysis looks right - any ideas on how we can improve the space one?

# If any number is found to be 0, the method updates all the numbers in the
# corresponding row as well as the corresponding column to be 0.
# Time complexity: O(n*m), where n and m are nums of rows and columns
# Space complexity: O(n + m), where n and m are nums of rows and columns
Copy link

Choose a reason for hiding this comment

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

Space complexity looks better than n + m to me. 🙂

Any ideas on what it might be?

Copy link
Author

Choose a reason for hiding this comment

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

Oh, good question! So ... since this method doesn't create any extra structures to hold data, is it just constant? This was my first try at a matrix and I think I forgot that all it is is a single particularly organized array. O(n)?

Copy link

Choose a reason for hiding this comment

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

I'd agree with the former - if you wanted to be super-technically correct, you could say "constant additional space". (Excluding the size of your input and output variable(s) is a matter of convention).

columns.times do |column|
next unless matrix[row][column] == "ZERO"

columns.times { |i| matrix[row][i] = 0 unless matrix[row][i] == "ZERO" }
Copy link

Choose a reason for hiding this comment

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

Personal preference nit: some programmers (e.g. many Java people) might find this a bit too concise, and will prefer something more explicit.

Rubyists, unlike Java people, sometimes embrace this conciseness even if it makes code take slightly more time to understand - as it results in less code needing to be understood. 🙂

(Of course - like all personal preferences, this one's totally up to you.)

Copy link
Author

Choose a reason for hiding this comment

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

Thank you! It's really helpful to hear about style preferences. When I work on this sort of thing in a vacuum, I can get totally drawn into the temptation to make it beauuuuuutifully elegant and forget about clarity/other people 😮 🙂

Copy link

@ace-n ace-n May 22, 2019

Choose a reason for hiding this comment

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

Yep - and style preferences depend heavily on the existing codebase. Many codebases/companies will have style guides, but inevitably your first few PRs at a new company (or even a new codebase within the same company! - e.g. iOS vs. Android) will be spent learning (and internalizing) that codebases' specific guidelines/patterns.

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.

2 participants