Quyết định thiết kế tồi tệ để ném ngoại lệ từ một người truy cập?

Question

Tôi đã đọc một số câu trả lời lại các pro và con của ném một ngoại lệ trong một accessor, nhưng tôi nghĩ tôi sẽ đề cập câu hỏi cụ thể của tôi với một ví dụ:

public class App { 
    static class Test {  
    private List<String> strings; 

    public Test() { 
    } 

    public List<String> getStrings() throws Exception { 
     if (this.strings == null) 
      throw new Exception(); 

     return strings; 
    } 

    public void setStrings(List<String> strings) { 
     this.strings = strings; 
    } 
} 

public static void main(String[] args) { 
    Test t = new Test(); 

    List<String> s = null; 
    try { 
     s = t.getStrings(); 
    } catch (Exception e) { 
     // TODO: do something more specific 
    } 
    } 
} 

getStrings() là ném một Exception khi strings vẫn chưa bộ. Liệu tình huống này sẽ được xử lý tốt hơn bằng một phương pháp?

Answer 1

Vâng, điều đó thật tệ. Tôi đề nghị khởi tạo các biến chuỗi trong việc kê khai, như:

private List<String> strings = new ArrayList<String>(); 

và bạn có thể thay thế các setter với một cái gì đó giống như

public void addToList(String s) { 
    strings.add(s); 
} 

Vì vậy, không có khả năng của một NPE.

(Hoặc không khởi tạo chuỗi biến trong việc kê khai, thêm khởi với phương pháp addToList, và có mã sử dụng nó kiểm tra getStrings() để xem nếu nó được vô lại.)

Đối với tại sao nó xấu, thật khó để nói với một ví dụ giả tạo như vậy, nhưng có vẻ như có quá nhiều thay đổi trạng thái và bạn đang phạt người dùng của lớp này cho nó bằng cách làm cho người dùng xử lý ngoại lệ. Ngoài ra còn có vấn đề là một khi các phương thức trong chương trình của bạn bắt đầu ném ngoại lệ thì nó có xu hướng di căn trên tất cả codebase của bạn.

Kiểm tra xem thành viên thể hiện này có cần được thực hiện ở đâu đó không, nhưng tôi nghĩ nó nên được thực hiện ở đâu đó có nhiều kiến ​​thức về những gì đang diễn ra hơn trong lớp này.

Answer 2

Tôi khuyên bạn nên ném ngoại lệ từ phương thức setter. Có lý do đặc biệt nào tại sao không tốt hơn để làm như vậy?

public List<String> getStrings() throws Exception { 
    return strings; 
} 

public void setStrings(List<String> strings) { 
    if (strings == null) 
     throw new Exception(); 

    this.strings = strings; 
} 

Ngoài ra, tùy theo ngữ cảnh cụ thể, sẽ không thể đơn giản chuyển trực tiếp công khai List<String> strings bằng hàm tạo? Bằng cách đó có lẽ bạn thậm chí sẽ không cần một phương pháp setter.

Answer 3

Nó thực sự phụ thuộc vào những gì đang làm với danh sách bạn nhận được. Tôi nghĩ rằng một giải pháp thanh lịch hơn sẽ là trả lại một Colletions.EMPTY_LIST hoặc, như @Nathan gợi ý, một số trống rỗng ArrayList. Sau đó, mã sử dụng danh sách có thể kiểm tra xem nó có trống không nếu nó muốn, hoặc chỉ làm việc với nó giống như với bất kỳ danh sách nào khác.

Sự khác biệt là sự khác biệt giữa không có chuỗi và không có danh sách các chuỗi. Việc đầu tiên phải được đại diện bởi một danh sách trống, thứ hai bằng cách trở về null hoặc ném một ngoại lệ.

Answer 4

Các cách chính xác để xử lý việc này, giả sử nó là một yêu cầu rằng danh sách này được thiết lập ở những nơi khác trước gọi getter được quy định như sau:

public List<String> getStrings() { 
    if (strings == null) 
     throw new IllegalStateException("strings has not been initialized"); 

    return strings; 
} 

Là một kiểm soát ngoại lệ, nó isn' t yêu cầu bạn khai báo nó trên phương thức, vì vậy bạn không gây ô nhiễm mã máy khách với rất nhiều tiếng ồn try/catch.Ngoài ra, vì điều kiện này có thể tránh được (ví dụ mã khách hàng có thể đảm bảo danh sách đã được khởi tạo), đây là lỗi lập trình và do đó ngoại lệ không được chọn là chấp nhận được và thích hợp hơn.

Answer 5

Tôi sẽ tham khảo thông số Java Beans về điều này. Có lẽ tốt hơn là không nên ném một java.lang.Exception mà thay vào đó hãy sử dụng java.beans.PropertyVetoException và đăng ký bean của bạn như là một java.beans.VetoableChangeListener và kích hoạt sự kiện thích hợp. Đó là một quá mức cần thiết nhưng sẽ xác định chính xác những gì bạn đang cố gắng làm.

Answer 6

Nó thực sự phụ thuộc vào những gì bạn đang cố gắng làm. Nếu bạn đã cố gắng thực thi điều kiện mà thiết lập đã được gọi trước khi getter được gọi, có thể có một trường hợp để ném một IllegalStateException.

Answer 7

Nói chung, đây là mùi thiết kế.

Nếu nó thực sự là một lỗi mà chuỗi không được khởi tạo, sau đó loại bỏ setter, và thực thi ràng buộc trong một constructor, hoặc làm như Bohemian nói và ném một IllegalArgumentException với một thông điệp giải thích. Nếu bạn làm điều cũ, bạn sẽ có hành vi thất bại nhanh.

Nếu không phải là lỗi là chuỗi rỗng, thì hãy cân nhắc việc khởi tạo danh sách thành danh sách trống và thực thi nó không phải là rỗng trong trình thiết lập, tương tự như trên. Điều này có lợi thế là bạn không cần phải kiểm tra null trong bất kỳ mã máy khách nào.

for (String s: t.getStrings()) { 
    // no need to check for null 
} 

Điều này có thể đơn giản hóa mã máy khách rất nhiều.

EDIT: Ok, tôi vừa thấy rằng bạn đang tuần tự hóa từ JSON, vì vậy bạn không thể xóa hàm tạo. Vì vậy, bạn sẽ cần một trong hai:

1. ném một IllegalArgumentException từ getter
2. nếu chuỗi là null, trả lại một Collections.EMPTY_LIST.
3. hoặc tốt hơn: thêm kiểm tra xác thực ngay sau khi deserialization, nơi bạn đặc biệt kiểm tra giá trị của chuỗi, và ném một ngoại lệ hợp lý.

Answer 8

Tôi nghĩ bạn đã tiếp xúc với một câu hỏi quan trọng hơn sau câu hỏi gốc: bạn nên làm việc để ngăn chặn các trường hợp ngoại lệ trong getters và setters?

Câu trả lời là có, bạn nên làm việc để tránh các ngoại lệ nhỏ.

Những gì bạn có ở đây là một cách hiệu quả lazy instantiation mà không phải là ở tất cả các động cơ trong ví dụ này:

Trong lập trình máy tính, khởi tạo lười biếng là chiến thuật trì hoãn việc tạo ra một đối tượng, tính toán của một giá trị hoặc một số quá trình khác tốn kém cho đến lần đầu tiên cần thiết.

Bạn có hai vấn đề trong ví dụ này:

1. dụ của bạn không phải là thread an toàn. Việc kiểm tra null đó có thể thành công (tức là, tìm ra rằng đối tượng là null) trên hai luồng cùng một lúc. Sự khởi tạo của bạn sau đó tạo ra hai danh sách chuỗi khác nhau. Hành vi không xác định sẽ xảy ra.

2. Không có lý do chính đáng trong ví dụ này để trì hoãn việc khởi tạo. Nó không phải là một hoạt động đắt tiền hoặc phức tạp. Đây là những gì tôi có nghĩa là "làm việc để tránh ngoại lệ tầm thường": nó là giá trị đầu tư chu kỳ để tạo ra một hữu ích (nhưng trống) danh sách để đảm bảo rằng bạn không ném chậm detonation null con trỏ ngoại lệ xung quanh.

Hãy nhớ rằng, khi bạn gây ra một ngoại lệ trên mã bên ngoài, bạn về cơ bản hy vọng các nhà phát triển biết làm thế nào để làm một cái gì đó hợp lý với nó. Bạn cũng đang hy vọng rằng không có một nhà phát triển thứ ba trong phương trình có bọc tất cả mọi thứ trong một eater ngoại lệ, chỉ để nắm bắt và bỏ qua ngoại lệ như của bạn:

try { 
    // I don't understand why this throws an exception. Ignore. 
    t.getStrings(); 
} catch (Exception e) { 
    // Ignore and hope things are fine. 
} 

Trong ví dụ dưới đây, tôi đang sử dụng Null Object pattern để cho biết mã tương lai mà ví dụ của bạn chưa được đặt. Tuy nhiên, đối tượng Null chạy như mã không đặc biệt và, do đó, không có chi phí và tác động đến luồng công việc của các nhà phát triển trong tương lai.

public class App { 
    static class Test {    
     // Empty list 
     public static final List<String> NULL_OBJECT = new ArrayList<String>(); 

     private List<String> strings = NULL_OBJECT; 

     public synchronized List<String> getStrings() { 
      return strings; 
     }  

     public synchronized void setStrings(List<String> strings) {   
      this.strings = strings;  
     } 
    } 

    public static void main(String[] args) {  
     Test t = new Test();  

     List<String> s = t.getStrings(); 

     if (s == Test.NULL_OBJECT) { 
      // Do whatever is appropriate without worrying about exception 
      // handling. 

      // A possible example below: 
      s = new ArrayList<String>(); 
      t.setStrings(s); 
     } 

     // At this point, s is a useful reference and 
     // t is in a consistent state. 
    } 
}