3
\$\begingroup\$

Little history to give context to the problem:

The Krusty-Burgers

Homer Simpson is a smart guy who likes to eat Krusty-Burgers, and it takes m minutes for Homer to eat a Krusty-Burger. However, there is a new type of burger on the market, the Apu-Burger. Homer likes them too, and takes n minutes to eat one. Given just minutes, you have to figure out the maximum number of burgers that Homer can eat without wasting time. If he has idle time, he can drink beer.

Input

The input consists of several test cases. Each test case consists of three integers m, n, t (0 <= m, n, t <= 1000).

Output

For each test case, the maximum number of burgers that Homer can eat without beer should be written on a single line. If Homer drinks beer by spare time, one should also print the time he has to drink, separated by a blank. It is preferable for Homer to drink as little beer as possible, as Marge does not like it when he arrives drunk at home.

Input example

3 5 54

3 5 55

Example of output

18

17

I would like to know if I can improve my code in some way in terms of performance or writing

import java.io.BufferedReader;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.sql.Date;
import java.text.SimpleDateFormat;

import javax.swing.JFileChooser;
import javax.swing.JOptionPane;
import javax.swing.filechooser.FileFilter;

public class ChallangePerformance {

    private  static File archive;
    private  static BufferedReader rdFont;
    static String fileLine = null;
    static int structuresNumber;

    public static void main(String[] args) 
    { 

        openFile();

        long start = System.currentTimeMillis();  

        boolean still_has = true;

        while ( still_has ) {
            readFileLine();
            if ( fileLine == null ) {
                still_has = false;
            } else {
                String array[] = fileLine.split(" ");           
                int kb = Integer.parseInt( array[0] );
                int ab = Integer.parseInt( array[1] );
                int total = Integer.parseInt( array[2] );
                int na = 0;
                int nk = (int) Math.floor(total/kb);
                int dif = total-nk*kb;
                int mk = nk;
                int ma = na;
                int ms = dif;
                while ( nk > 0 ) {
                    nk--;
                    int leftover = total-nk*kb;
                    na = (int) Math.floor(leftover/ab);
                    dif = total - (nk*kb + na*ab);
                    if ( dif < ms && ms > 0) {
                        mk = nk;
                        ma = na;
                        ms = dif;
                    }
                }
                System.out.println( fileLine );
                System.out.println( " " + mk + " " + ma );
            }
        }


        long end  = System.currentTimeMillis();   
        System.out.println(new SimpleDateFormat("ss.SSS").format(new Date(end - start)));  

    }

    static void readFileLine() 
      {
        try {
            fileLine = rdFont.readLine();
        } catch (IOException e) {
            e.printStackTrace();
        }
     }

      private static void openFile() {

            JFileChooser fileChooser = new JFileChooser();

            fileChooser.setFileSelectionMode( JFileChooser.FILES_ONLY );

            Filter7 filtro = new Filter7();

            fileChooser.addChoosableFileFilter( filtro );
            int result = fileChooser.showOpenDialog( null );

            if( result == JFileChooser.CANCEL_OPTION ) {
                return;
            }

            archive = fileChooser.getSelectedFile();
            openFont( archive );    

        }

        private static boolean openFont( File fileName ) {

            if( archive == null || fileName.getName().trim().equals( "" ) ) {
                JOptionPane.showMessageDialog( null, "Invalid file name", "Invalid file name", JOptionPane.ERROR_MESSAGE );
                return false;
            } else {
                try {
                    FileReader fr = new FileReader( archive );
                    rdFont = new BufferedReader( fr );
                } catch (FileNotFoundException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                } 
                return true;
            }
        }

}

class Filter7 extends FileFilter { 

    public boolean accept(File arg0) { 
         if(arg0 != null) {
             if(arg0.isDirectory()) {
              return true;
             }
             if( getExtensao(arg0) != null) {
                 if ( getExtensao(arg0).equalsIgnoreCase( "est" ) ) {
                     return true;
                 }
             };
         }
         return false;
    }

    /**
     * Returns which extensions can be chosen
     */
    public String getDescription() {
        return "*.est";
    }

    /**
     * Returns the part with the extension of an archive
     */
    public String getExtensao(File arq) {
    if(arq != null) {
        String filename = arq.getName();
        int i = filename.lastIndexOf('.');
        if(i>0 && i<filename.length()-1) {
            return filename.substring(i+1).toLowerCase();
        };
    }
        return null;
    }
}
```
\$\endgroup\$
  • 3
    \$\begingroup\$ Your logic is extremely hard to understand due to your variable naming e.g., kb, mk, ma. You should consider variable names that do not need explanation and outsource computations to methods so you can give them names. \$\endgroup\$ – Vajk Jun 17 at 18:29
  • \$\begingroup\$ The spec seems to be missing an explanation of what t is. \$\endgroup\$ – Peter Taylor Jun 18 at 9:46
4
\$\begingroup\$

The use of static variables to read the input file is kind of ugly and makes it harder to reason about what the code is doing. I'd ditch the archives, rdFont and fileLine variables, moving them into the main method. The openFont and readFileLine methods don't feel necessary either - openFile could just return the File that was picked and let the caller do whatever it wants with it.

The two-letter variable names make your code much harder to read. Yes, it's possible to figure out what it's doing, and even get a decent idea of why you chose the letters you did, but there's no need to put the people reading your code through that.

When you divide two ints, the result will be an int and thus rounded down if it has to. (int) Math.floor(a / b) can be written as simply a / b if a and b are ints.

When you initialise the variables, you do more or less the same logic as you do inside the loop later. I'd usually recommend keeping that logic in just one place to make it easier to change if necessary, with values in mk, ma and ms that will always be replaced on the first pass of the loop (though that does admittedly fail if one Krusty-burger takes longer to eat than the time you have available - more on that in a moment)

If there's a solution that leaves 0 time for beer, your algorithm will pick the first one it encounters, which might not be the one where you eat the most burgers. The input 5 1 15 should return 15 (as you can eat 15 entire Apu-burgers) but your code would return 3 (as eating 3 Krusty-burgers is the first solution you test, and that also leaves no time for beer). To get around this you could either iterate through all options every time, or you could have your loop start from "maximise whichever-burger-Homer-eats-faster" (which could be either of the two types) instead of "maximise Krusty-burgers".

If you do decide to go with the latter, you can end a loop early using the break statement. Your

if ( dif < ms && ms > 0) {
    mk = nk;
    ma = na;
    ms = dif;
}

could instead have been

if (dif < ms) {
    mk = nk;
    ma = na;
    ms = dif;
    if (dif == 0) break; // Don't bother going through the rest of the loop at all!
}

The Filter7 class could probably be removed in favour of using a javax.swing.filechooser.FileNameExtensionFilter.

It's usually considered good practice to define variables only in the block that needs them - the nk, na and dif variables can be moved into the loop pretty easily.

Both of those while loops seem to me like they'd be better suited as for loops. There's simple initialisation that really doesn't matter outside of the loop itself, and simple statements you want to execute as part of just keeping the loop going rather than as part of the logic within the loop.

If you do all this, you might end up with something like:

import java.io.BufferedReader;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.sql.Date;
import java.text.SimpleDateFormat;

import javax.swing.JFileChooser;
import javax.swing.JOptionPane;
import javax.swing.filechooser.FileNameExtensionFilter;

public class ChallangePerformance {

    public static void main(String[] args) throws IOException
    { 
        // This try-with-resources syntax is usually considered good practice because it takes care of closing files for you - not really an issue in a program like this though.
        try (BufferedReader rdFont = new BufferedReader(new FileReader(openFile()))) {
            long start = System.currentTimeMillis();  

            for (String line = rdFont.readLine(); line != null; line = rdFont.line()) { // Alternatively, `for (String line; (line = rdFont.readLine()) != null;)` also works if you want to be all fancy.
                String[] array = line.split(" ");           
                int krustyBurgerTime = Integer.parseInt(array[0]); // Used to be kb
                int apuBurgerTime = Integer.parseInt(array[1]); // Used to be ab
                int initialTime = Integer.parseInt(array[2]); // Used to be total

                boolean krustyBurgersAreFastest = krustyBurgerTime <= apuBurgerTime;
                int fastestBurgerTime;
                int slowestBurgerTime;
                if (krustyBurgersAreFastest) {
                    fastestBurgerTime = krustyBurgerTime;
                    slowestBurgerTime = apuBurgerTime;
                } else {
                    fastestBurgerTime = apuBurgerTime;
                    slowestBurgerTime = krustyBurgerTime;
                }

                // Sensible defaults in case there's not enough time to eat even a single burger - no need to duplicate the loop's logic as the loop will replace these anyway.
                int bestBeerTime = initialTime; // Used to be ms
                int bestFastBurgers = 0; // Used to be mk
                int bestSlowBurgers = 0; // Used to be ma

                // Replaced while loop with for loop
                for (int fastBurgers = initialTime / fastestBurgerTime; fastBurgers > 0; fastBurgers--) {
                    int leftover = initialTime - fastBurgers * fastestBurgerTime;
                    int slowBurgers = leftover / slowestBurgerTime;
                    int beerTime = initialTime - (fastBurgers * fastestBurgerTime + slowBurgers * slowestBurgerTime);
                    if ( beerTime < bestBeerTime && bestFastBurgers + bestSlowBurgers < fastBurgers + slowBurgers) {
                        bestFastBurgers = fastBurgers;
                        bestSlowBurgers = slowBurgers;
                        bestBeerTime = beerTime;
                        if (beerTime == 0) break; // Since the first perfect solution will be the best perfect solution, we can stop here.
                    }
                }

                if (krustyBurgersAreFastest) {
                    System.out.println(" " + bestFastBurgers + " " + bestSlowBurgers);
                } else {
                    System.out.println(" " + bestSlowBurgers + " " + bestFastBurgers);
                }
            }


            long end  = System.currentTimeMillis();   
            System.out.println(new SimpleDateFormat("ss.SSS").format(new Date(end - start)));  

        } catch (FileNotFoundException ex) {
            JOptionPane.showMessageDialog( null, "Invalid file name", "Invalid file name", JOptionPane.ERROR_MESSAGE );
        }
    }

    private static File openFile() {
        JFileChooser fileChooser = new JFileChooser();

        fileChooser.setFileSelectionMode( JFileChooser.FILES_ONLY );

        fileChooser.addChoosableFileFilter( new FileNameExtensionFilter("*.est", "est") );
        int result = fileChooser.showOpenDialog( null );

        if( result == JFileChooser.CANCEL_OPTION ) {
            return null;
        }

        return fileChooser.getSelectedFile();
    }
}
\$\endgroup\$
  • \$\begingroup\$ 'or you could have your loop start from "maximise whichever-burger-Homer-eats-faster" (which could be either of the two types) instead of "maximise Krusty-burgers".' I would even say that "eat as many of the fastest burgers as possible" is always the optimal solution - you will never have enough time left to eat another slow burger, as that would mean you also have time to eat one (or more!) fast burgers \$\endgroup\$ – Theoriok Jun 18 at 6:55
3
\$\begingroup\$

General

It's a good idea to give it a day or two before you accept an answer. Accepting too early discourages other people from answering.

Use @Override when overriding methods in a parent type. This makes the code easier to read and helps the compiler to warn you when something is amiss.

Your variable names are mostly meaningless, which makes your code hard to read and understand. That makes it more likely somebody editing it in the future will make a mistake. Use clear, meaningful variable names.

Your use of whitespace is inconsistent and non-idiomatic. There should be whitespace between a control flow keyword (if, for, ..) and the opening parenthesis. There should be whitespace on either side of a binary operator (+, -, ..). There should be no whitespace after an opening parenthesis. Again, this makes the code easier to read.

You should use guard clauses to return early and keep your code relatively flat. It's easy to get lost in code that looks like a >.

It's confusing that you have one method name in Portuguese and the rest in English.

ChallangePerformance

Challenge is misspelled.

Curly braces belong on the same line as the method declaration.

In java, we use camelCase, not snake_case.

All your math and looping is extraneous. The maximum number of burgers that can be consumed is time / min(timeToEatBurger1, timeToEatBurger2). The amount of time left over is time % min(timeToEatBurger1, timeToEatBurger2).

The still_has variable isn't necessary. Just break out of the loop.

Your handling of readers is dangerous. You should always make sure that readers get closed in a finally block or, preferably, using a try-with-resources.

Don't use class variables to save state that you need to pipe from one internal method to another. Use return variables from the methods and pass the values.

With some refactoring, you can loop over the reading of lines. The canonical way to do that is String line; while ((line = bufferedReader.readLine() != null) {

Declare arrays as String[] array, not String array[]. Both are legal, but the first is idiomatic and the second is rare.

openFile doesn't really need a Filter7 as a variable.

openFile really wants to check for ACCEPT_OPTION, since both CANCEL_OPTION and ERROR_OPTION are bad cases handled the same way. And if you modify the method to return the File, you can just always return getSelectedFile(), which will return null in either of those cases.

openFont is confusing. A font in English is a specific thing unrelated to your usage here.

Remove system-generated TODOs.

Filter7

The name of this class is not meaningful. Why 7? Why not EstFilter?

Hopefully Filter7 is defined in its own file. If not, it should be either a member of ChallengePerformance or defined in a separate file. While it's permissible to declare multiple classes in the same file, it's highly frowned upon and potentially problematic.

if statements don't need a ; at the end.

It appears that getExtensao() is designed to be used only by Filter7. In that case, it should be private. Try to minimize the scope of variables and methods wherever possible.

The second half of accept could be written return "est".equalsIgnoreCase(getExtensao(file));

getExtensao would be a little cleaner if you passed in a filename rather than a file.

The condition in the getExtensao if clause would be clearer if you used optional parentheses.

With all these modifications, your code might look more like:

public class ChallengePerformance {

    public static void main(final String[] args) {

        final File archive = findArchive();
        if (archive == null || archive.getName().trim().isEmpty()) {
            JOptionPane.showMessageDialog(null, "Invalid file name", "Invalid file name", JOptionPane.ERROR_MESSAGE);
            return;
        }

        final long start = System.currentTimeMillis();

        try (final FileReader fileReader = new FileReader(archive);
                final BufferedReader bufferedReader = new BufferedReader(fileReader)) {

            String line;
            while ((line = bufferedReader.readLine()) != null) {
                final String[] array = line.split(" ");
                final int timeToEatKrustyBurger = Integer.parseInt(array[0]);
                final int timeToEatApuBurger = Integer.parseInt(array[1]);
                final int timeToEatFastestBurger = Math.min(timeToEatKrustyBurger, timeToEatApuBurger);

                final int timeAvailable = Integer.parseInt(array[2]);
                final int burgersEaten = timeAvailable / timeToEatFastestBurger;
                final int beersDrunk = timeAvailable % timeToEatFastestBurger;

                System.out.println(line);
                System.out.println(" " + burgersEaten + " " + beersDrunk);
            }
        } catch (final IOException e) {
            e.printStackTrace();
        }

        final long end = System.currentTimeMillis();
        System.out.println(new SimpleDateFormat("ss.SSS").format(new Date(end - start)));

    }

    private static File findArchive() {
        final JFileChooser fileChooser = new JFileChooser();
        fileChooser.setFileSelectionMode( JFileChooser.FILES_ONLY );
        fileChooser.addChoosableFileFilter(new EstFilter());

        return fileChooser.getSelectedFile();
    }
}

With a filter class:

final class EstFilter extends FileFilter {

    @Override
    public boolean accept(final File file) {
        if (file == null) {
            return false;
        }

        if (file.isDirectory()) {
            return true;
        }

        return "est".equalsIgnoreCase(getExtension(file.getName()));
    }

    /**
     * Returns which extensions can be chosen
     */
    @Override
    public String getDescription() {
        return "*.est";
    }

    /**
     * Returns the part with the extension of an archive
     */
    private String getExtension(final String filename) {
        if (filename == null) {
            return null;
        }

        final int extensionIndex = filename.lastIndexOf('.');
        if ((extensionIndex > 0) && (extensionIndex < filename.length() - 1)) {
            return filename.substring(extensionIndex + 1).toLowerCase();
        }

        return null;
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.