Skip to content

Conversation

@masiljangajji
Copy link
Contributor

pull request

Approaches for Parallel Letter Frequency, ready for review!

Resolves #2710


Reviewer Resources:

Track Policies

Copy link
Member

@kahgoh kahgoh left a comment

Choose a reason for hiding this comment

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

Nice, I think this generally looks good! There's just a couple of things to fix in the Markdown.

<!-- Your content goes here: -->



Copy link
Member

Choose a reason for hiding this comment

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

I don't think this related to the PR. Could you please undo this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this related to the PR. Could you please undo this change?

the following error will occur.

.github/PULL_REQUEST_TEMPLATE.md:5 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]
.github/PULL_REQUEST_TEMPLATE.md:6 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 3]

Should we go ahead and cancel it anyway?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think we should. I've just tried running our Markdown Lint action on our main branch (before your change). I think the rules are a bit different with that one.

# Introduction

There are multiple ways to solve the Parallel Letter Frequency problem.
One approach is to use parallelStream, and another involves using ForkJoinPool.
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the existing approaches for other exercises, such as grains, I think it would be good to format the methods and classes with backticks (``) to maintain consistency in the formatting (example in the suggestion below). Could you go through and update?

Suggested change
One approach is to use parallelStream, and another involves using ForkJoinPool.
One approach is to use `Stream.parallelStream`, and another involves using `ForkJoinPool`.


## General guidance

To count occurrences of items, a map data structure is often used, though arrays and lists can work as well. A map, being a key-value pair structure, is suitable for recording frequency by incrementing the value for each key.
Copy link
Member

Choose a reason for hiding this comment

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

We generally format our Markdown one sentence per line. Could you please go through and update the format?

Suggested change
To count occurrences of items, a map data structure is often used, though arrays and lists can work as well. A map, being a key-value pair structure, is suitable for recording frequency by incrementing the value for each key.
To count occurrences of items, a map data structure is often used, though arrays and lists can work as well.
A map, being a key-value pair structure, is suitable for recording frequency by incrementing the value for each key.


Each subtask in LetterCountTask will continue calling compute() to divide itself further until the range is smaller than or equal to the threshold.

For tasks that are within the threshold, letter frequency is calculated. The [`isAlphabetic`][isAlphabetic] method is used to identify all characters classified as alphabetic in Unicode, covering various languages like English, Korean, Japanese, Chinese, etc., returning true for alphabetic characters and false for numbers, special characters, spaces, and others.
Copy link
Member

Choose a reason for hiding this comment

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

Suggest using Character.isAlphabetic here to make it clear the method is from the Character class.

Suggested change
For tasks that are within the threshold, letter frequency is calculated. The [`isAlphabetic`][isAlphabetic] method is used to identify all characters classified as alphabetic in Unicode, covering various languages like English, Korean, Japanese, Chinese, etc., returning true for alphabetic characters and false for numbers, special characters, spaces, and others.
For tasks that are within the threshold, letter frequency is calculated.
The [`Character.isAlphabetic`][isAlphabetic] method is used to identify all characters classified as alphabetic in Unicode, covering various languages like English, Korean, Japanese, Chinese, etc., returning `true` for alphabetic characters and `false` for numbers, special characters, spaces, and others.

@masiljangajji
Copy link
Contributor Author

Hi @kahgoh Thanks for reviewing!

I’ve incorporated the feedback and committed the changes. However, when I run markdownlint-cli2 on PULL_REQUEST_TEMPLATE.md encounter the error , Please check it

Copy link
Member

@kahgoh kahgoh 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 the updates! There are still some places where I think we should place backticks. I've gone ahead and added them as suggestions.

I wasn't sure if there was misunderstanding in regards my earlier comment about having a sentence per line as I can see there are now empty lines between each sentence. While each sentence should start on its own line, there should only be empty lines between paragraphs.

For example, this from your first commit was fine:


It looks like this:
For example, this from your first commit was fine:


There are multiple ways to solve the Parallel Letter Frequency problem.
One approach is to use `parallelStream`, and another involves using `ForkJoinPool`.

It renders like this:

There are multiple ways to solve the Parallel Letter Frequency problem. One approach is to use parallelStream, and another involves using ForkJoinPool.

But this makes two paragraphs:

There are multiple ways to solve the Parallel Letter Frequency problem.

One approach is to use `parallelStream`, and another involves using `ForkJoinPool`.

It looks like this:

There are multiple ways to solve the Parallel Letter Frequency problem.

One approach is to use parallelStream, and another involves using ForkJoinPool.

Notice the first example looks like one paragraph, whereas the second looks like two.

See other approaches, like the collatz conjecture or Difference of squares for examples on how they are formatted.

I thought the arrangement of paragraphs or empty lines from your first commit was fine - we just needed to move the sentences to start on its own line. I have added some suggestions where the empty lines can be removed to turn them back to paragraphs. Feel free to go through and update other spots accordingly.

<!-- Your content goes here: -->



Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think we should. I've just tried running our Markdown Lint action on our main branch (before your change). I think the rules are a bit different with that one.


The core of [`ForkJoinPool`][ForkJoinPool] is the Fork/Join mechanism, which divides tasks into smaller units and processes them in parallel.

THRESHOLD is the criterion for task division.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
THRESHOLD is the criterion for task division.
`THRESHOLD` is the criterion for task division.


THRESHOLD is the criterion for task division.

If the range of texts exceeds the THRESHOLD, the task is divided into two subtasks, and [`invokeAll`][invokeAll](leftTask, rightTask) is called to execute both tasks in parallel.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If the range of texts exceeds the THRESHOLD, the task is divided into two subtasks, and [`invokeAll`][invokeAll](leftTask, rightTask) is called to execute both tasks in parallel.
If the range of texts exceeds the `THRESHOLD`, the task is divided into two subtasks, and [`invokeAll(leftTask, rightTask)`][invokeAll] is called to execute both tasks in parallel.


If the range of texts exceeds the THRESHOLD, the task is divided into two subtasks, and [`invokeAll`][invokeAll](leftTask, rightTask) is called to execute both tasks in parallel.

Each subtask in LetterCountTask will continue calling compute() to divide itself further until the range is smaller than or equal to the threshold.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Each subtask in LetterCountTask will continue calling compute() to divide itself further until the range is smaller than or equal to the threshold.
Each subtask in `LetterCountTask` will continue calling `compute()` to divide itself further until the range is smaller than or equal to the threshold.


The Java 8 [`stream`][stream] API provides methods that make parallel processing easier, including the parallelStream() method.

With parallelStream(), developers can use the ForkJoinPool model for workload division and parallel execution, without the need to manually manage threads or create custom thread pools.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
With parallelStream(), developers can use the ForkJoinPool model for workload division and parallel execution, without the need to manually manage threads or create custom thread pools.
With `parallelStream()`, developers can use the ForkJoinPool model for workload division and parallel execution, without the need to manually manage threads or create custom thread pools.


The [`Character.isAlphabetic`][isAlphabetic] method is used to identify all characters classified as alphabetic in Unicode, covering various languages like English, Korean, Japanese, Chinese, etc., returning true for alphabetic characters and false for numbers, special characters, spaces, and others.

Additionally, since uppercase and lowercase letters are treated as the same character (e.g., A and a), each character is converted to lowercase.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Additionally, since uppercase and lowercase letters are treated as the same character (e.g., A and a), each character is converted to lowercase.
Additionally, since uppercase and lowercase letters are treated as the same character (e.g., `A` and `a`), each character is converted to lowercase.

# Introduction

There are multiple ways to solve the Parallel Letter Frequency problem.

Copy link
Member

Choose a reason for hiding this comment

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

Suggest removing the empty line to turn back into paragraph.

Suggested change

## General guidance

To count occurrences of items, a map data structure is often used, though arrays and lists can work as well.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

To count occurrences of items, a map data structure is often used, though arrays and lists can work as well.

A [`map`][map], being a key-value pair structure, is suitable for recording frequency by incrementing the value for each key.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Parallel processing typically takes place in a multi-[`thread`][thread] environment.

The Java 8 [`stream`][stream] API provides methods that make parallel processing easier, including the parallelStream() method.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@masiljangajji
Copy link
Contributor Author

Hi @kahgoh
I'm not very familiar with English, so I made a few mistakes. Thank you for the detailed review!

Copy link
Member

@kahgoh kahgoh left a comment

Choose a reason for hiding this comment

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

Thanks! I think we're almost there! Just spotted a couple of minor things.


`THRESHOLD` is the criterion for task division.
If the range of texts exceeds the `THRESHOLD`, the task is divided into two subtasks, and [`invokeAll(leftTask, rightTask)`][invokeAll] is called to execute both tasks in parallel.
Each subtask in LetterCountTask will continue calling compute() to divide itself further until the range is smaller than or equal to the `THRESHOLD`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggest clarifying that compute is called through invokeAll. Your example only shows invokeAll being called, not compute, so it might be confusing. Also, suggest putting the LetterCountTask and compute in backticks for formatting.

Suggested change
Each subtask in LetterCountTask will continue calling compute() to divide itself further until the range is smaller than or equal to the `THRESHOLD`.
Each subtask in `LetterCountTask` will continue calling `compute()` (via `invokeAll(leftTask, rightTask)`) to divide itself further until the range is smaller than or equal to the `THRESHOLD`.

@masiljangajji
Copy link
Contributor Author

@kahgoh Thank you! Is there anything else that could be improved?

Copy link
Member

@kahgoh kahgoh left a comment

Choose a reason for hiding this comment

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

Thanks! It looks good to me now!

@kahgoh kahgoh merged commit e28da7b into exercism:main Nov 14, 2024
2 checks passed
@masiljangajji
Copy link
Contributor Author

masiljangajji commented Nov 14, 2024

@kahgoh Thank you so much for the detailed review. This is my first time contributing to open source, and since I'm not very familiar with English, I made a lot of mistakes. Thanks to you, I was able to have a great experience!

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.

#48in24: Add approaches to Parallel Letter Frequency

2 participants