search

Thotogelo / URLShortner_Task

0 stars 0 forks source link

Quick review #1

Open sduduzog opened 3 months ago

sduduzog commented 3 months ago

TLDR; There's still a lot of things missing but I get that this is a url shortner service which for now only shortens urls.

  1. The only implemented feature does not have tests written for it. Part of learning how to build apps is learning various ways of testing them, it doesn't have to be perfect but you just have to be prepared.

  2. This check could have been a seperate method https://github.com/Thotogelo/URLShortner_Task/blob/d55c04dbabbf0e4d4b9f95faa41a726d6ba555b8/src/main/java/org/example/urlshortner_task/service/UrlService.java#L25

  3. One of these is not a longUrl . Can a longUrl have a longUrl inside of it? https://github.com/Thotogelo/URLShortner_Task/blob/d55c04dbabbf0e4d4b9f95faa41a726d6ba555b8/src/main/java/org/example/urlshortner_task/controller/UrlController.java#L23

  4. Encoding a url means something else, you have to be direct with naming. If its shortening a url, then it must be named along those lines i.e. shortenUrl https://github.com/Thotogelo/URLShortner_Task/blob/d55c04dbabbf0e4d4b9f95faa41a726d6ba555b8/src/main/java/org/example/urlshortner_task/service/UrlService.java#L22

  5. Nitpicking, on your commit messages not being clear

  6. The spec is not yet fulfilled, is there an endpoint to use when visiting the shorturl? as that is where you can track clicks, and not by a boolean but a counter

Thotogelo commented 3 months ago
  1. I will work on them, and we'll discuss them. A bit of a skill issue when it comes to testing. https://github.com/Thotogelo/URLShortner_Task/blob/4d52841b666e3f9e94075672f2a3efc430abe3f3/src/test/java/org/example/urlshortner_task/UrlControllerTest.java#L45
  2. Please elaborate on why you believe it needs to be separate. https://github.com/Thotogelo/URLShortner_Task/blob/4d52841b666e3f9e94075672f2a3efc430abe3f3/src/main/java/org/example/urlshortner_task/service/UrlService.java#L26
  3. I understand, I changed it to a record called requestUrl with a member called url. The endpoint accepts both short and long urls, so I need a generic name to accommodate both. https://github.com/Thotogelo/URLShortner_Task/blob/4d52841b666e3f9e94075672f2a3efc430abe3f3/src/main/java/org/example/urlshortner_task/controller/UrlController.java#L25
  4. Changed the method name to resolveOrShortenUrl. Either one will happen depending on the input. https://github.com/Thotogelo/URLShortner_Task/blob/4d52841b666e3f9e94075672f2a3efc430abe3f3/src/main/java/org/example/urlshortner_task/service/UrlService.java#L23
  5. Understood.
  6. There is a single endpoint, functionality depends on input. I moved from a boolean to a counter.https://github.com/Thotogelo/URLShortner_Task/blob/4d52841b666e3f9e94075672f2a3efc430abe3f3/src/main/java/org/example/urlshortner_task/entity/UrlEntity.java#L25