greengofar / spring-boot-pet-project

0 stars 0 forks source link

possible codebase improvements #1

Open spiashko opened 1 year ago

spiashko commented 1 year ago
  1. I would remove the common module Having it doesn't make sense right now and may only confuse.
  2. Liquibase change log files should contain base description like: db.changelog-1.0.sql -> db.changelog-1.0-base-structure.sql db.changelog-2.0.sq -> db.changelog-2.0-add-image-column.sql ... Having those base descriptions really helps in big codebase navigation during debugging Also, I would get rid of x.0 versioning as seems .0 is not used and would use just db.changelog-1-base-structure.sql
  3. https://github.com/greengofar/spring-boot-pet-project/blob/f221da2c71f770eb81fb56fa17234374566864f7/service/src/main/resources/application.yml#L31-L32 configs should not be bonded to specific local env, relative paths should be used, for example if I try to run it on my env it will fail to resolve those paths So something like this should work for everyone
    app.user.image.bucket: images/avatar
    app.product.image.bucket: images/product
  4. Some docker-compose with Postgres container should be added for faster bootstrap of dev env, so to check your project will be enough to just docker-compose up and mvn spring-boot:run
  5. https://github.com/greengofar/spring-boot-pet-project/blob/f221da2c71f770eb81fb56fa17234374566864f7/service/src/test/java/com/andev/integration/service/ProductServiceIT.java#L43-L51 you need to fix tests))))
  6. https://github.com/greengofar/spring-boot-pet-project/blob/f221da2c71f770eb81fb56fa17234374566864f7/service/src/main/java/com/andev/service/ProductService.java#L92-L92 why saveAndFlush ?
  7. https://github.com/greengofar/spring-boot-pet-project/blob/f221da2c71f770eb81fb56fa17234374566864f7/service/src/main/java/com/andev/service/ProductService.java#L101-L101 and why flush here?
  8. https://github.com/greengofar/spring-boot-pet-project/blob/f221da2c71f770eb81fb56fa17234374566864f7/service/src/main/java/com/andev/mapper/ProductReadMapper.java#L13-L13 instead of hand written mappers you may use tools like mapstruct
  9. https://github.com/greengofar/spring-boot-pet-project/blob/f221da2c71f770eb81fb56fa17234374566864f7/service/src/main/java/com/andev/service/ProductService.java#L102-L102 it doesn't make sense to return bool here as if entity was not found/deleted EmptyResultDataAccessException will be thrown
  10. https://github.com/greengofar/spring-boot-pet-project/blob/f221da2c71f770eb81fb56fa17234374566864f7/service/src/main/java/com/andev/validation/group/UpdateAction.java#L3-L3 not used code
  11. add some unit tests, UserInfoValidator should be good to start
spiashko commented 11 months ago
  1. generally I would advise using Postgres volume's local path relative to the current project in docker-compose https://github.com/greengofar/spring-boot-pet-project/blob/8e3c224dce9b43840e5af424e5a7a58b04cf685a/docker-compose.yaml#L9-L9 I would change to something like ./infra/postgres and add ./infra to .gitignore because if we have a project with lots of repos then your current config will override data for each project which will be inconvinient
spiashko commented 11 months ago
  1. mvn spring-boot:run didn't work because I missed that you actually didn't include spring boot plugin in build your current build configuration can be but the problem is that in the end, it doesn't produce the executable jar (if go to target folder and try java -jar service-1.0-SNAPSHOT.jar it will respond with no main manifest attribute, in service-1.0-SNAPSHOT.jar), the solution will be define manifest and also you will be required to create "fat" jar to include all dependencies into final jar or you can refactor to use spring boot plugin which already includes plugin configured to create manifest with main class and plugin configured to create "fat" jar
spiashko commented 11 months ago
  1. https://github.com/greengofar/spring-boot-pet-project/blob/8e3c224dce9b43840e5af424e5a7a58b04cf685a/service/src/main/resources/application.yml#L6-L6 your docker-compose defines local port as 5433 but configs refers to 5432 same about username/password
spiashko commented 11 months ago
  1. image

    UX is not quite well, through code I understood that the username must be email but it is not intuitive

spiashko commented 11 months ago
  1. I would advise adding some data in dev profile, so once I run the application in the dev profile I already have some predefined users and predefined products, and it will be easier to "try" application