akusilvennoinen / Superlaskuttaja-mysql-client

Superlaskuttaja-mysql-client
0 stars 0 forks source link

Koodikatselmointi #1

Open ndjurabe opened 10 years ago

ndjurabe commented 10 years ago

Date and time: 11.04.2014 - 12:26

+Program looks good. Looks like a great deal of work has been put into it. +One great thing is that your methods have good, descriptive names! -The downside to very descriptive names happens with class/package names. It would be great to perhaps rename some of the pacakges/classes into something shorter. For example, the package name "virtuaaliviivakoodigeneraattori.virtuaaliviivakoodigeneraattori" seems a bit too much(?) +-Is there a possibility to place related packages into a single package/file? I feel like it would help viewing the code a bit, for both - yourself and others. +the javadoc descriptions work fine for the program's Logic. I especially liked the extra mini decription that told what the method is used for. Great! +-Would it perhaps be possible to add small descriptions of classes that are not in Logic packages? That could also reduce the need for very descriptive names for those classes. -Although for the most part, the methods make sense and there is little copy-pasting, one method caught my attention as looking a lot like a block of copy-pasting: package: virtuaaliviivakoodigeneraattori.virtuaaliviivakoodigeneraattori.kayttoliittyma.yhteenveto.muokkaa class: MuokkaaLaskuttajanTietojaIkkuna method: luoKomponentit() Do you think it's possible to save all the information into a list and then (using for-loop or something) to go through that list and all of it into the 'tiedotPanel'? +All tests work well, and they seemed reasonable enough :) +The diagrams look good (though those sequence diagrams are kind of gigantic (shortening the names could aid this too ;) )

Hope this was even the tiniest bit helpful. Good work! ^^

akusilvennoinen commented 10 years ago

Thank You for very good feedback!

-The downside to very descriptive names happens with class/package names. It would be great to perhaps rename some of the pacakges/classes into something shorter. For example, the package name "virtuaaliviivakoodigeneraattori.virtuaaliviivakoodigeneraattori" seems a bit too much(?)

Yeah there's too long names and repetition in packet names. Renamed "virtuaaliviivakoodigeneraattori.virtuaaliviivakoodigeneraattori" -> "superlaskuttaja" etc.

+-Would it perhaps be possible to add small descriptions of classes that are not in Logic packages? That could also reduce the need for very descriptive names for those classes.

It would be possible, but maybe later :)

-Although for the most part, the methods make sense and there is little copy-pasting, one method caught my attention as looking a lot like a block of copy-pasting: package: virtuaaliviivakoodigeneraattori.virtuaaliviivakoodigeneraattori.kayttoliittyma.yhteenveto.muokkaa class: MuokkaaLaskuttajanTietojaIkkuna method: luoKomponentit() Do you think it's possible to save all the information into a list and then (using for-loop or something) to go through that list and all of it into the 'tiedotPanel'?

Good idea, but if I do that there will be many lines where I add something to that list thus there will not be smaller amount of lines in that class. Certainly it would look better and tidier to do that with loop.. I'll consider that.

Your feedback was really helpful, thanks again!