Win a copy of Kotlin in Action this week in the Kotlin forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic

Code For Review: Josephus Algorithm GUI  RSS feed

 
Simon Ritchie
Ranch Hand
Posts: 110
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi all,

To gain experience with JavaFX and with the Spring framework I've written this small application which is a graphical representation of the Josephus problem: Given a group of n people arranged in a circle under the edict that every nth person will be executed going around the circle until only one remains, find the position L(n,m) in which you should stand in order to be the last survivor.

This is my first real attempt at a JavaFX GUI and is among my first attempts at using Spring so I'd expect there to be a lot in the code that Java experts would find objectionable so I'm posting it here for review and feedback.  Note that there are far more efficient means to solve the Josephus problem available - there are methods that do it recursively in a very short time.  But performance efficiency was not my aim while designing this.  Instead I wanted to display to the user the people removed at each iteration.

You can set the number of people n and the interval at which they are "killed" by updating the ApplicationContext.xml file.  Either that or you can change the values via the textboxes on the GUI itself.

Any feedback is welcome!

GitHub: Josphus

Thanks
 
Campbell Ritchie
Marshal
Posts: 55694
163
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If at all possible please post the code here; some people are reluctant to download code from links. Also it is easier to say class X line 123 if everybody has the code in front of them.
 
Simon Ritchie
Ranch Hand
Posts: 110
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Sure, I'll post the code in a few minutes.
 
Simon Ritchie
Ranch Hand
Posts: 110
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
My Java Files - src/main/java

GUIJosephus.java




Person.java



Logic.java



Josephus.java (not used)



AlgorithmManagerJosephusImpl.java




JosephusImpl.java (not used)



My Resources - src/main/resources

ApplicationContext.xml



pom.xml

 
Campbell Ritchie
Marshal
Posts: 55694
163
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you.

I would start by cleaning up the indentation and the long lines. Line 121 in the GUI class throws out the formatting for everything else, and should be changed into multiple lines probably joined together by + operators. There are lots of other lines which need similar division. You can have a statement occupying several lines. There are some suggestions for breaking lines in the Sun code conventions (beware: old website).
In the same class, about line 360, you are starting to double‑space all the code. I suggest you only space lines which need spaces between them. It makes for better legibility.
In lines 288‑299, you have gone the other way, cramming something which ought to be on several lines into one line. Use at least four lines for a try: one for the try keyword, one for the {, one or many for the body of the statement and one line for the }. You can reduce that by one line if you use K&R indentation. I don't like to see } catch (MyException exc) { all on one line. I think it makes for difficulty reading, but other people are happy with that sort of code.

Why are you using that package name? Do you own the website bh25034.com? That format of package name belongs to whoever owns that website. Look here in the Java™ Tutorials. And bh25034 isn't a good package name unless you need to use it as a student number or similar.

Your algorithm manager interface should have lots of documentation comments; in fact something like 75% of many (pre‑Java8) interfaces consists of comments. You need to explain to users what each method is supposed to do and establish a general contract for it. Be very careful with these comments, because if you ever let that interface loose on an unsuspecting world, you can never change it.

Why are you writing Vector? That is (unofficially) considered to be a legacy class. Declare your Lists as List<XYZ> and consider whether to implement them as ArrayList, LinkedList or whatever.

Do you need all those fields in the GUI class? Can you change them to local variables in one of the methods? Why are yo usetting the min size and max size of your components to the same?

Sorry I haven't taken longer to look at your code; there is bound to be a lot more that I haven't noticed. I shall let others find that. I hope that is helpful.

Please confirm you are keeping the code for the algorithm separate from the code for the GUI. If yes, I am very pleased about that because it shows you know how to design an app. In fact the little code I have managed to look at does look good.
 
Simon Ritchie
Ranch Hand
Posts: 110
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You're right about the long lines - I only noticed from looking at my post how annoying it appears.  The reason that particular line is so long is that I just copied it from a website that gave a definition of Josephus and never bothered to change it.  The comments you make on double spacing are welcome.

The reason I chose that package name is just for simplicity.  It's an old student number.  If I was releasing this as a commercial application I'd have chosen something more appropriate.  I don't own a website so can't name it after that.

Vector is a legacy class?  Does that mean its use is being discouraged and it's soon to be deprecated?  If so I'm out of touch!  I'll re-code all instances as a List<XYZ>

I am keeping the code for the algorithm very much separate to the GUI, deliberately so.  In fact, one of the stumbling blocks was I didn't know how to incorporate multiple threads into the GUI (previously after a user clicked "Run" the GUI would go unresponsive as the algorithm worked through the numbers) and I spent some time looking at that as I wanted the algorithm to run in its own thread.

Many thanks for looking at my code, though!
 
Campbell Ritchie
Marshal
Posts: 55694
163
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Simon Ritchie wrote:. . . The reason I chose that package name is just for simplicity.  . . .
How about
package josephus.gui;
package josephus.algorithm;
or similar?

You will find people look at everything. That is what you sign up for with code review; you allow our sadistic streak free rein mwaahaahaahaahaahaahaahaa!
Vector is a legacy class?  Does that mean its use is being discouraged and it's soon to be deprecated?  . . .
No. It simply means everybody uses ArrayList instead. You get much more flexibility if you say List.
. . . incorporate multiple threads into the GUI . . . I wanted the algorithm to run in its own thread.
According to books like Core Java II by Horstmann and Cornell (latest edition by Horstmann only), it has proven very difficult to create a thread‑safe GUI framework which executes at a speed greater than that of a snail with ingrowing toenails. So there are severe restrictions about threading GUIs. In Swing, you would start a worker thread (see the Java™ Tutorials), which runs separately from the display. I presume there is something analogous in FX.
Many thanks for looking at my code, though!
My pleasure Actually it is nice to read.
 
Simon Ritchie
Ranch Hand
Posts: 110
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Campbell Ritchie wrote:
Vector is a legacy class?  Does that mean its use is being discouraged and it's soon to be deprecated?  . . .
No. It simply means everybody uses ArrayList instead. You get much more flexibility if you say List.


I feel stupid for asking but what do you mean by "legacy class" - what is it and why is it a bad thing?

Campbell Ritchie wrote:
According to books like Core Java II by Horstmann and Cornell (latest edition by Horstmann only), it has proven very difficult to create a thread‑safe GUI framework which executes at a speed greater than that of a snail with ingrowing toenails. So there are severe restrictions about threading GUIs. In Swing, you would start a worker thread (see the Java™ Tutorials), which runs separately from the display. I presume there is something analogous in FX.

The equivalent in FX appears to be the Task class.  I'm not sure how it compares to Swing's implementation...

Campbell Ritchie wrote:My pleasure Actually it is nice to read.


Thanks :-)
 
Simon Ritchie
Ranch Hand
Posts: 110
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
For some reason I can't edit my posts...
 
Campbell Ritchie
Marshal
Posts: 55694
163
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
It is better that you post the new version and the differences will become easier to see.

Thank you for the pie, whoever sent it I think you did.
 
Simon Ritchie
Ranch Hand
Posts: 110
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Yeah, that was me.
 
Campbell Ritchie
Marshal
Posts: 55694
163
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you again. I am looking forward to the next instalment, so we can see the improvements.

I shall duplicate this discussion in another forum because I think it will get more attention.
 
Simon Ritchie
Ranch Hand
Posts: 110
4
Eclipse IDE Hibernate Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Right, I've made the following changes:

  • I've changed the package names from bh25034 to josephus
  • I've included documentation/comments in the AlgorithmManagerJosephus interface
  • I've removed unused or outdated comments
  • I've changed all instances of Vector to List/ArrayList where appropriate
  • In the GUIJosephus class I've expanded the try statements out from one line to several lines


  • I might have made some smaller changes too but can't recall at the minute.  Anyone who's reviewing, I'd appreciate feedback on the following aspects of the code:

    How can I make the GUI look better?  It uses JavaFX but thanks to my inexperience it doesn't look a whole lot better than a GUI using Swing.
    How can I make better use of the Spring framework?  I just use one bean here.  If I wanted to make this a MVC-compliant application what would my class structure look like?

    GUIJosephus.java



    AlgorithmManagerJosephus.java



    AlgorithmManagerJosephusImpl.java



    Person.java



    ApplicationContext.xml

     
    Campbell Ritchie
    Marshal
    Posts: 55694
    163
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Thank you
    I trust other people will look at this discussion and will have their own comments. I see you have implemented most of the changes I have suggested. I can see some problems later on.

    Why are you using Thread#sleep? I don't know about FX, but if you use Thread#sleep in a Swing application, you are liable to freeze the display. It also depends which Thread you are sleeping; it may be all right to make a worker thread sleep. But please consider why you want it to sleep at all.
    Why are you using the exclusive or operator? Look it up in the Java® Language Specification (=JLS), but the JLS can be difficult to understand. I know people think it means square, but it doesn't. If you want squares, write x * x (usually). But I don't think you want to calculate squares. You want something like this.
    There is a lot about documentation comments here, and also in Effective Java by Joshua Bloch. Be sure to use the 2nd edition.
    I would call my components ageLabel or similar. Avoid abbreviating the names if possible (despite recent examples from other people, like this). Find out about so‑called Hungarian notation, for example here: be sure to read the three links, too.

    That should keep your nose to the grindstone and your shoulder to the wheel for the whole of today Mwaahaahaahaahaahaahaahaahaa
     
    Simon Ritchie
    Ranch Hand
    Posts: 110
    4
    Eclipse IDE Hibernate Spring
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I'm getting some very strange results using Maths.hypot() instead of my existing method.  Here, for example:



    xord and yord are initialised with a value of 300 each.  The result of Math.hypot(xord, yord) is 424.26406871192853.  The result of squaring xord, adding it to the square of yord and then getting the square root of that sum is 24.576411454889016.
     
    Campbell Ritchie
    Marshal
    Posts: 55694
    163
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    That is because you are still using the exclusive or operator. Read the documentation for Math#hypot() and see what it takes as its arguments. That method is intended for converting rectangular coordinates to polar, along with this method, but at present you probably only need half the conversion, so you only need one method.
     
    Simon Ritchie
    Ranch Hand
    Posts: 110
    4
    Eclipse IDE Hibernate Spring
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    You're right (again), thanks - I've corrected how the circles are drawn and it looks fine now.

    Onto the rest of the updates...
     
    Campbell Ritchie
    Marshal
    Posts: 55694
    163
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Simon Ritchie wrote:I'm getting some very strange results . . .  The result of Math.hypot(xord, yord) is 424.26406871192853.  . . .
    According to my SpeedCrunch app: 300 × √2 = 424.26406871192851464051;

    You are right; the 3 is incorrect
     
    Simon Ritchie
    Ranch Hand
    Posts: 110
    4
    Eclipse IDE Hibernate Spring
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Campbell Ritchie wrote:Why are you using Thread#sleep? I don't know about FX, but if you use Thread#sleep in a Swing application, you are liable to freeze the display. It also depends which Thread you are sleeping; it may be all right to make a worker thread sleep. But please consider why you want it to sleep at all.


    I'm using Sleep purely for stylistic purposes.  I want the user to see which people are removed at each iteration of the loop.  I also wanted to find out how to incorporate multi-threading into a graphical application and this was a good excuse to do it.  The good news is that the Task class in FX executes on its own thread so it doesn't freeze the display like in Swing.

    Now, who wants to see the updated GUI class?  I've made changes to how the positions of the circles on the canvas are calculated (now using Math.hypot instead of the logical OR operator).



    I'll make the changes to the documentation on the interface later, I promise ;-)
     
    Campbell Ritchie
    Marshal
    Posts: 55694
    163
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Don't know enough FX to be able to say much about the GUI, but, again, why are you setting the maximum and minimum sizes the same?
    You have still got your pl method (line 394) squashed into one line. Why are you using System.out in a GUI? You might not have access to System.out if you start your app from an executable .jar. You sh‍ould probably not use System.out for Exceptions; use System.err instead. Why is that method not static? It is very unusual fo rme to ask that question.
    I don't like using Exceptions to check whether your text fields contain a number. You will find a deep discussion about detecting numbers here.
    Can you think of another way to view the killing off, e.g. using a button to move to the next stage?
     
    Jeanne Boyarsky
    author & internet detective
    Sheriff
    Posts: 37230
    519
    Eclipse IDE Java VI Editor
    • Likes 1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Simon,
    First , I"m mentioning everything I see that could be improved or you might want to do. I think you'll find that helpful based on your other thread. You do many things well. I don't know Java FX well either so my comments are about the rest. Finally, I didn't read the whole thread (just the first post) before posting so you get feedback independent of Campbell. Like maybe it will be useful to see if we notice the same things or differenet ones.

  • By convention, Maven group ids follow the same conventions as package names. So com.bh25034 would be a better name.
  • By convention, Maven artifact ids are lowercase (and use dashes if two words). So josephus would be a better name. Or josephus-algorithm
  • Have you learned about @Override yet? It's helpful to use it in implementations of interfaces so you know quickly what goes with the interface
  • I forgot if you are using an IDE like Eclipse. If so, try running your code through its formatter to see what it changes. For example, your one line methods are harder to read than they need to be.
  • Try to convert your code to use the enhanced for loop. No need for referencing loop indexes when just going through a list. (At some point you should re-write to Java 8, but that's a bigger jump.
  • Is there any reason you use Vector instead of ArrayList? Unless you are interacting with a legacy library that requires it, Vector tells me you work on old code (or haven't learned anything new.) Not the message you want to share when you start interviewing


  • And to end with something that jumped out a me as a really good thing - the code was easy to read - good variable names!
     
    Jeanne Boyarsky
    author & internet detective
    Sheriff
    Posts: 37230
    519
    Eclipse IDE Java VI Editor
    • Likes 1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Also, I just noticed this is in the Java 8 forum. The code isn't using Java 8 features though. Updating it to use lambdas/streams would be a good exercise.
     
    Simon Ritchie
    Ranch Hand
    Posts: 110
    4
    Eclipse IDE Hibernate Spring
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Thanks for those comments Jeanne - they echo closely Campbell's feedback.  I'll make the necessary changes to the POM you recommended.  I've been aware of @Override but never really thought about adding that annotation before.  It's something I'll have to get into the habit of.  I'm using the Eclipse Luna IDE.

    I've a couple of questions, if that's okay?

    I'll make changes to the for loops too, as you recommended, but how would it look when re-written in Java 8 (I'm assuming you mean re-write the for loop in Java 8)
    I was using Vector because I'm, well, pretty out of touch as a Java developer and had no idea that it was considered old.  Is there a reference of other legacy classes that I should be aware of?

    I put this in the Java 8 forum because JavaFX is exclusive to Java 8, I thought.  But I'll read up on streams and lambdas (I'm not familiar with either), thanks.
     
    Campbell Ritchie
    Marshal
    Posts: 55694
    163
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Good think you got feedback from Jeanne; it is not a good idea for this thread to turn into a sort of Campbell Ritchie show. Especially if I get something wrong.
    I still think you have too much double‑spaced code. After line 280.
    I think you can enhance the findPeople method to use a Stream and a λ. You can get a Stream out of that List, and you can find out from that Stream whether you have elements matching certain predicates, which match the logical quantifiers ∃ and ∀. If you get a Stream to work, it will look much more elegant than that loop.

    I thought FX was older than Java8. I may be mistaken, however.
     
    Campbell Ritchie
    Marshal
    Posts: 55694
    163
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Simon Ritchie wrote:. . . I'll make the changes to the documentation on the interface later, I promise ;-)
    Agree; that can wait until after you have been through the rest of the code.

    I went back to your original post and broke line 121 and the first line of one of the XML files into several lines each, now that you are writing shorter lines. The left and right scrolling shou‍ld have stopped.
     
    Campbell Ritchie
    Marshal
    Posts: 55694
    163
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Simon Ritchie wrote:. . . I feel stupid for asking but what do you mean by "legacy class" - what is it and why is it a bad thing? . . .
    Sorry I didn't notice that bit earlier.

    If a class is officially described as a legacy class (like this one), they discourage you from using it in new code. Vector has a synchronisation overhead. It has fallen out of use because ArrayList usually gives faster execution, and can be synchronised if required with methods of the Collections class. So in this instance “legacy” means simply, “everybody uses something different nowadays.” See what Jeanne wrote about Vector, which probably explains it a lot better than I have. I see Jeanne also found the code good to read, so well done there
     
    Campbell Ritchie
    Marshal
    Posts: 55694
    163
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    We used the Josephus algorithm at primary school when playing tig (=tag), to see who was “it”.
     
    Jeanne Boyarsky
    author & internet detective
    Sheriff
    Posts: 37230
    519
    Eclipse IDE Java VI Editor
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Campbell Ritchie wrote:I thought FX was older than Java8. I may be mistaken, however.

    I think there were a lot of changes in Java 8, but it was definitely in Java 7. (at least a few fix packs in)

    Simon Ritchie wrote:I'll make changes to the for loops too, as you recommended, but how would it look when re-written in Java 8 (I'm assuming you mean re-write the for loop in Java 8)

    Here's an example:


    is the same as:


     
    Don't get me started about those stupid light bulbs.
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!