hadv / Hallelujah

Conway's Game Of Life
0 stars 0 forks source link

Cải tiến lần 5 #4

Closed ngocdaothanh closed 9 years ago

ngocdaothanh commented 9 years ago

https://github.com/hadv/ConwayGoL/blob/bee65bd69cbfe18eebcb2a886a55a56c8877da31/src/com/company/ConwayGOL.java

Nhận xét code

Người pv chưa cần soi kĩ, chỉ cần nhìn thoáng thấy code anh viết thấy có 2 vòng lặp lồng vào nhau thế này là cho anh điểm trừ luôn rồi, vì code này k phản ánh được tinh thần là "sparse" mà cách làm dùng Map hướng đến:

for (int i = verticalTop; i <= verticalBottom; i++) {
    for (int j = horizontalLeft; j <= horizontalRight; j++) {

Tại vì nó vẫn cố vét cạn hết các cell nằm trong hình chữ nhật => O(N^2).

Anh hãy cải tiến, chỉ loop theo các cell nằm trong tập hợp currentGeneration => O(N).

Ngoài ra cách làm với BitSet thì anh dùng integer làm index 1 chiều. Cách làm với Map này anh lại chế thêm cái Point làm gì cho nó loằng ngoằng ra. Anh chỉ cần dùng Integer làm key cho Map là được rồi, vừa đơn giản lại tốn ít bộ nhớ.

Ghi chú về các trạng thái của cell:

Bài này có 2 biến thể chính:

Sau khi cải tiến xong, anh hãy viết ra cái discussion là 2 biến thể này sẽ ảnh hưởng đến bài làm của anh như thế nào. Cho đến giờ anh làm theo biến thể 1, nếu làm theo biến thể 2 thì dự là anh sẽ cần sửa code như thế nào? Nếu máu me thì anh code thử luôn, còn k thì xong bài này mình sẽ sang tiếp bài khác.

hadv commented 9 years ago

Anh hãy cải tiến, chỉ loop theo các cell nằm trong tập hợp currentGeneration => O(N).

A đã cải tiến bằng việc implement code xoay quanh những live cell trong currentGeneration như commit bên dưới. Em xem giúp anh có ok không nhé!

https://github.com/hadv/ConwayGoL/commit/ec3e163f0a98d945ddaf4b15d9966a6d1d945b61

Phần code sau thì anh thấy cũng chưa hoàn toàn tối ưu đâu. Ngày mai anh sẽ xem xét để cái tiến vì có vẻ như đang bị tính toán lặp lại một số phần.

            // Check if any dead cells around current live cell will become live cell or not
            for (int i = p.getX() - 1; i <= p.getX() + 1; i++) {
                for (int j = p.getY() - 1; j <= p.getY() + 1; j++) {
                    Point pt = new Point(i, j);
                    if (currentGeneration.get(pt) == null) {
                        if (countLiveNeighbourCells(pt) == 3) {
                            tempGeneration.put(pt, LIVE_CELL_VAL);
                        }
                    }
                }
            }

Ngoài ra cách làm với BitSet thì anh dùng integer làm index 1 chiều. Cách làm với Map này anh lại chế thêm cái Point làm gì cho nó loằng ngoằng ra. Anh chỉ cần dùng Integer làm key cho Map là được rồi, vừa đơn giản lại tốn ít bộ nhớ.

Phần này anh sẽ làm trong ngày mai Ngọc nhé, hiện tại a vẫn sử dụng Point vì nó dễ hiểu hơn cách dùng index theo 1 chiều.

Sau khi cải tiến xong, anh hãy viết ra cái discussion là 2 biến thể này sẽ ảnh hưởng đến bài làm của anh như thế nào. Cho đến giờ anh làm theo biến thể 1, nếu làm theo biến thể 2 thì dự là anh sẽ cần sửa code như thế nào? Nếu máu me thì anh code thử luôn, còn k thì xong bài này mình sẽ sang tiếp bài khác.

OK Ngọc, sau khi hoàn thành code theo biến thể số 1 thì mình sẽ quyết định có làm tiếp biến thể số 2 hay làm bài khác nhé. biến thế số 2 có ít tương đồng với việc anh đã implement nhầm theo hướng sử dụng gridMap nhỉ? :smile:

ngocdaothanh commented 9 years ago

tempGeneration.clear();

Cách viết mới thì bỏ cái class member này đi cho gọn.

có vẻ như đang bị tính toán lặp lại một số phần

:+1:

horizontalRight

Mấy cái này chỉ dùng cho toString, cũng bỏ các class member này đi luôn cho gọn. Trong toString sẽ tính giá trị mấy cái này.

sẽ quyết định có làm tiếp biến thể số 2 hay làm bài khác nhé

Anh discuss là được rồi. Đến đây anh đã quen bài này rồi, nên việc discuss quan trọng hơn là làm.

hadv commented 9 years ago

Hi Ngọc,

Cách viết mới thì bỏ cái class member này đi cho gọn.

Anh đã bỏ những biến class member không cần thiết đi rồi. Và các method về extend grid cũng không còn cần thiết nữa theo cách implement mới nên anh cũng xoá hết đi rồi. Nhìn code đã ngắn gọn hơn kha khá rồi :smile: .Ngọc xem commit bên dưới nhé!

https://github.com/hadv/ConwayGoL/commit/77e8737c253d64548f69f163779803ae68ca1f47

toString

Đúng thế nhỉ, có thể tính toán nếu cần. Tuy nhiên, để đơn giản thì hiện tại anh print state trong giới hạn grid 10x10 :smile:

ngocdaothanh commented 9 years ago

Nhìn code đã ngắn gọn hơn kha khá rồi

Thường bài làm pv chỉ khoảng 20-50 dòng thôi (k tính comment). Nếu làm dài hơn thì nên chột dạ.

hadv commented 9 years ago

có vẻ như đang bị tính toán lặp lại một số phần

https://github.com/hadv/ConwayGoL/commit/7dc041682478969840ac247a0270ddd10c5af297

Phần này a viết lại bằng cách dùng constants cho nó gọn, chứ a cũng chưa tìm đc ra cách để tối ưu cho việc tính toán lặp lại cho các dead cell.

Phần này anh sẽ làm trong ngày mai Ngọc nhé, hiện tại a vẫn sử dụng Point vì nó dễ hiểu hơn cách dùng index theo 1 chiều.

Nếu dùng index 1 chiều thì anh thấy là phát sinh nhiều biến phụ và tính toán phụ nên hiện tại anh vẫn sử dụng Point. Theo Ngọc thì thế nào?

Cảm ơn Ngọc nhé!

ngocdaothanh commented 9 years ago

Nếu dùng index 1 chiều...

Trước đây anh đã sửa thử 1 lần rồi. Sửa thành index 1 chiều thì cũng nhẹ nhàng giống lần đấy thôi. Dù sao thì cái này cũng chỉ là một cái discussion, có bài tập về stack mới rồi nên cũng k cần mất công sửa.

anh hãy viết ra cái discussion là 2 biến thể này sẽ ảnh hưởng đến bài làm của anh như thế nào

Anh discuss nốt cái trên.

hadv commented 9 years ago

có bài tập về stack mới rồi nên cũng k cần mất công sửa.

Vậy anh làm bài stack nhé! :smile:

Anh discuss nốt cái trên.

Anh nghĩ là 2 biến thể này ảnh hưởng đến đến bài làm của anh ở các điểm sau đây:

  1. Số lượng neighbour cell không phải lúc nào cũng là 8 cells mà có thể thay đổi tuỳ vào input
  2. Các cell empty thì không thể trở thành live cell hay dead cell được. Khi apply rule thì phải loại trừ các cell này ra.
  3. Khi biểu diễn state của system, chẳng hạn bằng method toString() thì cũng phải biển diễn empty cell khác với live cell và dead cell để có thể thấy được sự khác biệt.

Hiện tại, a nghĩ là có các ảnh hưởng như vậy. Ý kiến của Ngọc thế nào?

ngocdaothanh commented 9 years ago

không phải lúc nào cũng là 8 cells

Tại sao?

hadv commented 9 years ago

Tại sao?

Ý anh là không phải lúc nào cũng là 8 cells mà thuộc các loại live cell và dead cell. Còn nếu tính cả empty cell thì lúc nào cũng là 8 cells nhưng lúc apply rule thì sẽ loại trừ các empty cell ra. A hiểu như vậy có đúng yêu cầu của bài toán không Ngọc?

hadv commented 9 years ago

À nhưng nếu lúc apply rule thì chỉ count số live neighbour cell nên cũng không cần để ý đến empty cell hay không cũng được nhỉ!

ngocdaothanh commented 9 years ago

OK, bài này có thể dừng ở đây. Cái feedback cuối cùng của người pv là: