snejy/Core-Java-1

За задачите

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, и сме перфектни :)

Оправих го, благодаря. :)