januschung / mock

GNU General Public License v2.0
3 stars 0 forks source link

A mostly tooling review from reddit #3

Open LIttleAncientForestKami opened 1 year ago

LIttleAncientForestKami commented 1 year ago

Hi and all the best in the new year 2023! I hope it will be a better year for you, especially the ending!

I've seen your post on Reddit and decided to give some feedback. Others focused mostly (if not only) on your code. I'll focus on everything else then. In as-I-spotted-it order:

  1. com.tnite package suggests you own the tnite.com domain. Java packages are RDNS-like for a reason. Consider repackaging as io.github.januschung. Or leave, it's not a major problem unless you intend to publish the library on Maven Central or other sites like that.
  2. Minor. Dockerfile looks for app.jar, yet Maven produces a full-jar-name-with-snapshot.jar - did not try so perhaps a non-issue but ain't this a problem?
  3. Security / Performance. Docker base image of your choice has a slimmer and an (arguably) tad more secure version. Snyk report for openjdk-17: https://snyk.io/test/docker/openjdk%3A17 and for slimmer version: https://snyk.io/test/docker/openjdk%3A17-slim - whatever your choice here, I'd mark this with a comment in the Dockerfile itself, so others (or you yourself in 3 months) will know and perhaps do something with it. Leaving yourself an issue about it here in the repository is also fine.
  4. Minor. Tooling. I'd add tool files to your .gitignore (IDE). If you use global .gitignore, do ignore this.
  5. Minor. Tooling. A good practice is to have .gitattributes, unless you are a sole-system user and you don't expect to share code with cross-platform users.
  6. Minor. Tooling. Maven wrapper. Something that allows anyone to run your repository without installing or possessing Maven.
  7. Security. Tooling again. GitHub dependabot. I'd consider making it a collaborator here in the repository so that it would point out things for you, like which versions of your dependencies are unsafe, outdated, etc. I heard GitHub makes the scans for most (or all?) public repos so perhaps you won't have to, but I'd check.
  8. Tooling. Consider Spring Actuator. https://docs.spring.io/spring-boot/docs/current/reference/html/actuator.html

POM

  1. Packaging as JAR is done by default, tag can be omitted.
  2. If you publish, do not use SNAPSHOT versions. Snapshot versions indicate this is not ready, however if you ask for public opinion, it's somewhat ready. That's confusing. I'd suggest making this a 1.0 version, or 1.0-beta (less interesting for releasing later versions will be problematic, read on why Mockito 2 appeared as 2.1 and not 2.0).
  3. The tag http://maven.apache.org would be fine if you worked for Maven or were maintaining Maven plugins. A mighty good practice, one that really makes a difference, is to launch a project GitHub page (here in this very repo) and point the URL at it. It might be a simple static HTML, or MD file that will be converted to HTML, illustrating usages, examples and the like - the beginning of the documentation portal. If you don't have the desire / time / whatever, just point at this repository.
  4. Formatting is peculiar - some lines are indented different than others for no easily discernible reason. Also, seeing quickly what test dependencies you have is not easy (see next point).
  5. My litmus test for how good a person is with Maven is to see their properties in the POM. What I usually look for:

Tests

The largest tip I can give you here is: consider the test pyramid. You may want to layer your tests and add Maven failsafe plugin for tests that are not UNIT ones (aka: they talk with something, a file system, a web stack and protocol, a foreign service).

API tests should be something here, after all, REST service is what you are doing. While we're on REST topics, have you looked at National Bank of Belgium REST design guide? It's on wiki on their github. You may want to look at that and see if you don't want to introduce some changes in your REST endpoints.

Performance tests are something I'd be interested in when evaluating a library like yours. How performant is it? Consider a simple test of brute forcing your API. I'd say JMeter will be rather easy to use here and you can add these tests in this repository and add a docs section instructing how to launch them. If you have SDKman or ASDF installing JMeter is done via single command sdk i jmeter and there you go, type jmeter and start using. Feel free to go for Gatling of you want a nicer/more modern/often paid tool.

WireMock

There's a project that your project will be compared to. My advice: add a section to readme saying something like "Yeah, it's similar, no I did not know WireMock at the time of writing" or "Screw WireMock, use my library because..." or "it's a practice project, so it does duplicate some features..." - anything that will explain this to your satisfaction and will allow you to point all the folks asking about it to readme. :-)

JavaDocs

Notably absent. Best feature of JavaDocs are the examples. Please add examples to your public APIs, helping a junior programmer understand how to use this API and what to use it for.

BTW, if you upgrade to Java 18, you get @snippet tag in Javadocs. https://docs.oracle.com/en/java/javase/18/code-snippet/index.html

All the best to you and happy New Year!

januschung commented 1 year ago

Greetings to @LIttleAncientForestKami!

Thank you for spending so much time to check out the project and provided me with the suggestion. I would like to summarize what I have learned from you so far:

  1. I didn't know tooling like snyk.io exists and would have not check potential security issue from any 'official docker image'. This is a very great tip. I found there is no security concern for https://snyk.io/test/docker/amazoncorretto%3A17.0.5-alpine3.16 so I might switch to it if it works for my project.
  2. I am going to remove the maven url from the pom file and possibly replace it to the github repo for now. If ever one of my java project reach more audience I will definitely come up with a landing page. This again is something I didn't know of so thanks for pointing that out.
  3. I should have checked the formatting better; I just setup my working environment brand new from the recent laid off. I am switching from intellij to eclipse and am learning to set thing up properly. I will update all those tab with space for the next commit.
  4. The concept of .gitattributes has been ignored by me. I will look for a sample and add it.
  5. Adding spring actuator is a good idea
  6. For the versioning, this project probably will never be ready since one has to customize it for his/her/their usage to fit the need. I will pay attention to it for my next project definitely.
  7. In my previous life my team did use Maven failsafe plugin. I write it down here to remind myself to check if I need it for future purpose. Thanks for pointing this out.
  8. I didn't check "National Bank of Belgium REST design guide" but I will now :).
  9. I will look for performance testing for this project just for fun. It would be nice if I can wire it to github action.
  10. This is actually the first time I heard about wiremock. I have no intention to compete or cover a lot of what they do by myself. My original purposes for this project are
    • to keep myself fresh with what I learned from the previous job
    • to have something to showcase in an interview if I am ever asked
    • to simply clone and make quick change for future need without starting from scratch again
    • to hope for getting advice from the community for feedback and improvement (thank you for that)
    • to give back to the community if someone ever find it useful or get something out of it
  11. Since this will never be a complete project without customization, I am not planning to add any documentation. I am going to add Openapi though if it supports Spring boot 3.

I feel like I have to type a long response to you to show my gratitude. Sorry it is not organized :). I will make the change during the week while job hunting to keep myself busy.

Happy New Year and wish you a fruitful 2023!

LIttleAncientForestKami commented 1 year ago

Re: WireMock. What you wrote, adjust to readme and that's it. Hope this helps and all the best to you in your search.