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

A mostly tooling review from reddit #3

Open
5 of 7 tasks
LIttleAncientForestKami opened this issue Jan 2, 2023 · 2 comments
Open
5 of 7 tasks

A mostly tooling review from reddit #3

LIttleAncientForestKami opened this issue Jan 2, 2023 · 2 comments

Comments

@LIttleAncientForestKami
Copy link

LIttleAncientForestKami commented Jan 2, 2023

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:
  • Java version set there
  • Maven compiler source, target (and perhaps release if newer Java is used) set to said Java version (once and for all)
  • UTF-8 (or other) encoding set, so the build is not platform dependent, yours right now is for the encoding is left for the platform to set
  • versions of dependencies grouped together - this is mightily handy for maintaining and updating versions, for later extracting things to BOM if you will need it etc. Also good for security checks, bumping versions automatically and other things like that.
  • Maven plug-ins lock-in by specifying the versions
  • perhaps a main class name or manifest attributes, if you use them
  • build > finalName to have a nice name for your final JAR. Unless you like what Maven is doing here.

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
Copy link
Owner

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
  1. 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
Copy link
Author

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

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

No branches or pull requests

2 participants