facecolors.txt might not get loaded on program startup.

[GoogleCodeExporter]

If you don't have a 'logfile' entry in your .props file, it doesn't load the 
face colors when you first start the program :(  This is because the default 
puzzle building starts when MC4DSwing line 618 is reached, but the 
puzzleChanged listener isn't added until line 619.

Original issue reported on code.google.com by [email protected] on 20 Nov 2009 at 4:51

[GoogleCodeExporter]

Original comment by [email protected] on 20 Nov 2009 at 6:32

[rzhao271]

Well, here's a small hack... http://prntscr.com/7x1tal

[cutelyaware]

Duplicating code is almost always a bad idea because it bloats the code and the multiple versions can get out of sync when a bug is fixed in one but not the others. Ideally, each block of code will be unique, and any overlap should be factored out. It looks to me like the real problem here is that some puzzle initialization does not happen when a log file is not found. the key code is in MC4DSwing beginning on line 942:

if(logfile.exists()) {
    ...
    puzzleManager.initPuzzle(schlafli...
    ....
}

I think a clean fix would be to add an else case that sets the default puzzle, something like this:

else 
    puzzleManager.initPuzzle(MagicCube4D.DEFAULT_PUZZLE, MagicCube4D.DEFAULT_LENGTH, ...);

Or slightly better, invert the test to check for the exceptional case first, especially as it's the simpler one:

if( ! logfile.exists())  // Initialize the default puzzle.
    puzzleManager.initPuzzle(MagicCube4D.DEFAULT_PUZZLE, MagicCube4D.DEFAULT_LENGTH, ...);
else {   // read the log file, possibly reinitializing length and history.
    ....
}

That way, the puzzle listener is always called as a result of initPuzzle() being called, which is also what the name implies. Doing this may even let us remove some other code that sets a default puzzle elsewhere.

For future code reviews, we should use patch files rather than screen shots. Your IDE will have an easy way to generate them I'm sure. I assume you can then attach them to to these issues though I forget how.

[rzhao271]

Not quite sure about how to generate patch files. I see the 'Team > Apply Patch' option though.

I made a blank constructor for puzzleManager and made it so that initPuzzle was called later, esp since the original constructor called initPuzzle anyway but without the listener yet in place.

The else cases were then added that called initPuzzle with default parameters. One else case is if there's a parse error in the log file, and the other is if there file doesn't exist. Because of the way the parser has to run through the file first, I think it's better to leave it this way rather than invert the if-else.

                if (!parsingError) {
                    statusLabel.setText("Read log file '" + log + "'");
                }
                else { // for serious cases where log file is unreadable
                    initDefaultPuzzle(); // first call here
                }
            }
            else { // case where the log file doesn't even exist
                statusLabel.setText("Couldn't find log file '" + log + "'");
                logFileChooser.setSelectedFile(null);
                initDefaultPuzzle(); // another call here
            }

[cutelyaware]

A no-argument constructor could be a good idea. If so, that suggests PuzzleManager may want to be a singleton. Do you know the pattern?

Another option to creating more complicated if/else trees is to only perform one default puzzle initialization at the end, regardless of why it's needed. EG:

 if(log != null) {
     try {
         ...
     }
 }
 if(not initialized) // Could be a local variable or a method call, 

just not a member variable. initDefaultPuzzle();

See if you can figure out how to create patch files. You'll need to do that sooner or later anway.

On 7/26/2015 11:08 AM, Ray Z wrote:

Not quite sure about how to generate patch files. I see the 'Team > Apply Patch' option though.

I made a blank constructor for puzzleManager and made it so that initPuzzle was called later, esp since the original constructor called initPuzzle anyway but without the listener yet in place.

The else cases were then added that called initPuzzle with default parameters. One else case is if there's a parse error in the log file, and the other is if there file doesn't exist. Because of the way the parser has to run through the file first, I think it's better to leave it this way rather than invert the if-else.

             if  (!parsingError) {
                 statusLabel.setText("Read log file '"  +  log+  "'");
             }
             else  {// for serious cases where log file is unreadable
                 initDefaultPuzzle();// first call here
             }
         }
         else  {// case where the log file doesn't even exist
             statusLabel.setText("Couldn't find log file '"  +  log+  "'");
             logFileChooser.setSelectedFile(null);
             initDefaultPuzzle();// another call here
         }

— Reply to this email directly or view it on GitHub https://github.com/cutelyaware/magiccube4d/issues/95#issuecomment-125023860.