6
\$\begingroup\$

Problem Statement:

Design an employee swap in swap out system. The system will have a machine which records the swap in and swap out. The user can also login in a portal where he can check his swap in /swap out time. He can correct his time also.

There will be managers for employee who can check the entry for all the employees which are under them and correct their subordinates timings also.

Similarly, a HR person can only view every Employee's records.

I chalked out the following skeleton. I am new to design and would like any pointers on how to approach such problems.

class User{
    String name;
    String designation;
    int id;
}

class AccessGroup{
    int priority;
    String name;
    Boolean read;
    Boolean write;
    //Setter - Getter methods
}

static class AccessManagement{
    HashSet<String, PriorityQueue<AccessGroup> accessControlTable;

    public void addUser(User u, AccessGroup g);
    public void addUserToGroup(User u, AccessGroup g);
    public void removeUser(User u);
    public void removeUserFromGroup(User u, AccessGroup g);
    public AccessGroup getPriorityAccessGroupForUser(User u);
    public PriorityQueue<AccessGroup> getUserGroups(User u);
    public boolean isValidUser(User u);
    public boolean hasReadPermission(User u);
    public boolean hasWritePermission(User u);
}

class Mediator{
    String url="";

    public List<Records> getRecords(User u);
    public void updateRecordForUser(User u, Record r, Value v){
        if(!AccessManagement.isValidUser(u))
            throw Exception();

        if(AccessManagement.hasWritePermission(u)){
            try{
                Connection conn = DriverManager.getConnection(url);
                Statement statement = conn.createStatement();
                statement.executeQuery("Update r in TimeStamps where id= u.id");
            }
            catch(Exception e){
                //Do something
            }        
        }        
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ I believe you have written your own implementation for HashSet<T, E>. because the java HashSet does have only one type parameter. HashSet<T> \$\endgroup\$ – Jude Niroshan Jul 6 '17 at 6:10
  • \$\begingroup\$ Probably should be HashMap \$\endgroup\$ – Trent Bartlem Jul 6 '17 at 6:32
6
\$\begingroup\$
  • AccessGroup should contain a Set of permissions to be extensible, rather than have the Read permission and Write permission directly attached. Otherwise you end up with a dozen attributes hanging off AccessGroup in a year's time.
  • (this also means you can just union the permissions on a user to see what they can do, instead of messing with AccessGroup.getXXXX attributes)
  • You're overloading the terms 'add' and 'remove'. You should createUser and deleteUser
  • The User class is quite anemic. Perhaps you could use composition to hide the entire AccessManagement class inside it, maybe by having an AuthenticatedUser subclass that knows about its groups and permissions.
  • Your mediator should have specific methods to swapIn and swapOut. TBH I'd model it as an event stream so there would be no updates, only inserts.
\$\endgroup\$
3
\$\begingroup\$

Welcome to Code Review, and thanks for sharing your code! Altogether, your program looks pretty good. It's nice and short, you have good indentation, all your variables have reasonable names, and it's very easy to read. A couple things to fix and work on in the future:

1. Always use braces in if statements.

It looks nicer, prevents bugs, increases readability, and there is no down-side to it. So change this statement:

    if(!AccessManagement.isValidUser(u))
        throw Exception();

to this:

    if(!AccessManagement.isValidUser(u)) {
        throw Exception();
    }

2. Missing >.

    HashSet<String, PriorityQueue<AccessGroup>> accessControlTable;
                                             ^

3. Make use of the keyword throws.

Ahhhh yes... Back to this statement again. This could be removed altogether with a simple line in your header. Rather than this:

    if(!AccessManagement.isValidUser(u)) {
        throw Exception();
    }

Just do this:

    public void updateRecordForUser(User u, Record r, Value v) throws Exception {
        if(AccessManagement.hasWritePermission(u)){
            try{
               //...code down here

That's just about all I could think of. Keep up the good coding!

\$\endgroup\$
6
  • 2
    \$\begingroup\$ I don't understand where the last point comes from ... \$\endgroup\$ – Vogel612 Jul 6 '17 at 8:49
  • 2
    \$\begingroup\$ All good points except for the last one. Stating "throws Exception" for a method I think is poor practice, as you should never really be throwing a raw 'Exception', rather a sub-class of an exception that describes what the problem actually is. I would recommend something like UserNotValidException. \$\endgroup\$ – Mike Baxter Jul 6 '17 at 12:56
  • \$\begingroup\$ @Vogel612 it just shortens code, while producing the same effect. \$\endgroup\$ – CodingNinja Jul 7 '17 at 0:39
  • \$\begingroup\$ @Teifi "Stating "throws Exception" for a method I think is poor practice..." do you have some evidence to back that up? it sounds more like an opinion to me, rather than a fact. \$\endgroup\$ – CodingNinja Jul 7 '17 at 0:41
  • 1
    \$\begingroup\$ @Tommy it's very poor practice, because it means the calling code has no specific information as to which exceptions are thrown. That's a problem, because specialized Exception types do contain additional information, which can't be recovered without guessing their type, making the whole exercise a problem... \$\endgroup\$ – Vogel612 Jul 7 '17 at 8:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.