Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Usability

#Usability InIn your github mainclass, you're asking the user:

System.out.println("Enter command type");

Then expecting them to enter a number. Consider adding some kind of menu so that people that haven't used the application before know what number 3 does before selecting it and getting the message 'UnRegistered user wants to enter a website'.

Usage

#Usage YouYou haven't specified how you're expecting your URL shortener to be used. To me, the current setup doesn't really make sense. The reason I shorten a URL is so that I can share it with other people, so I'm expecting two main uses:

  1. Create short URL from real URL, as a registered/unregistered user
  2. Get real URL from short URL

The way you've coded it, it looks like as a registered user, only I can access my shortened URL mappings. This seems flawed in a practical usage sense.

Time To Live

#Time To Live You'veYou've got two public methods on the class

public void addNewEntryForUser(String url,String userId) {
public void addNewEntryForUserWithTTL(String url,String userId,int timeToLive) {

The first simply calls the second passing in the default TTL. According to your requirements, only registered users should be able to set a TTL, however the class supports anybody setting a TTL, as long as they call the method with the time to live parameter. This might be OK, but it really depends on who owns the logic to use a default TTL for certain users. At the moment that's not clear. I'd also consider overloading these methods as addNewEntryForUser, I don't think having the WithTTL in the method name really gives you anything over the extra parameter.

Time to Die

#Time to Die ThereThere doesn't appear to be any expiry logic - Not really true, but it's not obvious. Rather than calling the method isValid, something like isExpired, isTTLExpired would make it more obvious that the expiration is triggered by this method.

Responsibilities

#Responsibilities ItIt feels like your shortener is doing too much. It could just shorten URLs. However, it's also responsible for storing a list of registered users, the list of all shortened URLs by user, setting TTL. At a minimum, I'd extract the registered user list from the class and provide some way to get it via the classes constructor (either a repository, or even just a supplied list) to give some separation. Hard coded user Id's really shouldn't be in this class.

#Usability In your github mainclass, you're asking the user:

System.out.println("Enter command type");

Then expecting them to enter a number. Consider adding some kind of menu so that people that haven't used the application before know what number 3 does before selecting it and getting the message 'UnRegistered user wants to enter a website'.

#Usage You haven't specified how you're expecting your URL shortener to be used. To me, the current setup doesn't really make sense. The reason I shorten a URL is so that I can share it with other people, so I'm expecting two main uses:

  1. Create short URL from real URL, as a registered/unregistered user
  2. Get real URL from short URL

The way you've coded it, it looks like as a registered user, only I can access my shortened URL mappings. This seems flawed in a practical usage sense.

#Time To Live You've got two public methods on the class

public void addNewEntryForUser(String url,String userId) {
public void addNewEntryForUserWithTTL(String url,String userId,int timeToLive) {

The first simply calls the second passing in the default TTL. According to your requirements, only registered users should be able to set a TTL, however the class supports anybody setting a TTL, as long as they call the method with the time to live parameter. This might be OK, but it really depends on who owns the logic to use a default TTL for certain users. At the moment that's not clear. I'd also consider overloading these methods as addNewEntryForUser, I don't think having the WithTTL in the method name really gives you anything over the extra parameter.

#Time to Die There doesn't appear to be any expiry logic - Not really true, but it's not obvious. Rather than calling the method isValid, something like isExpired, isTTLExpired would make it more obvious that the expiration is triggered by this method.

#Responsibilities It feels like your shortener is doing too much. It could just shorten URLs. However, it's also responsible for storing a list of registered users, the list of all shortened URLs by user, setting TTL. At a minimum, I'd extract the registered user list from the class and provide some way to get it via the classes constructor (either a repository, or even just a supplied list) to give some separation. Hard coded user Id's really shouldn't be in this class.

Usability

In your github mainclass, you're asking the user:

System.out.println("Enter command type");

Then expecting them to enter a number. Consider adding some kind of menu so that people that haven't used the application before know what number 3 does before selecting it and getting the message 'UnRegistered user wants to enter a website'.

Usage

You haven't specified how you're expecting your URL shortener to be used. To me, the current setup doesn't really make sense. The reason I shorten a URL is so that I can share it with other people, so I'm expecting two main uses:

  1. Create short URL from real URL, as a registered/unregistered user
  2. Get real URL from short URL

The way you've coded it, it looks like as a registered user, only I can access my shortened URL mappings. This seems flawed in a practical usage sense.

Time To Live

You've got two public methods on the class

public void addNewEntryForUser(String url,String userId) {
public void addNewEntryForUserWithTTL(String url,String userId,int timeToLive) {

The first simply calls the second passing in the default TTL. According to your requirements, only registered users should be able to set a TTL, however the class supports anybody setting a TTL, as long as they call the method with the time to live parameter. This might be OK, but it really depends on who owns the logic to use a default TTL for certain users. At the moment that's not clear. I'd also consider overloading these methods as addNewEntryForUser, I don't think having the WithTTL in the method name really gives you anything over the extra parameter.

Time to Die

There doesn't appear to be any expiry logic - Not really true, but it's not obvious. Rather than calling the method isValid, something like isExpired, isTTLExpired would make it more obvious that the expiration is triggered by this method.

Responsibilities

It feels like your shortener is doing too much. It could just shorten URLs. However, it's also responsible for storing a list of registered users, the list of all shortened URLs by user, setting TTL. At a minimum, I'd extract the registered user list from the class and provide some way to get it via the classes constructor (either a repository, or even just a supplied list) to give some separation. Hard coded user Id's really shouldn't be in this class.

Source Link
forsvarir
  • 11.8k
  • 7
  • 39
  • 72

#Usability In your github mainclass, you're asking the user:

System.out.println("Enter command type");

Then expecting them to enter a number. Consider adding some kind of menu so that people that haven't used the application before know what number 3 does before selecting it and getting the message 'UnRegistered user wants to enter a website'.

#Usage You haven't specified how you're expecting your URL shortener to be used. To me, the current setup doesn't really make sense. The reason I shorten a URL is so that I can share it with other people, so I'm expecting two main uses:

  1. Create short URL from real URL, as a registered/unregistered user
  2. Get real URL from short URL

The way you've coded it, it looks like as a registered user, only I can access my shortened URL mappings. This seems flawed in a practical usage sense.

#Time To Live You've got two public methods on the class

public void addNewEntryForUser(String url,String userId) {
public void addNewEntryForUserWithTTL(String url,String userId,int timeToLive) {

The first simply calls the second passing in the default TTL. According to your requirements, only registered users should be able to set a TTL, however the class supports anybody setting a TTL, as long as they call the method with the time to live parameter. This might be OK, but it really depends on who owns the logic to use a default TTL for certain users. At the moment that's not clear. I'd also consider overloading these methods as addNewEntryForUser, I don't think having the WithTTL in the method name really gives you anything over the extra parameter.

#Time to Die There doesn't appear to be any expiry logic - Not really true, but it's not obvious. Rather than calling the method isValid, something like isExpired, isTTLExpired would make it more obvious that the expiration is triggered by this method.

#Responsibilities It feels like your shortener is doing too much. It could just shorten URLs. However, it's also responsible for storing a list of registered users, the list of all shortened URLs by user, setting TTL. At a minimum, I'd extract the registered user list from the class and provide some way to get it via the classes constructor (either a repository, or even just a supplied list) to give some separation. Hard coded user Id's really shouldn't be in this class.

lang-java

AltStyle によって変換されたページ (->オリジナル) /