За задачите
GeorgiPachov opened this issue · 3 comments
public boolean isMonotonous( List<Integer> array)
- малко... форматинга... (навсякъде надолу в методите в Utils е същия. Освен това, array
хич не е хубаво име за ... списък.
- методите в Uitls трябваше да са utility методи, с други думи - статични. Както си ги направила, ти се налага да инстанцираш Utils, клас, който няма член данни, за да ги ползваш. Not cool.
- Иначе хубаво, че си я направила на 1 ред с преизползване на предните. Супер : )
private Utils ms;
ms? Не разбирам името. Лошо име.assertEquals(true, ms.isMonotonous(Arrays.asList(1,2,3,4,5,6)));
- не. ПолзвайassertTrue(booleanExpr)
. Съответно иassertFalse(booleanExpr)
, когато е необходимо.
ArrayList<Integer> array = new ArrayList<Integer>();
array.add(2);
array.add(1);
array.add(3);
array.add(5);
array.add(4);
Не. Ползвай Arrays.asList(2,1,3,5,4)
за такива случаи. В sort
и reverse
никъде не трябва да променяш/чистиш по списъците, така че въобще не те бърка, че Arrays.asList
връща immutable list.
public void log( int level, String message) throws LevelOutOfRangeException
- throws декларацията е излишна при runtime exception-и!
public void log( int level, String message) throws LevelOutOfRangeException{
if(level <= 0){
throw new LevelOutOfRangeException();
}
try{
if(level > this.getLevel()){
return;
}
else{
System.out.println(level + " => " + message);
}
}
catch (LevelOutOfRangeException exception){
exception.printStackTrace();
}
}
Това не го разбирам. Как и защо в try-блока if-else се ще хвърли Exception? Кой ще го хвърли? Откъде ще дойде?
Освен това, представям на твоето внимание кода на Адрияна Стефанова:
public void log(int currentLEVEL, String currentMessage) throws InvalidLogLevelException {
if(currentLEVEL < 1) {
throw new InvalidLogLevelException();
}
try {
if (currentLEVEL > LEVEL) {
return;
} else {
System.out.println(currentMessage);
}
} catch (InvalidLogLevelException e){
e.printStackTrace();
}
}
Доста се съмнявам и двете съвсем случайно и независимо да сте направили една и съща (дооста странна и нетипична!) грешка.
Все ми е тая дали си хвърлила едно око по кода на Адриана и си го преписала на ръка в твоята програма, или Ади е направила нещо подобно. Нито ще се занимавам да crossreference-вам комитите ви, нито ще сравнявам timestamp-ове, нито други подобни. То си е за вас.
Много по-ценно обаче е кода да идва от нас самите, дори да не е правилен, дори да не решава същата задачата даже, отколкото да препишем на ръка чужд код, без да знаем как и защо е написан.
Anyway, също така:
- не си изкарала 3 (default-ното ниво) в константа. Можеше да отиде в едно хубаво final поле.
public Date time;
public DateFormat dateFormat = new SimpleDateFormat("|hh:mm:ss dd.MM.YY| ");
защо са public полетата?
- защо запаметяваш Date? Ако създам Logger-a, и го използвам след 2 минути, ще ми изкара с 2 минути по-старо време. А DateFormat можеше да е константа :)
- Имаш дублиращ код в двата
log
метода. Можеше единия просто да вика другия с още един аргумент - 3. - По същия имаш дублирана логика в двата конструктора на Logger.
- Не използваш super.log методите в DateLogger,а имаше добра възможност да се разминеш с по-малко код, използвайки тях.
Пооправих нещата. Колкото до Exception-a - аз погледнах, защото не бях сигурна как се прави, а нямах време да търся в нета. (И все още не съм сигурна, че е правилно, така че ако може да погледнеш новия exception - ще съм много благодарна. ) Последните няколко корекции не ги разбрах. Не е ли идеята на Logger-a да слага времето, в което съобщенията са се логнали , а не това, в което е създаден?
За последното: да, точно това е идеята! Въпросът ми беше защо ти е член данна за времето на логване? Ти го гледаш това време само в лог метода, и край :) Не ти трябва член данна :)
private final int defaultLevel = 3;
=> case-a за константи е такъв - DEFAULT_LEVEL
. Не го знам как се казва този кейсинг, ама се ползва по конвенция на много места в java за константи.
Също така, на константите не им правим getter методи - тъй като константите няма да се променят, и за да не се копира константата във всеки обект от тип Logger
, я правим static
. А за да я достъпим в наследника, я правим protected static final int ...
. Getter-и за константи - няма смисъл.
public void log( int level, String message) {
if(level <= 0){
throw new LevelOutOfRangeException();
}....
much, much better :) Единия ти log метод преизползва другия, и стана доста по-кратко.
public void log(int level, String message){
if(level <= super.getLevel()){
time = new Date();
System.out.println(dateFormat.format(time));
super.log(level, message);
}
}
public void log(String message){
this.log(super.getDefaultLevel(), message);
}
Тук (първия log метод) => в super.log
я имаш проверката за дали level-a е ОК. Оптималното тук е да си вземеш датата, да я prepend-неш в message (като append, ама отпред), и да викнеш супер.log-а, той да се оправя, там си има и проверката и всичко.
А за втория метод в DateLogger
интересното какво е? Че няма нужда да го override-ваш :) Който го използва ще викне log метода на Logger
, който ще викне log(level, message)
... който е override-нат, и ще се викне метода на DateLogger. How cool is that? :)
Останаха 1-2 override нотацийки, reusage в конструкторите по същия начин, като с log методите, да се махне излишния member Date, и да се направи DateFormatter-а static final, и сме перфектни :)
Оправих го, благодаря. :)