Fix linear search

[dlesnoff]

Fix #31 and CI. Ran nimpretty directly.

[ZoomRmc]

@beef331 is right. Since the logic has already been changed to iterate the array in ascending manner, better to change the default to Natural(0).

I would also removed the custom type aliases, it trades better locality for shorter lines (that still wrap), so no win as I see it.

What's the reason for changing the func to proc?

[dlesnoff]

What's the reason for changing the func to proc?

I used echo and forgot to change it. Done.

I would also removed the custom type aliases, it trades better locality for shorter lines (that still wrap), so no win as I see it.

Same intent here. Done.

@beef331 is right. Since the logic has already been changed to iterate the array in ascending manner, better to change the default to Natural(0).

I had overlooked that. I completely forget that the index is not retained with openarray[T]. Done.

[SatinWukerORIG]

Maybe just do this, to avoid confusion. Overall nim prettify has done illustriously 👍
And indeed, adding type definition to Natural and Option[Natural] looks redundant and verbose. well done!

[pietroppeter]

As a note on linearSearch I think current version is buggy in case of arrays with negative indices (which are allowed for open array), see https://play.nim-lang.org/#ix=4yJs

import std/options

func linearSearch*[T](arr: openArray[T], key: T): Option[Natural] =
  # key is the value we are searching for in the array.
  for i, val in arr.pairs():
    if val == key:
      return some(Natural(i))
  none(Natural) # `key` not found

type
  NegIdxArray = array[-1 .. 1, int]

let
  a: NegIdxArray = [1,2,3]

echo linearSearch(a, 1) # some(0)

Correct answer should be some(-1)

[dlesnoff]

@pietroppeter This is also the case for any array with a custom starting index, isn't it?
This won't fix since I have no way to pass to the procedure the initial arr.low and arr.high apart from giving them explicitely as function parameters.
I will add a comment and call it a day.
I think that it is the caller's responsibility to readapt to the correct indexing afterwards.

echo some(linearSearch(a, 1).get() + a.low)

is a partial fix for the caller. This now raises a UnpackDefect when linearSearch returns none(Natural).

[beef331]

This is the case for any array that does not start at 0 or use a data type that is an integer, it's probably best to just have an openArray and a array[Idx, T] search.

[dlesnoff]

Having two functions for each would be contrary to the DRY - Don't Repeat Yourself principle. That feels like unnecessary duplicated code for me.

[beef331]

Sure but they have to be specialized. array[bool, T] or array[MyEnum, T] gets Option[Natural] which requires a type conversion back to index type by the user. The most reusable that should work for all types and the least amount of DRY breaking is as follows:

func linearSearchImpl[T](arr: openArray[T], key: T): int =
  for i, val in arr.pairs():
    if val == key:
      return i
  -1
  
func linearSearch*[T](arr: openArray[T], key: T): Option[Natural] =
  let val = linearSearchImpl(arr, key)
  if val >= 0:
    some(Natural(val))
  else:
    none(Natural)
    
func linearSearch*[Idx, T](arr: array[Idx, T], key: T): Option[Idx] =
  let val = linearSearchImpl(arr, key)
  if val >= 0:
    some(Idx(ord(arr.low) + val))
  else:
    none(Idx)  

[pietroppeter]

Ah, I did not know that openarray did not support low and high. checked here indeed it has 0 as low always for openarray: https://play.nim-lang.org/#ix=4yJD

[beef331]

Well they support it as you've shown, but openArray is explicitly a type erasure so yea it loses the source type information. It's implemented as a ptr T, len afterall.

[dlesnoff]

@beef331 I do not want to merge your modifications. It adds much complexity to handle an often superfluous feature of array reindexing. I prefer to keep the implementation as simple as possible.

While one of our implementation's goals is « easy integration in one's code project », our primary goal is pedagogical algorithm teaching. I will use your code if indexed arrays reveal to be needed in the future.

[ZoomRmc]

@beef331 I do not want to merge your modifications. It adds much complexity to handle an often superfluous feature of array reindexing. I prefer to keep the implementation as simple as possible.

In this case it would be nice to document that the function does return the absolute index and not a corresponding index value for range/enum-indexed arrays.

[Panquesito7]

Approving to allow the merge. Thanks. 👍