implment a userfactory, Iuser interface, 1.0 #19
Conversation
|
|
||
| return user; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
An error raise could be more appropriate here..?
There was a problem hiding this comment.
That is correct, but as I have mentioned earlier, if we will stick to the factory design pattern, we shouldn't have too much places to raise an error in the UserFactory class.
There was a problem hiding this comment.
General review
First of all, nice start and attempt to create a pull request! I think you are trying to contribute, but it might be better if we can settle a general direction before we do anything that implements new features!
Also, note that the pull request did not follow our naming convention, please use the following prefixes in both the Pull Requests and the commit messages:
[+], to denote any NEW features that are added
[*], to denote any MODIFIED features
[-], to denote any REMOVED features and/or files
Also, try to provide more details when you are submitting a pull request, try to mention what you're trying to achieve so that other members can easily follow along. The more description, the better, as the other team members can also try to understand and help.
Detailed comments
- Convention issues
- Not clear direction of modification
- No purpose seen on IUser.java type
- Not clearly following the factory method (Or maybe I am misinterpretting here)
- Useless comment
- Redundant code (Through copy pasting)
@ScottCTD, decide on whether we should close this pull request or not.
I personally prefer discussing on the GUI implmentation details first, so that we can write the use case based from the GUI. 👍
| package billgates.entities; | ||
|
|
||
| public class UserFactory { | ||
| //use getShape method to get object of type shape |
There was a problem hiding this comment.
I'm not sure what this "getShape" is supposed to mean in the comment here
| @@ -0,0 +1,5 @@ | |||
| package billgates.entities; | |||
|
|
|||
| public interface IUser { | |||
There was a problem hiding this comment.
Hmm, what does this Interface user contain?
Usually, an interface should probably have methods that should be overridden by the classes that tries to implment it, in this case, User.java. Maybe try declaring some methods for User to implement?
|
|
||
| public class UserFactory { | ||
| //use getShape method to get object of type shape | ||
| public IUser getUser(int id, String name, String UserType) { |
There was a problem hiding this comment.
I believe the factory method doesn't work like this?
We should have a way to store the information, then call multiple generation methods to return the User types we are interested in.
E.g.
private int id = -1;
private String name = "";
private String password = "";
private int billId = -1;
public void setId(int id) { this.id = id; }
public void setName(String name) { this.name = name; }
public void setPassword(String password) { this.password = password; }
public DummyUser generateDummyUser() { return DummyUser(id, name); }
public User generateUser() { return User(id, name, password, billId); }
|
|
||
| return user; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
That is correct, but as I have mentioned earlier, if we will stick to the factory design pattern, we shouldn't have too much places to raise an error in the UserFactory class.
| return null; | ||
| } | ||
|
|
||
| public IUser getUser(int id, String name,String password,int billID,String UserType) { |
There was a problem hiding this comment.
Hmm, this looks pretty much like a "Code smell" that we are creating redundant lines by copy pasting from the above, maybe my suggestion could adhere to the factory method better.
…se those instance to generate User/dummyuser in getuser function [*]Modify Iuser, so this interface include abstract functions supposed to return instance of user and dummyuser,(may add more function later) [+]Add constructor for Userfactory [+]Add quickGetUser in Userfactory, using default value stored to create desired type User [*]cleaning up some Redundant code in UserFactory
|
[*]modify UserFactory so it can set or store temporary instance and use those instance to generate User/dummyuser in getuser function [*]Modify Iuser, so this interface include abstract functions supposed to return instance of user and dummyuser,(may add more function later) [+]Add constructor for Userfactory [+]Add quickGetUser in Userfactory, using default value stored to create desired type User [*]cleaning up some Redundant code in UserFactory ps. this is modified from a history version of your branch, I am not sure if this will cover your changes, sorry for the inconvenient and this uncertainty. |
userfactory entity and add a shared interface of user and dummyuser