Gangmax Blog

Qi2Pa1 Code(1)

Qi2Pa1 = 奇葩

Here is the code.

AccountRepository.java
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
public class AccountRepository {
    private List<PjpRoleMapping> roleMappings = new ArrayList<PjpRoleMapping>();

    ...

    public void addRoleMapping(PjpRoleMapping roleMapping) throws Exception {
        for (PjpRoleMapping pjpRoleMapping : roleMappings) {
            if (!roleMapping.pjpUser.getUser().equals(pjpRoleMapping.pjpUser.getUser())) {
                continue;
            }

            if (!roleMapping.pjpRole.getRoleName().equals(pjpRoleMapping.pjpRole.getRoleName())) {
                continue;
            }

            if (roleMapping.pjpTenant==null) {
                if (pjpRoleMapping.pjpTenant == null) {
                    return;
                } else {
                    continue;
                }
            } else {
                if (pjpRoleMapping.pjpTenant == null) {
                    continue;
                } else if (!roleMapping.pjpTenant.getId().equals(pjpRoleMapping.pjpTenant.getId())) {
                    continue;
                }
            }


            if (roleMapping.pjpProject==null) {
                if (pjpRoleMapping.pjpProject == null) {
                    return;
                } else {
                    continue;
                }
            } else {
                if (pjpRoleMapping.pjpProject == null) {
                    continue;
                } else if (!roleMapping.pjpProject.id.equals(pjpRoleMapping.pjpProject.id)) {
                    continue;
                } else {
                    return;
                }
            }

        }
        roleMappings.add(roleMapping);
    }
}
PjpRoleMapping.java
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
public class PjpRoleMapping {

    public PjpUser pjpUser;
    public PjpRole pjpRole;
    public PjpTenant pjpTenant;
    public PjpSecurityProject pjpProject;

    public PjpRoleMapping(PjpUser pjpUser,  PjpRole role, PjpTenant pjpTenant) {
        this.pjpUser = pjpUser;
        this.pjpRole = role;
        this.pjpTenant = pjpTenant;
    }

    public PjpRoleMapping(PjpUser pjpUser,  PjpRole role, PjpTenant pjpTenant, PjpSecurityProject pjpProject) {
        this.pjpUser = pjpUser;
        this.pjpRole = role;
        this.pjpTenant = pjpTenant;
        this.pjpProject = pjpProject;
    }

    public String toString() {
        StringBuffer sb = new StringBuffer();

        sb.append("PjpRoleMapping[");
        if (pjpUser != null) {
            sb.append("user=" + pjpUser.getUser());
        }

        if (pjpRole != null) {
            sb.append(",role=" + pjpRole.getRoleName());
        }

        if (pjpTenant != null) {
            sb.append(", tenantName=" + pjpTenant.getName() + ", tenantId=" + pjpTenant.getId());
        }

        if (pjpProject != null) {
            sb.append(", projectName=" + pjpProject.name + ", projectId=" + pjpProject.id);
        }
        sb.append("]");
        return sb.toString();
    }
}

It took me quite a while to understand “AccountRepository.java line 7-47”, how much time do you get it? ;)

The code wants to make sure no duplicate elements would be added into “List<PjpRoleMapping> roleMapping”. When a qualified Java programmer gets the purpose, he/she would know that the code was implemented in a wrong way.

Two simple facts:

  1. If you don’t want duplicate elements in a “Collection“, use “Set“;

  2. If you want to compare 2 instances of a class, implement your own “equals()/hashCode()“ methods.

So, the right code should be:

AccountRepository.java
1
2
3
4
5
6
public class AccountRepository {
    private Set<PjpRoleMapping> roleMappings = new HashSet<PjpRoleMapping>();
    public void addRoleMapping(PjpRoleMapping roleMapping) throws Exception {
        roleMappings.add(roleMapping);
    }
}
PjpRoleMapping.java
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
public class PjpRoleMapping {

    public PjpUser pjpUser;
    public PjpRole pjpRole;
    public PjpTenant pjpTenant;
    public PjpSecurityProject pjpProject;

    public PjpRoleMapping(PjpUser pjpUser,  PjpRole role, PjpTenant pjpTenant) {
        this.pjpUser = pjpUser;
        this.pjpRole = role;
        this.pjpTenant = pjpTenant;
    }

    public PjpRoleMapping(PjpUser pjpUser,  PjpRole role, PjpTenant pjpTenant, PjpSecurityProject pjpProject) {
        this.pjpUser = pjpUser;
        this.pjpRole = role;
        this.pjpTenant = pjpTenant;
        this.pjpProject = pjpProject;
    }

    @Override
    public int hashCode() {
        final int prime = 31;
        int result = 1;
        result = prime * result
                + ((pjpProject == null) ? 0 : pjpProject.hashCode());
        result = prime * result + ((pjpRole == null) ? 0 : pjpRole.hashCode());
        result = prime * result
                + ((pjpTenant == null) ? 0 : pjpTenant.hashCode());
        result = prime * result + ((pjpUser == null) ? 0 : pjpUser.hashCode());
        return result;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (getClass() != obj.getClass())
            return false;
        PjpRoleMapping other = (PjpRoleMapping) obj;
        if (pjpProject == null) {
            if (other.pjpProject != null)
                return false;
        } else if (!pjpProject.equals(other.pjpProject))
            return false;
        if (pjpRole == null) {
            if (other.pjpRole != null)
                return false;
        } else if (!pjpRole.equals(other.pjpRole))
            return false;
        if (pjpTenant == null) {
            if (other.pjpTenant != null)
                return false;
        } else if (!pjpTenant.equals(other.pjpTenant))
            return false;
        if (pjpUser == null) {
            if (other.pjpUser != null)
                return false;
        } else if (!pjpUser.equals(other.pjpUser))
            return false;
        return true;
    }

    @Override
    public String toString() {
        return "PjpRoleMapping [pjpUser=" + pjpUser + ", pjpRole=" + pjpRole
                + ", pjpTenant=" + pjpTenant + ", pjpProject=" + pjpProject
                + "]";
    }
}

The total code lines of the 2 implementation may be similar, be I think the latter is much better than the former because:

  1. Much easier to read and understand since it is “intuitive” to understand for any qualified Java programmer;

  2. The “toString()/equals()/hashCode()” methods are generated by IDE, it took me less than 10 seconds.

It would be a pathetic shame that a 10+ years experience Java programmer could not make it right.

Comments