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:
- Create short URL from real URL, as a registered/unregistered user
- 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:
- Create short URL from real URL, as a registered/unregistered user
- 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:
- Create short URL from real URL, as a registered/unregistered user
- 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:
- Create short URL from real URL, as a registered/unregistered user
- 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.