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

New Exercise: Word count #90

Merged
merged 18 commits into from
Jun 8, 2022
Merged

New Exercise: Word count #90

merged 18 commits into from
Jun 8, 2022

Conversation

marianfoo
Copy link
Collaborator

@marianfoo marianfoo commented Mar 26, 2022

Quite a few errors in solution.
Set status to "WIP" so only maintainers see this on exercism (as documented here)

Source

https://github.com/exercism/javascript/tree/main/exercises/practice/word-count

SAP Unit Tests

image

ABAP Lint

Few errors

local_testclass_location

Don´t know what that means

./exercises/practice/word-count/zcl_word_count.clas.testclass.abap[1, 7] - Place local testclass "ltcl_word_count" in the testclass include (local_testclass_location) 

check_syntax

May have a problem with the GROUP.

./exercises/practice/word-count/.meta/zcl_word_count.clas.abap[51, 7] - Component "size" not found in structure (check_syntax)

check_syntax

return_structure is a local type.

./exercises/practice/word-count/.meta/zcl_word_count.clas.abap[52, 7] - "return_structure" not found, findTop (check_syntax) 

Run Tests in Docker

Don´t know if my docker run is faulty because there are many errors in the output.

image

@marianfoo marianfoo requested a review from larshp March 26, 2022 14:17
@larshp
Copy link
Member

larshp commented Mar 29, 2022

rename file zcl_word_count.clas.testclass.abap to zcl_word_count.clas.testclasses.abap

@marianfoo marianfoo marked this pull request as ready for review March 29, 2022 12:19
@larshp
Copy link
Member

larshp commented Mar 29, 2022

LOOP AT GROUP BY does not work yet

@larshp larshp removed their request for review March 29, 2022 12:19
@marianfoo
Copy link
Collaborator Author

LOOP AT GROUP BY does not work yet

no worries, It's probably better to refactor the solution than wait for the implementation which is surely not that easy, right?

@larshp
Copy link
Member

larshp commented Mar 29, 2022

we'll never really know 😅

if the exercise is most easily solved with GROUP BY, then we should wait, but 🤷‍♂️ everything has different solutions

@marianfoo
Copy link
Collaborator Author

ok, i have a look if i find a different solution

but there is also the other error
Is this about VALUE ?
image
image
image

@pokrakam
Copy link
Collaborator

If the obvious solution is to use group by then we should hold back, because it means others will try that too.

I had a look at the exercise, and can see a couple of ways. IMHO group by is something more savvy people are likely to try, but that's pure speculation.

@larshp
Copy link
Member

larshp commented Mar 29, 2022

@marianfoo can you paste the full code example?

Update: oh, its the original code, guess its a derived error

@marianfoo
Copy link
Collaborator Author

@marianfoo
Copy link
Collaborator Author

@marianfoo can you paste the full code example?

Update: oh, its the original code, guess its a derived error

ah ok, would make sense

@marianfoo
Copy link
Collaborator Author

If the obvious solution is to use group by then we should hold back, because it means others will try that too.

I had a look at the exercise, and can see a couple of ways. IMHO group by is something more savvy people are likely to try, but that's pure speculation.

You can try with a different solution :D
In any case, it is not a solution for beginners.

@pokrakam
Copy link
Collaborator

You can try with a different solution :D

I didn't mean for us, of course we can find workarounds 🙂

What I meant was caution about putting out something where the probability of people running into issues is high. Maybe put a note in the description to avoid LOOP with GROUP BY if time to resolve is unknown, or hold this in draft if we expect it can be implemented in the next week or so.

@larshp
Copy link
Member

larshp commented Mar 29, 2022

perhaps just wait a bit, focus on getting #87 merged, then there are 2 more ready to merge?

might leave some time for GROUP BY to be implemented 😅

@marianfoo
Copy link
Collaborator Author

marianfoo commented Apr 1, 2022

perhaps just wait a bit, focus on getting #87 merged, then there are 2 more ready to merge?

might leave some time for GROUP BY to be implemented 😅

The two others (#88, #89) are concept exercises which need the concepts created first.
This is a "normal" exercise we can publish if it would work.

@mbtools
Copy link
Contributor

mbtools commented May 29, 2022

I don't think loop ... group by is common knowledge. Seems complicated.

here's my solution:

  METHOD count_words.
    DATA(clean) = replace( val = to_lower( phrase ) sub = `'` with = `` occ = 0 ).
    clean = replace( val = clean sub = `\n` with = ` ` occ = 0 ).
    clean = replace( val = clean sub = `\t` with = ` ` occ = 0 ).
    clean = replace( val = clean regex = `[^a-z0-9]` with = ` ` occ = 0 ).

    SPLIT condense( clean ) AT ` ` INTO TABLE DATA(words).

    LOOP AT words INTO DATA(word).
      DATA(one_result) = VALUE return_structure( word = word count = 1 ).
      COLLECT one_result INTO result.
    ENDLOOP.
  ENDMETHOD.

this does require a change of the result type i.e. with key word which should be done anyway:

    TYPES:
      return_table TYPE STANDARD TABLE OF return_structure WITH KEY word.

PS: Since it's now an abapGit repo, this needs package.devc.xml and the xml for the class. Best to push from a SAP system using abapGit. Then copy the class files to the .meta folder and add the solution there (using GH or VScode).

PPS: abaplint errors will go away with #157.

@marianfoo
Copy link
Collaborator Author

@mbtools thanks for you solution, i created the abapGit Config
it works in 1909 but unfortunately not here
image

@larshp can you please help here with the error message
image

@mbtools
Copy link
Contributor

mbtools commented Jun 8, 2022

yeah, I forgot that the collect statement isn't implemented yet
https://github.com/abaplint/transpiler/blob/main/packages/transpiler/src/statements/collect.ts

simple statement in abap but lot's of things happening behind the scenes.

you could replace it with:

DATA(one_result) = VALUE return_structure( word = word count = 1 ).
" COLLECT one_result INTO result.
READ TABLE result ASSIGNING FIELD-SYMBOL(<result>) WITH TABLE KEY word = one_result-word.
IF sy-subrc = 0.
  <result>-count = <result>-count + one_result-count.
ELSE.
  INSERT one_result INTO TABLE result.
ENDIF.

@marianfoo
Copy link
Collaborator Author

@mbtools green 🥳

Copy link
Contributor

@mbtools mbtools left a comment

Choose a reason for hiding this comment

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

just remove tests.toml, results.json

@marianfoo
Copy link
Collaborator Author

just remove tests.toml, results.json

done

@marianfoo marianfoo requested a review from mbtools June 8, 2022 09:39
Copy link
Contributor

@mbtools mbtools left a comment

Choose a reason for hiding this comment

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

Great 👍

@mbtools
Copy link
Contributor

mbtools commented Jun 8, 2022

just a question of how many students will use collect ... @larshp can you deal with the "pressure"?

@larshp larshp merged commit c0cdb04 into exercism:main Jun 8, 2022
@marianfoo
Copy link
Collaborator Author

just a question of how many students will use collect ... @larshp can you deal with the "pressure"?

programming in abap for 4 years, never heard of it
So i guess not that many use it?

@marianfoo
Copy link
Collaborator Author

Exercise is only and works with the solution :)
image

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