Skip to content

Conversation

@SatinWukerORIG
Copy link
Member

I only added one searching algorithm to this directory, linear search, because I just want to make sure that the comments, format, and code style I did are correct and appropriate according to the contribution guidelines. If linear_search.nim is okay, I will continue working on other searching algorithms. @dlesnoff
And please review my code, I can't wait to make more contributions haha
Thank you!

@SatinWukerORIG SatinWukerORIG requested a review from dlesnoff as a code owner June 19, 2023 08:40
Adding time and space complexity
Support search in types other than int, such as char and string
Copy link
Collaborator

@dlesnoff dlesnoff left a comment

Choose a reason for hiding this comment

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

Thanks for this new contribution!
I made some style comments. I am going to do a second pass for the algorithm itself tonight.

@dlesnoff
Copy link
Collaborator

I would copy other the-algorithms repositories and name the repertory searches instead.

SatinWukerORIG and others added 4 commits June 19, 2023 03:01
Co-authored-by: dlesnoff <54949944+dlesnoff@users.noreply.github.com>
Co-authored-by: dlesnoff <54949944+dlesnoff@users.noreply.github.com>
@ZoomRmc
Copy link
Contributor

ZoomRmc commented Jun 19, 2023

Nicely done! A few general remarks:

  • Lines starting with double octothorps are doc comments. To work properly, they go under the thing, using the proper indentation.
  • In the header the multiline comment is a regular one, it won't be included in the generated documentation.
  • Since Nim is very overload heavy, following argument conventions (including naming and ordering) is more important than in many other languages. For searches, consider following the standard library and use key for the searched argument name. (See binarySearch)
  • Explaining what the common types (like openArray) is probably excessive.
  • There's a ..< for "[A, B)" (end not included) ranges.
  • Use return only when you're modifying the control flow (early returns), for regular returns use expressions or an implicitly declared result (I strongly prefer the former).
    So, to return a value from a proc/block, instead of
    proc foo(a: T): T =
      return foo(bar)
    Use:
    proc foo(a: T): T =
      foo(bar)
  • Consider using the module-level runnableExamples block for explaining the usage, comparison between implemented functions and general narration. This allows the per-funciton documentation to be more succinct and technical.
  • For input index arguments, use Natural.

EDIT: Add indentation to the code examples.

@dlesnoff
Copy link
Collaborator

For input index arguments, use Natural.

Problem: the item might not be in the array, and @SatinWuker returns -1, so a Natural is not an option.
I would recommend using an Option[Natural] for the index instead.

@ZoomRmc
Copy link
Contributor

ZoomRmc commented Jun 19, 2023

For input index arguments, use Natural.

Problem: the item might not be in the array, and @SatinWuker returns -1, so a Natural is not an option. I would recommend using an Option[Natural] for the index instead.

You're correct. Seems I was talking about input arguments in general. For this specific function int is fine. Option[Natural] for both the input and output is ok too - any performance penalty is probably offset by the recursive nature of the proc.

Copy link
Collaborator

@dlesnoff dlesnoff left a comment

Choose a reason for hiding this comment

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

You are almost there.

SatinWukerORIG and others added 2 commits June 20, 2023 13:48
Co-authored-by: dlesnoff <54949944+dlesnoff@users.noreply.github.com>
Co-authored-by: dlesnoff <54949944+dlesnoff@users.noreply.github.com>
@SatinWukerORIG
Copy link
Member Author

Ahhh finally

Copy link
Member Author

@SatinWukerORIG SatinWukerORIG left a comment

Choose a reason for hiding this comment

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

I have double-checked, and the code works 👍
Hope this will be merged so I can move on to other tasks other than searches algorithms lol

Copy link
Collaborator

@dlesnoff dlesnoff left a comment

Choose a reason for hiding this comment

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

The code looks much better to me. I am about to merge this, but the comments do not reflect the last code changes. The writing style can be improved.

@dlesnoff
Copy link
Collaborator

Please run nim r .scripts/directory.nim > DIRECTORY.md and add the file to your PR.

Co-authored-by: dlesnoff <54949944+dlesnoff@users.noreply.github.com>
Co-authored-by: dlesnoff <54949944+dlesnoff@users.noreply.github.com>
@SatinWukerORIG
Copy link
Member Author

Please run nim r .scripts/directory.nim > DIRECTORY.md and add the file to your PR.

DIRECTORY.md updated 👌

Copy link
Collaborator

@dlesnoff dlesnoff left a comment

Choose a reason for hiding this comment

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

You nailed it. Solid first contribution that will serve as reference for others.

@dlesnoff dlesnoff merged commit c3440d4 into TheAlgorithms:main Jun 20, 2023
@SatinWukerORIG
Copy link
Member Author

You nailed it. Solid first contribution that will serve as reference for others.

Thank you so much for helping me from the beginning all the way to the end! This process is just like a linear search hahaha.
I really learned a lot from you guys, and I will write better-quality code and maintain this project.

@ZoomRmc
Copy link
Contributor

ZoomRmc commented Jun 20, 2023

Thank you for the contribution and sorry for a bit laborious review process.

Nim has an exceptionally high educative potential due to its uncluttered and intuitive syntax, so adhering to strict guidelines within this repository right from the get go can showcase it in a good light and help it stand out in comparison with implementations in other languages.

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