BG Development


  Reply to this topicStart new topicStart Poll

> Загадка - какво не е наред с този клас?
FidelDahan
Публикувано на: 03-07-2017, 17:26
Quote Post



Име:
Група: Потребител
Ранг: Почетен член

Мнения: 2177
Регистриран на: 12.06.08




CODE
public final class TimestampRecorder {

 private final Deque<String> records = new LinkedList<>();

 public List<String> getTimestamps() {
   synchronized (this.records) {
     LinkedList<String> timestamps = new LinkedList<>(this.records);
     if (isRecording()) {
       timestamps.removeLast();
     }
     return timestamps;
   }
 }

 public void start(long timestamp) {
   synchronized (this.records) {
     if (!isRecording()) {
       this.records.add(timestamp + "-");
     }
   }
 }

 public void stop(long timestamp) {
   synchronized (this.records) {
     if (isRecording()) {
       this.records.add(this.records.removeLast() + timestamp);
     }
   }
 }

 public boolean hasStarted() {
   synchronized (this.records) {
     return !this.records.isEmpty();
   }
 }

 public boolean isRecording() {
   synchronized (this.records) {
     return hasStarted() && this.records.getLast().endsWith("-");
   }
 }

}


Как може да се подобри дизайна?

А имплементацията как ще стане по-добре?
PMEmail Poster
Top
thrawn
Публикувано на: 03-07-2017, 17:50
Quote Post



Име:
Група: Потребител
Ранг: Старо куче

Мнения: 859
Регистриран на: 17.01.17



А каква е целта на тоя код?

Въртиш някаква опашка, но цялата логика е концентрирана в последния елемент. Значи опашката е излишна и спокойно може да се замени с обикновен String (бих казал, че тук трябва да е някакъв обект който да предлага съответните полета а не да лепиш и парсваш стрингове).

Ако трябва да обработваш опашка, то ти трябва отделен клас който да я реализира/имплементира + логика за синхронизация и евентуална нотификация (ако ще се ползва многонишкова обработка). Трябва ти работник на който менаджерът с опашката да подава обект (последния?) и работникът да връща в опашката съответния статус.

Иначе, така единствения проблем който виждам е в start/stop. Те могат да се издънят (isRecording може да върне false) а ти няма как да обработиш подобно изключение с тоя гол void.

Това мнение е било редактирано от thrawn на 03-07-2017, 17:52
PMEmail Poster
Top
FidelDahan
Публикувано на: 03-07-2017, 17:57
Quote Post



Име:
Група: Потребител
Ранг: Почетен член

Мнения: 2177
Регистриран на: 12.06.08



QUOTE (thrawn @ 03-07-2017, 17:50)
А каква е целта на тоя код?

Нещо като лог за старт/стоп команди, от които се произвеждат интервали. При "старт" започва да се записва ново "парче" аудио, при "стоп" текущото парче е готово. При следващ старт започва следващото и така. Накрая public List<String> getTimestamps() връща тези интервали.

CODE
public class TimestampRecorderTest {

 private TimestampRecorder recorder;

 @Before
 public void setUp() {
   this.recorder = new TimestampRecorder();
 }

 @Test
 public void addingNoDataReturnsEmptyList() throws Exception {
   List<String> timestamps = this.recorder.getTimestamps();
   assertEquals(Collections.emptyList(), timestamps);
   assertFalse(this.recorder.hasStarted());
   assertFalse(this.recorder.isRecording());
 }

 @Test
 public void addingStarttimeReturnsEmptyList() {
   this.recorder.start(1L);
   List<String> timestamps = this.recorder.getTimestamps();
   assertEquals(0, timestamps.size());
   assertTrue(this.recorder.hasStarted());
   assertTrue(this.recorder.isRecording());
 }

 @Test
 public void addingTimestampPairReturnsCompletePart() {
   this.recorder.start(1L);
   this.recorder.stop(2L);
   List<String> timestamps = this.recorder.getTimestamps();
   assertEquals(1, timestamps.size());
   assertEquals("1-2", timestamps.get(0));
   assertTrue(this.recorder.hasStarted());
   assertFalse(this.recorder.isRecording());
 }

 @Test
 public void imcompleteTimestampIsIgnored() {
   this.recorder.start(1L);
   this.recorder.stop(2L);
   this.recorder.start(3L);
   this.recorder.stop(4L);
   this.recorder.start(5L);
   List<String> timestamps = this.recorder.getTimestamps();
   assertEquals(ImmutableList.of("1-2", "3-4"), timestamps);
 }

 @Test
 public void finalizeIncompleteListReturnsCompletePart() {
   this.recorder.start(1L);
   this.recorder.stop(2L);
   this.recorder.start(3L);
   this.recorder.stop(4L);
   this.recorder.start(5L);
   this.recorder.stop(6L);
   List<String> timestamps = this.recorder.getTimestamps();
   assertEquals(ImmutableList.of("1-2", "3-4", "5-6"), timestamps);
 }

 @Test
 public void duplicatedCallsAreIgnored() {
   this.recorder.start(1L);
   this.recorder.start(2L);
   this.recorder.stop(3L);
   this.recorder.stop(4L);
   List<String> timestamps = this.recorder.getTimestamps();
   assertEquals(ImmutableList.of("1-3"), timestamps);
 }

}


QUOTE
(бих казал, че тук трябва да е някакъв обект който да предлага съответните полета а не да лепиш и парсваш стрингове)

Точно, това е проблема в дизайна със стринговете API-то е много зле.

QUOTE

Иначе, така единствения проблем който виждам е в start/stop. Те могат да се издънят (isRecording може да върне false) а ти няма как да го обработиш.

Това е проблема в имплементацията. Дефакто този обект си е стейт-машина...

Това мнение е било редактирано от FidelDahan на 03-07-2017, 17:58
PMEmail Poster
Top
Bender
Публикувано на: 03-07-2017, 18:07
Quote Post



Име:
Група: Потребител
Ранг: Почетен член

Мнения: 4652
Регистриран на: 19.06.14



1. Някак си ми убягва логиката защо всичко е синхронизирано след като е очевидно че не поддържа конкурентни старт/стоп

2. Ако кажа 5 пъти старт ще си мълчи и крие грешката без да се обади. Трябва да хвърля изключение след като не поддържа nested старт/стоп.

3. Също вместо стрингове може да се ползва някакъв вътрешен клас - Interval, който е по-интелигентен, или пък аако се гони някаква супер оптимизация да се ползват 2 масива :Д

4. Не разбирам защо hasStarted() се казва така.

Това мнение е било редактирано от Bender на 03-07-2017, 18:19


--------------------
Живота е спагети, кода за да работи добре трябва да го наподобява - Дон Реба
...
Живеем в греховни времена, какво очакваш богоугоден и благочестив код ли? - Дон Реба
...
много положителна енергия черпя от вас двамата,единият комунистически девствен,другият яко яхнал асемблерните розови понита - saruman
...
Рано или късно усерите на Виндофс разбират че стоят от неправилната страна на хуя. - ici
PM
Top
bvbfan
Публикувано на: 03-07-2017, 18:28
Quote Post



Име:
Група: Потребител
Ранг: Почетен член

Мнения: 2393
Регистриран на: 08.12.13



Не че разбирам Java, но много лошо впечатлени ми прави конструкцията
CODE
synchronized (this.records) {
    if (isRecording()) // <--------- отново synchronized

deadlock? Mutex (Java) е single or recuirsive ?


--------------------
QUOTE (Bender @ 23-04-2015, 19:11)
Xamarin: ЛАПАЙ!
Ти: Добре...
PMEmail Poster
Top
Bender
Публикувано на: 03-07-2017, 18:29
Quote Post



Име:
Група: Потребител
Ранг: Почетен член

Мнения: 4652
Регистриран на: 19.06.14



QUOTE (bvbfan @ 03-07-2017, 19:28)
Не че разбирам Java, но много лошо впечатлени ми прави конструкцията
CODE
synchronized (this.records) {
    if (isRecording()) // <--------- отново synchronized

deadlock? Mutex (Java) е single or recuirsive ?

synchronized e reentrant


--------------------
Живота е спагети, кода за да работи добре трябва да го наподобява - Дон Реба
...
Живеем в греховни времена, какво очакваш богоугоден и благочестив код ли? - Дон Реба
...
много положителна енергия черпя от вас двамата,единият комунистически девствен,другият яко яхнал асемблерните розови понита - saruman
...
Рано или късно усерите на Виндофс разбират че стоят от неправилната страна на хуя. - ici
PM
Top
stewie
Публикувано на: 03-07-2017, 19:21
Quote Post



Име:
Група: Потребител
Ранг: Почетен член

Мнения: 2497
Регистриран на: 14.07.16



Като .нет експерт и джава дебил да попитам - synchronized върху обект работи като lock, т.е. thread sync нали ?
Чудя се колко е надеждно локването ти преди гет операция като hasStarted() - имаш един елемент в свързаният ти списък точно преди някоя нищка да влезе в този скоуп, а точно след нея чака друга на stop() да го маха.
PM
Top
thrawn
Публикувано на: 03-07-2017, 19:28
Quote Post



Име:
Група: Потребител
Ранг: Старо куче

Мнения: 859
Регистриран на: 17.01.17



Ми то тая постановка така или иначе не се прави така.
Редно е да има две опашки - в едната се тъпчат обекти които трябва да се обработят, махат се от там и след обработка се мятат в друга опашка (обработени обекти).
Така ще отпадне и подозрителната функция getTimestamps (поне като логика).

Това мнение е било редактирано от thrawn на 03-07-2017, 19:32
PMEmail Poster
Top
FidelDahan
Публикувано на: 03-07-2017, 22:44
Quote Post



Име:
Група: Потребител
Ранг: Почетен член

Мнения: 2177
Регистриран на: 12.06.08



Утре ако ми остане време ще го рефакторизирам. От стринг към някакъв интервалов тип и с вътрешна стейт машина...
PMEmail Poster
Top
0 потребители преглеждат тази тема в момента (0 гости, 0 анонимни потребители)
Потребители, преглеждащи темата в момента:

Topic Options Reply to this topicStart new topicStart Poll

 


Copyright © 2003-2015 | BG Development | All Rights Reserved
RSS 2.0