The data-encryption-to-ppm-image from tul-it-2020

Code review ALG2

0. Dodelat JavaDoc a vsude doplnit popis parametru:

Napriklad:

/**
* Load info about file to memory from picture. 
* @param header //zbyva popis
* @param data  //zbyva popis
*/

1. Vyhodit vsechne nepouzite promene a nepouzite importy. Vyhodit vsechne okomentovane kusy kodu, ktere se uz nepouzivaji a nepouzite metody.

2. Nazvy promen se pisou s male pismeny a camel casem, constanty velkymi pismeny a pres podtriztko.

napriklad mas v kodu

private String MagicNumber;

ktere by se psalo

private String magicNumber;

atd.

3. Aplication.java

Podle me, vsechno co se tyka uzivatele (sc.nextLine(), System.out.println(...)) musi byt v UI (User Interface = Uzivatelske rozhrani).
Tim padem Aplication.java jen spusti UI.run(...(muze obsahovat parametry, ktere se nemusi byt v UI)...), ve ktere jsou vsechne ty metody, co jsou ted v Aplication.java.

4. Metoda `private static void addFile () {...}` (Aplication.java)

4.1 Promene:

private static DataFile dataFile2store;

nemusi byt classovou promenou, staci byt lokalni v te metode

4.2 vyhodit nepouzitelne pole:

DataFile[] dataFiles;

4.3 zjednodusit podminku:

if (picture.addFile(dataFile2store) == false) {...}

slo by

if (!picture.addFile(dataFile2store)) {...}

protoze metoda picture.addFile vraci boolean

4.4 zjednoduseni podminky

if (!picture.addFile(dataFile2store)) {
    UI.print("Nelze provést! Soubor je příliš velký.\n");
} else {
    UI.print("Soubor byl uložen do obrázku\n.");
}

slo by prepsat jako

if (!picture.addFile(dataFile2store)) {
    UI.print("Nelze provést! Soubor je příliš velký.\n");
    return;
}
UI.print("Soubor byl uložen do obrázku\n.");

jak jsi to delal o par radku nahoru

5. metoda `public static void main(String[] args) {..}` (Aplication.java)

5.1 Promene:

private static boolean exit = false;

nemusi byt classovou promenou, staci byt lokalni v te metode

boolean exit = false;

6. UI.java

6.1 NullPointerException
V metodach

public static File[] listAllPictires(File dir, String extension) {...}

public static File[] listAllFiles(File dir) {...}

nacitas soubory a ukladas do Files[] files a pak pouzivas cyklus for pro vypis nazvu az se nepostane do files.length, ale metoda

File[] files = dir.listFiles();

vraci null kdyz je spatne uvedeny nazev adresari a length from null vyhodi ti NullPointerException, ktery jsi zadnym zpusobem neosetril, co vede k padnuti programu.

6.2 Zbytecna promena
Namisto

String name = sc.nextLine();
return name;

stacilo by napsat

return sc.nextLine();

7. PictureFormatInterface.java

7.1 Modifiers
V interface by default modifikatory pristupu je public, proto nemusis je psat

public int getHeight();
public int getwidth();
...

7.2 Priorita vyjimek
Metody

public void loadPicture(File path) throws FileNotFoundException, IOException;

public void save2File(File path) throws FileNotFoundException, IOException;

nemusi vyhazovat FileNotFoundException protoze vyhazuje IOException, ktera je prednostni a obsahuje FileNotFoundException

8. FormatPPM.java

8.1 private boolean readHead (BufferedReader br) throws IOException {...}
Pokud mas jednu podminku a pak defaultni akce, tak je lepe pouzivat if misto swicth.

switch (MagicNumber) {
    case "P3": 
        break;
    default:
        return false;   // unsuported format
}

slo by

if (!"P3".equals(MagicNumber)) {
    return false;   // unsuported format
}

Navic slo by zjednodusit podminku na konci:

if (Integer.parseInt(br.readLine()) != MAX_VALUE) return false;
return true;

klidne se prepise jako

return Integer.parseInt(br.readLine()) == MAX_VALUE;

8.2 private boolean readData (BufferedReader br) throws IOException {...}
– Vymenit switch na if, protoze je zbytecny (stejne jako v bode 8.1 nahore)

– Zbytecny if:

if ((pixel = readPixel(br)) == null) return false;

protoze promena pixel vsude ma hodnotu a metoda readPixel nikdy nevrati null proto promena pixel nikde nedostane hodnotu null => zbytecna podminka => vyhodit

– Vyhodit okomentovane testovaci potreby:

//System.out.format("pixel index: %d \n", i);
//System.out.format("pixel: %s \n", pixel);

A cela ta metoda se prepise z tvaru:

private boolean readData (BufferedReader br) throws IOException {
        data.clear();
        switch (MagicNumber) {
            case "P3":
                Pixel pixel;
                for (int i = 0; i < this.height*this.width; i++) {
                    //System.out.format("pixel index: %d \n", i);
                    if ((pixel = readPixel(br)) == null) return false;
                    //System.out.format("pixel: %s \n", pixel);
                    data.add(pixel);
                }
                break;
            default:
                return false;   // unsuported format
        }
        return true;
}

private boolean readData (BufferedReader br) throws IOException {
        data.clear();
        if ("P3".equals(MagicNumber)) {
            for (int i = 0; i < this.height * this.width; i++) {
                data.add(readPixel(br));
            }
            return true;
        }
        return false;   // unsuported format
}

8.3 private boolean writeData (FileWriter fw) throws IOException {...}
Vymenit switch na if, protoze je zbytecny (stejne jako v bode 8.1 a 8.2 nahore)

9. DataFile.java

9.1 public Byte getNextNbites(int chunkSize) {...}
Nepouzivas tuto metodu, je v ni potreba?

10. Picture.java

10.1 Classove promenne mohou byt final, protoze jednou nastavis jejich hodnoty jen v konstruktoru a nemenis je.

private File path;
private String name;
private String format;
private PictureFormatInterface pictureDataAndInfo;
private RandomAccessPixelStream data;

10.2 Invert methods
Metodu

private boolean checkForHeader () {
...
return HEADER_KEY.compareTo(pictureContent.toString()) == 0;
}

// nekde v kodu
if (!checkForHeader()) {
    return null;
}

nekolikkrat pouzitas s negaci, proto by slo negovat vysledek ty metody.

private boolean checkForHeader () {
...
return HEADER_KEY.compareTo(pictureContent.toString()) != 0;
}

// nekde v kodu
if (checkForHeader()) {
    return null;
}

10.3 Spatny return type
Metodu

private boolean createAndStoreHeader () {
        // TODO check if key fit to file !!!
        data.setByteIndex(0);
        byte[] array = HEADER_KEY.getBytes();
        for (byte B : array) {
            data.storeNextByte(B);
            //System.out.format(" %d \n", (int)B);
        }
        // empty chain termination
        data.storeNextNBytes(int2Bytes(EMPTY));
        //System.out.format(" %d %d \n", data.get(0).getR(), data.get(0).getG());
        /*
        for (int i = 0; i < HEADER_KEY.length()/NUMBER_OF_CHANELS; i++) {
            System.out.format(" %s \n", data.get(i));
        }*/
        return true;
}

bych prepsal bez zbytecneho kodu a zmenil bych return type na void, protoze vraci porad true a typboolean nikdy nevyuzivas.

10.4
Metody

public int getNumberOfStoredFiles () {...}

public DataFile[] storedFiles() {...}

padnou ( NullPointerException ), kdyz behem nacitani dat nenajde adresar.

10.5 Vyhodit testovaci metodu main.

11. RandomAccessPixelStream.java

11.1 Zbytecna inicializace listu:

private List<Pixel> data = new ArrayList();

Stacilo by

private List<Pixel> data;

11.2 Nemenne classove promene predelat na final.

11.3 public long getCapacity () {...}
Nejlip bych zjednodusit return a urcite potrebujes kastovat (cast) int to long:
Code before:

public long getCapacity () {
        assert chunkSize != -1 : "ERROR, invalid implementation";
        long capacity = data.size()*3*chunkSize/8;
        return capacity;
}

After:

public long getCapacity () {
        assert chunkSize != -1 : "ERROR, invalid implementation";
        return (long) data.size()*3*chunkSize/8;
}

12. ByteTools.java

12.1 Typizace interfasu
Interface List je tzv. Generic a musis psat, jakeho typu je ten List:

public static void add2List (List list, byte[] bytes) {...}

V tvojem pripade je List<Byte> list

12.2 Zjednoduseni metody
– String se inicalizuje pres operator "" => String s = "", v zadnem pripade String s = new String();.
– Pro opakovane pridani jednoho String k jinemu existuje metoda s.repeat(), nemusis psat cyklus for.

V tvem pripade kod se zmeni takto:
Bylo

public static String int2BinString (int n, int len) {
        String bin = toBinString(n);
        String zeros = new String();
        for (int i = 0; i < len - bin.length(); i++) {
            zeros += String.valueOf(0);
        }
        return zeros + bin;
}

Stalo

public static String int2BinString (int n, int len) {
        String bin = toBinString(n);
        return String.valueOf(0).repeat(len - bin.length()) + bin;
}

12.3 Hodne nepouzitelnych metod. Potrebujes je?

Zaver

Celkove je velmi dobre, tridove rozdeleni je v pohode, mozna bych se kouknul jestli nezle neco zmenit ve prospech interfasu, jinak super.
Mas hodne okomentovanych kusu kodu, nepouzitych metod, promen a importu — je poterba to uklidit.
Nekde je mozny kod zjednodusit, pises to komplikovane, kvuli neznalosti featur jazyka.
A vice bych pracoval z vlastnimi vyjimkami.
Jinak velmi dorbe!

tul-it-2020 / data-encryption-to-ppm-image Goto Github PK

data-encryption-to-ppm-image's People

Contributors

Watchers

data-encryption-to-ppm-image's Issues