Project Euler 22: Assigns scores for names that are imported from text file

Question

Project Euler 22

Using atext file containing over five-thousand first names, and you will calculate a score for each name. Begin by reading the names from the file and sorting them in alphabetical order. Then, you will compute an alphabetical value for each name where A is 1, B is 2, C is 3 and so on. The alphabetical value of a name is just the sum of the values of the letters in the name. To compute the name-score for a name, you simply multiply this alphabetical value by its position in the sorted list.

For example, when the list is sorted into alphabetical order, COLIN, which is worth 3 +たす 15 +たす 12 +たす 9 +たす 14 =わ 53, is the 938th name in the list.

So, the name-score for COLIN would be 938 * 53 = 49714. What is the sum total of all the name-scores in the file?

import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
public class ScoringNames {
 BufferedReader x = null;
 String location = "C:\\Users\\xxxxxxxi\\Dropbox\\"
 + "College\\COSC 1430\\Scoring Names\\names.txt";
 ArrayList<String> names = new ArrayList<String>();
 static ArrayList<Integer> scores = new ArrayList<Integer>();
 static int ns[];
 public void fillNS(){
 for (int x = 0; x < ns.length; x++){
 ns[x] = scores.get(x) * (x+1);
 }
 }
 public void printNS(){
 for (int x = 0; x < ns.length; x++)
 System.out.print(ns[x] + ", ");
 }
 public void readFile(){ //Opens file, and prints every line.
 try{
 x = new BufferedReader(new FileReader(location));
 } catch(FileNotFoundException e) {
 e.printStackTrace();
 }
 try{
 String name = x.readLine();
 while(name != null){
 //System.out.println(name);
 names.add(name);
 name = x.readLine();
 }
 } catch(IOException e) {
 e.printStackTrace();
 }
 }
 public void sortNames(){ //Sort names array in alphabetical order
 Collections.sort(names);
 }
 public void printNames(){
 for(String counter: names)
 System.out.println(counter);
 }
 public int alphValue(char x){
 return (x - 'A' + 1);
 }
 public void returnSize(){
 System.out.println(names.size());
 }
 public int nameScore(String name){
 int score = 0;
 char[] tempName = name.toCharArray();
 for (char i : tempName){
 score += alphValue(i);
 }
 return score;
 }
 public void addScores(){
 for(String x : names){
 scores.add(nameScore(x));
 }
 }
 public void printScores(){
 for(int counter: scores)
 System.out.println(counter);
 }
 public int totalNS(int[] x){
 int sum = 0;
 for(int i = 0; i < x.length; i++){
 sum += x[i];
 }
 return sum;
 }
 public static void main(String[] args) {
 ScoringNames abc = new ScoringNames(); 
 abc.readFile(); 
 abc.sortNames(); 
 //abc.printNames(); 
 abc.addScores(); 
 //abc.printScores(); 
 ns = new int[scores.size()];
 abc.fillNS(); 
 //abc.printNS();
 //System.out.println(ns[937]); //"COLIN"
 System.out.println("The sum total of all name scores is: " + 
 abc.totalNS(ns));
 }
}

• Panthy: gotcha @Heslacher
• Panthy: @Heslacher i did not know this was on project euler hahaha

Answer 1

Lots of alternative and partial rewrites have been given already. In this answer I'm going to try to focus on the strange mix of static vs non-static methods and variables, and badly named methods and variables, in addition to proper signature of methods.

At the end I'm going to present an alternative version, which might not be optimal with regards to using streams and other new-fashioned stuff, but it should work, and hopefully will illustrate some of the stuff I'm commenting upon.

Code smells in current code

When reading your code I react to the following code smells:

• Hard-coding of the filename – Since you've created a class it would be natural to give the filename as part of the constructor of this class. This would allow for various instances to read from various files. With the name hard-coded you're making it a lot harder to reuse your class, which is one of the base principles of OOP and the point of having classes.

• The x variable – Throughout your code you are repurposing the type of and the use of x. It is both a BufferedReader, int, char, String and int[]. In neither case is it aptly named, and this is plain and simply not good.

  When you've declared it in the class scope as a BufferedReader I'm expecting it to be used for this throughout the class, and not to be hidden by local scoping it to other stuff within the various methods.

  It would be a lot better if you typed out what the x is supposed to hold in each of the cases, and it wouldn't trigger that much confusion. For a general looping variable (of various types) it is common to use i, j, and so on.

• The fillNS and printNS methods, and ns variable – Yet another sample of lazy naming. Please type it out as in fillNameScores, printNameScores and nameScores.

• names and scores are disconnected – You add names to names, and sort it, and then you calculate the scores. However, if you add a name, or change when you sort the names array this will wreck your code. The tables should be connected to keep the scores together with their name.

  Alternatively you shouldn't store the scores, and only use it dynamically, to keep these from being disconnected. However, better alternatives does exist.

• public or private? – Most, if not all, your methods are public, but some of them should only be used within the context of the class creation. sortNames() should in your implementation only be called after readFile() has been executed. And readFile() shouldn't be called twice (as it doesn't reset the names array), and could possibly be set to be private. addScores doesn't need to be public either.

• returnSize() doesn't return, it prints – The name should reflect the action. This methods prints something, it doesn't return it.

• addScores() doesn't add the scores, it calculates the score – It could be argued that it adds the calculated score to the scores array, but a better name would still be calculateScores or calculateNameValues.

• The counter in printScores isn't a counter – It is a single score, not a counter counting the progress.

• Static vs class variables – In main() you start of initializing an instance using new ScoringNames(). This is fine, but then suddenly you're starting to use the static ns and scores variables. These are intermixed in the calls to the non-static class methods addScores and fillNS, and finally you pass a static array to totalNS. This is not good, and would create side effects all over the place if you tried scoring names for various files at the same time.

  Please only use static stuff related to general implementation details applicable across the instances. For example the generation of the alphabetic value of a name (aka nameScore) could be a static method, as it only relies on a String name and not anything instance specific. Similar for the alphValue method.

Alternative version

I've made an alternative version were I've cleaned up most of this, but it does introduce some other concepts as well, so let me start explaining a little on the reasoning behind it:

• Connecting name and value – As the map variants of Java doesn't allow for access by index, and since I would like to strongly connect the name and the associated value I created a class for this NameWithValue. This I made an inner class, as it isn't particular useful outside of ScoringNames.

  Making this a class allows for using List<NameWithValue> to store the names together with the values in a single list. However, it creates two new issues related to sorting and locating elements by name.

• Fix sorting – To allow for sorting using Collections.sort(names), I provided a comparator to tell Java how to compare two NameWithValue instances. This comparator method (aka compareTo) simply compares the name part and ignores the value totally.

• Locating elements by name – List<> has an indexOf method which can locate an object within the list. With no specialization this, I think, use the object reference for equality. I therefore provided an equals method, similar to the compareTo method, which test for equality of the name part of two instances of NameWithValue.

• Eased printing of the NameWithValue – To avoid repeating my self, I also added an toString method to allow for easy printing of these instances, where it include both the name and the value.

• Your alphValue and nameScore I've included into calculateValue – If this hadn't been an inner class, this could very well have been a static method. But as is, it is just an internal method use by the class.

Having this helper class, NameWithValue, eases the task of the main class quite drastically, so I provided some extra methods to allow for retrieving the name/value combo by index, or by name, so that one could verify that the COLIN is in the right place. Some other comments related to the main class:

• The new readFile – I do believe it is a good idea to traverse larger files line by line, and not read entire files into memory at once. Using the try (...) construct also allows for better file handling. This code is untested though, as I used an online java compiler to write the code.
• The someRandomNames() – This is just a method to provide some dummy data to play with. To actually do your test, replace the filename "names.txt" with a proper filename, and switch around the commenting in the constructor.
• getNameScore(), get(int) and get(String – These are just methods to show how one could provide public access to the internal lists and present information to the outside world. Note some magic might need to be done related to zero-based indexes internally, and one-based indexes in the outside world.
• Finally the getTotalSum – This method should explain it self, possibly with the exception of the incremental of the i which is done after multiplying with the name.value. If your sum is incorrect, you might need to change the initialization to i = 1. :-)
• Comments should be javadoc... – I was a little bit laxy in the end, and added single line comments to describe the methods. This should really be javadoc comments, but I used single-line comments to save a little space.

That should be more than enough text, lets see the final code:

import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
class ScoringNames {
 // Internal helper class to keep names and values in an ordinary list
 class NameWithValue implements Comparable<NameWithValue>{
 String name;
 Integer value;
 // Initializes an instance with the name, and calculated alpha value
 public NameWithValue(String name) {
 this.name = name;
 this.value = calculateValue(name);
 }
 // Used by output methods to present "name: value"
 public String toString(){
 return this.name + ": " + this.value;
 }
 // A comparator to allow for sorting of the list
 @Override
 public int compareTo(NameWithValue other) {
 return this.name.compareTo(other.name);
 }
 // An equality operator to allow for indexOf to do its magic
 @Override
 public boolean equals(Object o) {
 if (!(o instanceof NameWithValue)){
 return false;
 }
 NameWithValue other = (NameWithValue) o;
 return this.name.equals(other.name);
 }
 // Calculate the alphabetical value of the name based
 int calculateValue(String name) {
 int value = 0;
 for (char c : name.toCharArray()) {
 value += c - 'A' + 1;
 }
 return value;
 }
 }
 ArrayList<NameWithValue> names = new ArrayList<NameWithValue>();
 // Builds the sorted names list with values.
 public ScoringNames(String nameFile) {
 //readFile(nameFile); // Uncomment to read names from file
 someRandomNames(); // Comment to not add dummy names... :-D
 // After getting a list of names (with value) sort the list
 Collections.sort(names);
 }
 /**
 * Adds some random names (with values) to names, so that
 * we have something to play with
 */
 private void someRandomNames() {
 names.add(new NameWithValue("COLIN"));
 names.add(new NameWithValue("FIRTH"));
 names.add(new NameWithValue("AGAMANDER"));
 }
 /**
 * Read names from the given file, and populate names
 * with these names. Note this is still unsorted
 */
 private void readFile(String nameFile) {
 try (BufferedReader reader = new BufferedReader(new FileReader(nameFile))) {
 String name;
 while ((name = reader.readLine()) != null) {
 // name = name.substring(1, name.length() - 1); // If you need to remove quotes
 names.add(new NameWithValue(name));
 }
 }
 catch(IOException e) {
 // Do something useful if you get this exception...
 System.out.println(e);
 System.exit(1); // I'm just bailing out... :-D
 }
 }
 // Print all names with their value
 public void printNames() {
 for (NameWithValue name : names) {
 System.out.println(name);
 }
 }
 // Returns "name:value" by index
 public String get(int index) {
 return names.get(index).toString();
 }
 // Returns "[idx] name:value" located by name
 public String get(String name) {
 int index = names.indexOf(new NameWithValue(name));
 return "[" + (index + 1) + "] " + get(index);
 }
 // Returns the idx*name.value score for a given name
 public int getNameScore(String name) {
 int index = names.indexOf(new NameWithValue(name));
 return (index + 1) * names.get(index).value;
 }
 // The real deal; calculate the total score
 public int getTotalSum() {
 int i = 0, sum = 0;
 for (NameWithValue name : names) {
 sum += name.value * i++;
 }
 return sum;
 }
}
class Main {
 static public void main(String[] args) {
 ScoringNames scoringNames = new ScoringNames("names.txt");
 scoringNames.printNames();
 System.out.println("\nIndex 1 is: " + scoringNames.get(1));
 System.out.println("\n'FIRTH' returns: " + scoringNames.get("FIRTH"));
 System.out.println("\n'FIRTH' nameScore is: " + scoringNames.getNameScore("FIRTH"));
 System.out.println("\nThe total sum is " + scoringNames.getTotalSum());
 } 
}

PS! The code, as is, is available at repl.it.

Answer 2

Reading the file: you declare the BufferedReader as an instance variable while it is only used locally. Don't do that: variables should always have the smallest scope possible. In addition, resources should normally be closed in the same scope that opens them (which brings me to the point that you never close the buffered reader.)

Furthermore, you put the opening and reading in two different try-catch constructs. That makes no sense. If the first fails, you cannot expect the second to work.

Thus, using "classic" code, you should put it together as a try-with-resources block, which automatically closes the opened resource, something like this:

try (BufferedReader reader = new BufferedReader(new FileReader(location))) {
 String name;
 while((name = reader.readLine()) != null) { // classic construct ;-)
 names.add(name);
 }
}
catch(IOException e) {
 // Do something useful. Especially, do NOT continue program
 // execution, as you don't have any data anyway!
}

But as this is 2017 and the library is quite mature, you should really simply use java.nio.Files:

Files.readAllLines(Paths.get(location))

is all you need.

There are quite a few more problems in your code (especially the mixup of static and instance variables), but alas my time is limited right now.

Answer 3

Mixing static and non-static

There is no need for all those static variables in your code. Just construct the ScoringNames object in the main method and act upon that.

As for methods: methods that do not use the fields of an object should be static. If these methods are never to be used outside of the class, they should also be private

Pass arguments to methods

To be able to reuse more code, consider passing arguments to methods. One example is 'readFile'

 public void readFile(String location)
 {
 List<String> names = Files.readAllLines(Paths.get(location));
 }

Use Java's built-in Collections.

Your application screams for the use of a Map<String, Integer>, where the name is mapped to a score. But, you need to keep the keys sorted, so you will need an OrderedMap. TreeMap seems appropriate here, as it sorts its keys in natural order.

 Map<String, Integer> namesWithScores = new TreeMap<>();
 for (String name: names)
 {
 namesWithScores.put( name, nameScore(name) );
 }

Then we need to update this items in the Map, to multiply their position:

 int position =1;
 for ( Map.Entry <String, Integer> nameWithScore : namesWithScores.entrySet())
 {
 namesWithScores.put( nameWithScore.getKey(), nameWithScore.getValue() * position++ );
 }

Note that this is not the most efficient way to do this, but I think the easiest to understand.

Calculating the total score (for example: using Streams)

When all the values are in the Map, you just have to sum up all the values:

int sum = namesWithScores.values().stream().mapToInt(Integer::intValue).sum();

Or, when using a for-loop:

 sum =0;
 for (int value : namesWithScores.values())
 {
 sum += value;
 }

Proposed solution

(Note that this solution is not the fastest, nor the most decomposed. But is is readable and fairly easy to understand IMHO)

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;
public class ScoringNames
{
 private Map<String, Integer> namesWithScores = new TreeMap<>();
 private int sum;
 public void buildFrom( String location ) throws IOException
 { 
 //Read all lines
 List<String> names = Files.readAllLines( Paths.get( location ));
 //Add them with scores
 for ( String name : names )
 {
 namesWithScores.put( name, nameScore( name ) );
 }
 //Calculate the position-score
 int index = 1;
 for ( Map.Entry<String, Integer> nameWithScore : namesWithScores.entrySet() )
 {
 namesWithScores.put( nameWithScore.getKey(), nameWithScore.getValue() * index++ );
 }
 //Calculate the total sum
 sum = namesWithScores.values().stream().mapToInt( Integer::intValue ).sum();
 }
 private static int alphValue(char x)
 {
 return (x - 'A' + 1);
 }
 private static int nameScore(String name)
 {
 int score = 0;
 for (char i : name.toCharArray()){
 score += alphValue(i);
 }
 return score;
 }
 public int getSum()
 {
 return sum;
 }
 public static void main( String[] args )
 {
 String location = "C:\\Users\\xxxxxxxi\\Dropbox\\" + "College\\COSC 1430\\Scoring Names\\names.txt";
 ScoringNames scoringNames = new ScoringNames();
 try
 {
 scoringNames.buildFrom( location );
 System.out.println( "The sum total of all name scores is: " + scoringNames.getSum() );
 }
 catch ( IOException e )
 {
 System.err.println( "Could not read file from location:" + location );
 }
 }
}

Answer 4

Only a short answer about Style

• You should choose a coding style and after choosen you should stick to it. Right now you have mixed styles, e.g sometimes you are using braces {} for single statement for loops and sometimes you omit them.

  I would like to encourage you to always use them because omitting braces can lead to hidden and therefor hard to track bugs.

• Commented code is dead code and should be removed. If you need to know such stuff you should use a code revision system like svn/git

• If you are using Java >= 7 you should use the diamond operator like so

   List<String> names = new ArrayList<>();
  

Answer 5

Not a full answer, but I don't have enough reputation to comment on the actual question.

public void printScores(){
 for(int counter: scores)
 System.out.println(counter);
}

For your code to make sense and be readable, consider using the singular (if possible) of the name of the List you're iterating:

public void printScores(){
 for(int score: scores)
 Systemm.out.println(score);
}
public void printNS(){
 for (int x = 0; x < ns.length; x++)
 System.out.print(ns[x] + ", ");
}

Instead of writing your own method for printing an array, I'd recommend using the standard Java API way of doing it:

System.out.println(Arrays.toString(x));

This does not only neatly get rid of the trailing ,_ (note the space... well, underscore, because apparently SE doesn't like trailing spaces in inline code) for the last element; it is also more efficient if you were to process the resulting String further since it internally uses a StringBuilder to build the String representation of the array. The output would look like this:

[1, 2, 3, 4, 5]

Answer 6

Just for fun, I coded it up. Note that using the Streams API in Java8 gives very nice, concise but readable code, but it may be hard to read until you get used to it.

Also note that I use an ugly int[] since I can't modify those ints from within the lambdas (the code in forEachOrdered). I could've used an accumulator but that is harder to read I think.

private static int alphascore(String s) {
 return s.chars().map(c -> c - 'A' + 1).sum();
}
public static void main(String... args) throws IOException {
 int[] ints = new int[] {0, 1}; // @index 0: Score. @index 1: Current index
 // Question doesn't state whether the word list is zero-index or one-indexed?
 Files.readAllLines(Paths.get("c:/temp/p022_names.txt")).stream()
 .flatMap(s -> Stream.of(s.split(","))) // split on , and convert string[] to new stream
 .map( s -> s.substring(1, s.length()-1)) // remove leading/trailing "
 .sorted()
 .mapToInt(P22::alphascore)
 .forEachOrdered( i -> {ints[0] += i*ints[1]; ints[1]++;});
 System.out.printf("Total score: %1d based on %2d words. %n", ints[0], ints[1]-1);
}

• holroy: This is not a review of the OP's code, but rather a full blown reimplementation.
• Panthy: I don't even understand what this does :(
• Willie Wheeler: Still pretty cool though.

Answer 7

To refine @dpnmn stream answer. One should avoid shared mutability, meaning to change stream external variables within the stream processing. Therefore I propose for streams this solution. First it builds a sorted list from all available names, second it calculates and sums the name scores.

public static void main(String args[]) throws UnsupportedEncodingException, IOException {
 final List<String> sortedNames = Files.lines(Paths.get("target/classes/org/tw/namescan/names.txt").toAbsolutePath())
 .flatMap(line -> Stream.of(line.split(",")))
 .sorted()
 .collect(toList());
 int sum = IntStream.range(0, sortedNames.size())
 .map(i -> alphascore(sortedNames.get(i))*i)
 .sum();
 System.out.println(sum);
}
private static int alphascore(String s) {
 return s.chars().map(c -> c - 'A' + 1).sum();
}

• Rob Audenaerde: This is the most concise solution :)