求教牛人两段代码为什么不合理?

Question

   近日完成了一个项目,然后把代码交给总公司进行code review时,总公司同事指出下面两段代码存在问题,并给出了修改意见,是用英文描述的,小弟基本上能看懂其意思,但不知如何来修改,特地请教各位大虾.    第一段代码:
         public Category getCategory() {
return category;
}
public Category getCategoryRank() {
return categoryRank;
}
      同事给出的Error Description:
       Directly return the object reference of a mutable object from getter method may violates the loose coupling concept and casue thread interference problem unless it is well thought and written in documents.         Suggestion:
            Return a duplicated object
    第二段代码:      public synchronized boolean add() {
  boolean result = false;

try {
               .........
             }catch(Exception ){.....
            }finally{
             .... 
             }
          return result
              }        同事给出的Error Description:
           Where 2 users add a new category at the same time, the 2 new    categories can be with the same name which is not desired.
       
             Suggestion:
          Intrinsic locks of class but not object.

Answer 1

第二个是把方法改为static的 第一个好像是说返回的对象的clone，
英文不好不知道说的对不对

Answer 2

      第二个是把方法不能改为static,因为在add()方法里面引用了非静态的对象,会出现"cannot make static reference to the non-static field " 错误.

Answer 3

category变量是什么类型啊？是指针还是变量实例？

Answer 4

第二个控制synchronized 粒度在细些！

Answer 5

在第二个类里成名个static 类型的属性，用这个属性做锁旗标，用来锁类（个人理解）

Answer 6

private static String flag="sword";public boolean add() {     synchronized (flag){
            boolean result = false;

  try {
               .........
             }catch(Exception ){.....
            }finally{
             .... 
             }
               return result;
           }
}

Answer 7

import java.lang.Runnable;public class TestFrame{
public static void main(String[] args){
synchronized (name){
    TestRunnable test1 = new TestRunnable();
    TestRunnable test2 = new TestRunnable();
    Thread t1 = new Thread(test1);
    t1.start();
    TestRunnable.b = false;
    Thread t2 = new Thread(test2);
    t2.start();
}

}
}class TestRunnable implements Runnable{
public static boolean b = true;
public static String str = new String("1234");
public void run(){
if(b){
while(true){
this.show1();
    }
}else{
while(true){
this.show2();
    }
}


}

public void show1(){
synchronized (str){
System.out.println("in test runnable"+Thread.currentThread().getName());
try{
Thread.sleep(10000);
}catch(InterruptedException msg){
}

System.out.println("out test runnable"+Thread.currentThread().getName());
}

}

public void show2(){
synchronized (str){
System.out.println("in test runnable"+Thread.currentThread().getName());
try{
Thread.sleep(1000);
}catch(InterruptedException msg){
}

System.out.println("out test runnable"+Thread.currentThread().getName());
}
}
}

Answer 8

category是变量实例.
谢谢各位的解答.第一段该如何修改.

Answer 9

public Category getCategoryRank() {
return categoryRank;
}返回类型 确定是正确的？

Answer 10

哦,忘记写了,在代码开头有定义:           private Category categoryRank;

  private Category category;
               

Answer 11

首先，他的这个建议要看你这个JavaBean，还有这个Category的具体情况，并不适用于所有情况。他的意思是说，由于你直接返回了当前这个JavaBean的某个property/field的实例本身，而不是它的一个clone，导致了某个类似调用getCategory().setX(1000);的改变category/categoryRank的内部属性的操作，会使得整个系统错误。

Answer 12

public Category getCategory() {
return category;
}
public Category getCategoryRank() {
return category;
}
第二個：
利用內部排他鎖處理就好了，不明白就直接問總公司的人

Answer 13

1、java中的对象都是引用传递的，所以直接return category;这样返回的话，其他
Category a = getCategory() ;
a.setXXX(b);
这样的话，category的内容也会做相应的修改，他们的建议是你做好能够将category对象里面的内容“复制”一份给局部变量，然后返回局部变量；2、该段代码的同步范围太“大”，只需要在你具体add的地方，进行同步就足够了。

Answer 14

the loose coupling concept

Answer 15

对第一个的回答，能用clone就用clone，不能就再建个Category(Category category) method，return category改为return new Category(category),另一个照做。

Answer 16

clone 就占用两分内存了,而且colne本身也消耗一定的时间(计算密集的时候可能)
,如果必要的话改成友好,或者保护,另外在Category设置为不可变更,如Integer

Answer 17

谢谢各位了,问题解决,采用的是kingdom_seu的建议.
在第二段代码中进行了如下修改:  public  boolean add() {
      boolean result = false;
   synchronized(adminCategory.class)
     {
try {
               .........
             }catch(Exception ){.....
            }finally{
             .... 
             }
       }
          return result
              }