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

create search controller #53

Merged
merged 9 commits into from
Feb 5, 2022
Merged

create search controller #53

merged 9 commits into from
Feb 5, 2022

Conversation

teruto725
Copy link
Contributor

@teruto725 teruto725 commented Feb 5, 2022

Did

  • created kanji form
  • created SearchController class
  • created kanji-visualize endpoint

Caution

  • I also created some static methods at GraphController and InfoController. Please check it. It's ok overwrite it if you want.

Isuues
#46
#47

@teruto725 teruto725 mentioned this pull request Feb 5, 2022
Copy link
Contributor

@wowry wowry left a comment

Choose a reason for hiding this comment

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

LGTM 👍 💯

@ani-hovhannisyan
Copy link
Owner

ani-hovhannisyan commented Feb 7, 2022

I have some comments related to coding, if you have time please replace them.
Nice to do later
In backend/SearchController:

  • if we use if statement for logically same checks and return similar result we can use switch case instead, butas if checks only string symbol, it's possible to use regexp with one if statement, please check the # 50 pull request, I commented similar regexp for frontend checking, so you can use it here too, something like this will work for all three conditions:
    /^[\u3000-\u303f\u3040-\u309f\u30a0-\u30ff\uff00-\uff9f\u4e00-\u9faf\u3400-\u4dbf]{1}$/

  • however, if you prefer to report 3 different messages for 3 different inputs, then I wonder why the return sends same 400 code for 3 different messages, why not to add different numbers?

  • as the message might appear other places or we might want to change it later, better to use const variable which will be written in one place and you can use it every time, otherwise you should grep where is it written and change the message, same logic with code number too, keep it in the constant variable than in return. Also, in the test you use similar messages, why not to have the config which contains the messages? What do you think of this, how to make it more generic?

  • about static methods, why do you use static instead of object method? Because you just want to connect the classes in a hurry to check your code? If so, it's fine. But later, usually it is said static method is for utility usage not the class/object specific usage, and in here we know it's not the utility.

Must follow from now

  • write more relative word to what is going to be sent as a response from api, such as host/get/kanjidata like we decided in Friday meeting: https://docs.google.com/document/d/1q9L0y9OQzgb0q9xUPpt3rnvNsbCtOJyu7gly8lK9lgk/edit#
    or anything else which has small meaning of what is the response, but not visualize, because visualization will happen in the frontend, also that word has bigger meaning than the graph nodes json is now. This is again more easy to understand later usage and fix
  • about try catch logic: we should use exception in all cases when we don't expect that behavior to be happen, or we don't know how to handle that situation, but in this case we definitely know there's no kanji, and it's usual common behavior of this project (even we were considering another use case), then why not using if/else instead?
  • I looked at the tests at the main and SearchController, but still don't get why do we trough an multiple exception for just a wrong symbol? It's crucial for server to have always reporting an EXCEPTION when get request sent some wrong symbol. As far as I know, exception is heavy than if/else. Imagine how easy it will be to take out of order (to force to crash) the server from point of hacking? Server should skip as much as possible unnecessary cases and to proceed only necessary data. But here we not only allow the wrong input to be checked in the if statements but also throw an exception and print everything about that minor thing.
  • the 27th line in SearchController is exceeding 80 symbol, usually methods and height should be small for readabality.
    see the reference Maximum Line Length (I sent it on slack chat too last Friday). If you don't agree we can use 99 length, but me personally don't prefer that.
  • print messages should always have some explanatory what is going to be printed, like a "Length of kanji", but if it's for you to track the code development, then don't commit use git add -p, or comment out it, or in bigger teams they use logging (which we don't have now :)))) :(((

Note: Please always consider your written code will be used by other developers too, so write as much as possible generic code, which is easy to debug, to enhance, to fix and find the issues.

@teruto725 teruto725 linked an issue Feb 7, 2022 that may be closed by this pull request
@teruto725
Copy link
Contributor Author

teruto725 commented Feb 7, 2022

I do refactoring at new issue
#57

@teruto725 teruto725 linked an issue Feb 8, 2022 that may be closed by this pull request
@teruto725 teruto725 deleted the feature/search-controller branch February 16, 2022 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coding Coding related task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[backend] create backend post api for receive kanji. [frontend] Create search form
3 participants