Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've got 2 simple text files:

besede.txt

To je vsebina datoteke z besedami. V njej so razlicne besede ki imajo lahko pomen ali pa tudi ne.

skrivno.txt

4 3 19 2 3 2 4 3

I wrote a program which receives these 2 files' names as arguments. It finds pairs of numbers in skrivno.txt and the first number in the pair is the index of the word in besede.txt; the second number in the pair is the index of the letter in that word.

So in this case I get:

  • 4th word, 3rd letter - T
  • 19th word, 2nd letter - E
  • 3rd word, 2nd letter - S
  • 4th word, 3rd letter - T

So, TEST.

Here is my program that does this. I'm open to suggestions on what I could improve and do better, etc.

public class meow {
    public static void main(String args[]) throws Exception{
        Scanner sc1 = new Scanner(new File(args[0])); //besede.txt
        Scanner sc2 = new Scanner(new File(args[1])); //skrivno.txt
        int inW = 0, inL = 0; //index of word, index of line
        String text = "", code = "";
        while(sc1.hasNext()){
            text += sc1.next() + " ";
        }
        String[] text2 = text.split(" ");
        while(sc2.hasNext()){
            inW = sc2.nextInt();
            inL = sc2.nextInt();
            if(inW == 0 && inL == 0){
                code += " ";
            }
            code += text2[inW-1].charAt(inL-1);
        }
        System.out.printf("%s\n", code);
    }
}
share|improve this question

2 Answers 2

up vote 11 down vote accepted

There are quite some things you can improve here, this list may not cover everything that could be improved though:

  1. Adhere to the Java naming conventions. Classes are named in PascalCase for example, so it would be public class Meow. On other places it seems fine.

  2. Use proper indentation everywhere, this definately applies to the main method inside public class meow, as all items should be intended one to the right. This may also apply to the while loops, I would prefer to write while(sc1.hasNext()){ as while (sc1.hasNext()) { for readability.

  3. Variable names are not expensive. sc1, sc2 are dubious, but could be ok. However inW and inL are plain alarming, the fact that you need to comment to explain them is a bad sign. My suggestions are wordIndex and lineIndex.

  4. Consider using a StringBuilder over dealing with raw strings. This means for example that:

    String text = "", code = "";
    while(sc1.hasNext()){
         text += sc1.next() + " ";
    }
    

    could be rewritten as:

    StringBuilder sb = new StringBuilder();
    while (sc1.hasNext()) {
            sb.append(sc1.next()).append(' ');
    }
    String text = sb.toString();
    
  5. You have a bug in your second while-loop. You check for sc2.hasNext() and you take two times sc2.nextInt(). Your hasX() and nextX() calls should always match. This means that in the loop per iteration in total there should be two hasNextInt() calls with two nextInt() calls.

  6. The same deal about StringBuilder applies here to your second while-loop for building code.

  7. In the future you can consider switching to using the java.nio.Path API, over the old java.io.File API. I will not show more code here on how to do it, as it would completely change the structure of the program and is best used in conjunction with Java 8, which is not in the scope of this answer.

share|improve this answer
    while(sc1.hasNext()){
        text += sc1.next() + " ";
    }
    String[] text2 = text.split(" ");

You can create a List<String> to store the words in the your first file, and use list.get(i) operation to retrieve the desired element when reading your second file.

Full suggestion (by breaking steps into methods):

public class Main {

    private List<String> wordList = new ArrayList<>();

    public Main( final File wordFile ) {
        parseWordFile( wordFile );
    }

    private void parseWordFile( final File wordFile ) {
        try (Scanner scanner = new Scanner( wordFile )) {
            while( scanner.hasNext() ) {
                wordList.add( scanner.next() );
            }
        } catch( FileNotFoundException e ) {
            throw new RuntimeException( e );
        }
    }

    private Map<Integer, Integer> getLetterDerivation( final File derivationFile ) {
        Map<Integer, Integer> result = new HashMap<>();
        try (Scanner scanner = new Scanner( derivationFile )) {
            while( scanner.hasNextInt() ) {
                Integer key = Integer.valueOf( scanner.nextInt() - 1 );
                if ( scanner.hasNextInt() ) {
                    result.put( key, Integer.valueOf( scanner.nextInt() - 1 ) );
                }
            }
        } catch( FileNotFoundException e ) {
            throw new RuntimeException( e );
        }
        return result;
    }

    private String getWord( final Map<Integer, Integer> letterDerivation ) {
        if ( wordList.isEmpty() ) {
            return ""; // nothing to derive
        }
        final StringBuilder result = new StringBuilder();
        for ( final Entry<Integer, Integer> entry : letterDerivation.entrySet() ) {
            // note, following line can throw IndexOutOfBoundsException
            // wrap with try-catch if necessary
            result.append( wordList.get( entry.getKey().intValue() ).charAt( entry.getValue().intValue() ) );
        }
        return result.toString();
    }

    public String getWord( final File derivationFile ) {
        return getWord( getLetterDerivation( derivationFile ) );
    }
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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