Skip to content

Conversation

icemberg
Copy link
Contributor

@icemberg icemberg commented Oct 5, 2024

Please mention the label for hacktoberfest and gssoc' extd 24 . It will be helpful. I am willing to contribute further . Can you please help me if I have any doubts??

@icemberg icemberg closed this Oct 5, 2024
@icemberg icemberg reopened this Oct 5, 2024
Copy link
Collaborator

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Also, Step X: ... comments should just be .... The enumeration isn't helpful and is harmful when "steps" are added / removed.

@@ -0,0 +1,58 @@
function counting_sort(arr, key_function)
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. This function should be returned directly for require to work with this as expected rather than polluting globals;
  2. I would rename arr to list for consistency with the rest of the code.
  3. key_function should have a comment explaining what it does and what it defaults to. Especially since this sorting algorithm has special requirements (keys must be integers).

-- If no key_function is provided, use identity function
key_function = key_function or function(x) return x end

-- Step 1: Find the range of keys (min_key and max_key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Code is self-explanatory, no comment needed. You can also simplify this to

local min_key, max_key = math.huge, -math.huge
for _, elem in ipairs(list) do
    local key = key_function(elem)
    min_key = math.min(min_key, key)
    max_key = math.max(max_key, key)
end


-- Step 2: Initialize the count array
local count = {}
for i = min_key, max_key do
Copy link
Collaborator

Choose a reason for hiding this comment

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

You get a problem here when arr is empty. Please add a special case to return early in that case.
Furthermore, as it currently is, this may (unnecessarily) populate the hash part if min_key is not 1, so please shift accordingly to prevent that.

end

-- Step 3: Count the occurrences of each key. In this case key is same as arr[i]
for i = 1, #arr do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again use ipairs

count[i] = 0
end

-- Step 3: Count the occurrences of each key. In this case key is same as arr[i]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary comment, also slightly misleading (keys are produced by the key function).

end
end

-- Sample array
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add proper tests rather than some example code at the end of the file. This can essentially use the same tests as Radix Sort: check_sort(countingsort, nil, true), see https://github.com/TheAlgorithms/Lua/blob/main/.spec/sorting/sort_spec.lua.

@icemberg icemberg closed this Oct 7, 2024
@icemberg icemberg reopened this Oct 7, 2024
@icemberg
Copy link
Contributor Author

icemberg commented Oct 7, 2024

Taken all the specified changes into account..

@icemberg
Copy link
Contributor Author

icemberg commented Oct 7, 2024

Made all the changes

@icemberg
Copy link
Contributor Author

icemberg commented Oct 7, 2024

Can you please check this once..

max_key = math.max(max_key, key)
end

-- Initialize the count array
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
-- Initialize the count array

-- key_function to map elements to integer keys, defaults to identity
key_function
)
-- Default to identity function if no key_function is provided
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
-- Default to identity function if no key_function is provided

-- Default to identity function if no key_function is provided
key_function = key_function or function(x) return x end

-- Handle empty list case
Copy link
Collaborator

@appgurueu appgurueu Oct 7, 2024

Choose a reason for hiding this comment

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

Suggested change
-- Handle empty list case

(If anything a useful comment would explain why not handling it early creates problems further down the line. The fact that we are handling it here early is obvious.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I am sorry for this issue.....

@icemberg
Copy link
Contributor Author

icemberg commented Oct 8, 2024

Removed the comments that you mentioned . Retained the comments which have the core logic for this sorting technique.

end)
describe("Countingsort", function()
-- Test with multiple radii
local countingsort = require("sorting.countingsort")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
local countingsort = require("sorting.countingsort")

@icemberg
Copy link
Contributor Author

icemberg commented Oct 9, 2024

Is it correct now?

@appgurueu appgurueu merged commit cedc141 into TheAlgorithms:main Oct 10, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants