Author Topic: [java] Selection Sort With GUI (Read 3118 times)

leewillz · « **on:** May 10, 2012, 05:10:22 pm »

a little program to show selection sort as a GUI bit more interesting than command line...

Deque · « **Reply #1 on:** May 14, 2012, 07:01:08 pm »

Hello leewillz,

I see that you are not a beginner anymore. You have knowledge in programming. But at the same time you didn't take care to make it right.

1. Your code is pretty sloppy.
2. I ran it and all it shows is an empty window. I didn't look for the cause, but I suppose that it worked for you and thus is a platform dependend problem.

Some additions about the first point:

The indentation sucks in most parts of the code and the bracketing is inconsistent.

Code: [Select]

public SelectionSorter(int[] anArray,JComponent aComponent){
    a = anArray;
    sortStateLock = new ReentrantLock();
    component = aComponent;
    }
public void sort() throws InterruptedException{
    for(int i = 0;i<a.length-1;i++){
    int minPos = minimumPosition(i);
    sortStateLock.lock();
try{
    swap(minPos,i);
    alreadySorted = i;
}finally{
    sortStateLock.unlock();
}
pause(2);
}
}

You write the type in variable names several times, like seen in the code snippet above.
aComponent, anArray - type information doesn't belong in the name.

You define FRAME_HEIGHT and never use it.

Code: [Select]

final int FRAME_WIDTH = 300;
final int FRAME_HEIGHT = 400;
frame.setSize(FRAME_WIDTH, FRAME_WIDTH);

Code: [Select]

frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
That option should be avoided. Close your application properly instead of shutting down the whole JVM out of lazyness, which makes the whole program unembeddable.

Code: [Select]

(int)(Math.random() * 301);
Use Random.nextInt(n) instead. Math.random() is not appropriate for your task, because you have to cast a double to an int and Math.random() requires about twice the time.

Satan911 · « **Reply #2 on:** May 15, 2012, 12:40:41 am »

Quote from: Deque on May 14, 2012, 07:01:08 pm

1. Your code is pretty sloppy.

This. I was going through your code to give you some input and recommendations but I stopped after 10 seconds because the code is not readable. It's a really good to make sure your code is readable by making sure it's properly indented, well commented, the variable names are significant and the overall class structure is important too (doesn't really apply here).

Nonetheless this is a good effort and interesting project. I encourage you to work on your code and don't give up.

techb · « **Reply #3 on:** May 15, 2012, 04:45:56 am »

Quote from: Satan911 on May 15, 2012, 12:40:41 am

[snip] well commented, [snip]

But don't take comments too far. Like don't explain that your assigning 8 to x. Just an overall explanation of what the code is doing in there methods/functions.

Deque · « **Reply #4 on:** May 15, 2012, 02:56:37 pm »

Comments are something that is arguable. Imho they are in most cases an odor for smelling code. If I want to write a comment I first think about a way to avoid this and make the code more readable instead.
Nevertheless I often write Javadocs after I finished the whole stuff to have some documentation.

Satan911 · « **Reply #5 on:** May 15, 2012, 03:51:15 pm »

Hence why I used "well commented" and not "with lots of comments". The definition of well commented is not defined in any way but personally I think you need comments anywhere the code isn't 100% clear. Especially when working on big projects you might end up doing something a certain way because of how it will interact with another part of the software and in that case it's a good practice to justify your code for co-workers or to remind yourself you are not crazy when reviewing your code.

Also always nice to follow proper Javadoc syntax for function headers as it provides nice documentation. I do however know it's really hard to stop and take the time to add a header to all your function when you are coding like crazy and your mind if full of ideas.

leewillz · « **Reply #6 on:** May 16, 2012, 12:49:53 am »

Hi all, thanks for al the feedback, firstly i do all my coding in netbeans and indentation and generall format does get aplied to the file for some reason although in the IDE it is all tabbed, comments are something i lack and i am generally very poor on comments, its definely something i need to work on. also another point was made aout a variable not being used, thats simply my error it should be "frame.setSize(FRAME_WIDTH, FRAME_HEIGHT);". another point i totally agree with is the casting math.random to an integer, the reason for this is very simple, the assignment i was set requred atleast one use of math.random and instead of using thread pools just create threads as needs be, it was basically find an alternative to what you already know assignment, and lastly the names of variables you seemed to not like, that to me is a personal choice i prefer to use a description in my variable names such as anArray for a array variable, it just helps me to keep track of where i am and what each variable is and does, would you prefer int[] a; ? anyway thanks for the feedback guys ill be sure to keep posting some more stuff and making usuer my stuff is tabbed before it comes on here.

Deque · « **Reply #7 on:** May 16, 2012, 06:35:31 am »

Quote

the names of variables you seemed to not like, that to me is a personal choice i prefer to use a description in my variable names such as anArray for a array variable, it just helps me to keep track of where i am and what each variable is and does, would you prefer int[] a; ?

a is not better, especially not for a parameter name. One letter names should only be used for temporary variables.
A good name is one that describes concisely what the variable is for. The type in the name is redundant (Java is strongly typed) and not really helpful. You can see the type in the declaration. In order to get the semantics of a variable that is only named after its type you have to track the variable throughout its life cycle. That is not necessary if you provide the semantics right away in the name.

Satan911 · « **Reply #8 on:** May 16, 2012, 07:25:36 am »

One letter names should never be used at all. I read an article recently on bad variable names and the author mentioned that all variable are temporary variables. So never name a variable temp or data because they are always temp and always data.

I'm not really really strict on variables names but I don't remember ever using one letter name and when I use temp I at least use something lime tempFile and normally I'm more for using curLine or curFile (current) in loops and such just because current brings up the fact that there probably was one before and one after.

Again these are just tips and it's only a few years programming and finally getting to work on big project that you get how important this is. I'm currently starting a new job and the software I'm working on has over 3 millions lines of code. It's impossible to understand every part of it but when you have to work on something specific it helps if the code is really clear and you can understand what it does instantly rather than checking the call hierarchy and trying to find the whole call flow that brings you there. Saves a lot of time and time is money in business... but that also strongly applies to the video game I finished about 1 month ago. Team of 5 and around 40k-50k lines but we wasted so much time understand each others code that the final results suffered a lot from this was of time.

Good habits never get lost.

Deque · « **Reply #9 on:** May 16, 2012, 09:04:54 am »

Quote

I don't remember ever using one letter name

Not even the i in a for loop?

Satan911 · « **Reply #10 on:** May 16, 2012, 03:05:25 pm »

Quote from: Deque on May 16, 2012, 09:04:54 am

Not even the i in a for loop?

Always

My bad on that one.

techb · « **Reply #11 on:** May 16, 2012, 10:02:08 pm »

Quote from: Deque on May 16, 2012, 09:04:54 am

Not even the i in a for loop?

The only time I use i in a for loop is when the var name doesn't matter. Like looping through some numbers to get an average or something. But if I'm looking at the x pixels in an images, I name it appropriately.