4
\$\begingroup\$

First time using code review. I am still a student so there will be a lot to be desired.

Basically as part of the homework we are told what methods to create (Method names are given with their parameters). We essentially are parsing a bunch of files or raw CSV data looking for either specific names at a rank, names at a specific rank, total births, etc. The code does exactly what it is meant to and works but I feel it could seriously be improved if not minimised somehow. Feel free to be as harsh as possible, I am looking for serious feed back.

Examples of the CSV data would be:

2012(Test CSV File)

Sophia,F,10
Emma,F,9
Isabella,F,8
Olivia,F,7
Ava,F,6
Jacob,M,8
Mason,M,7
Ethan,M,7
Noah,M,6
William,M,5

2013 (Test CSV file)

Sophia,F,10
Emma,F,8
Olivia,F,8
Isabella,F,7
Ava,F,6
Noah,M,12
Liam,M,9
Jacob,M,8
Mason,M,8
William,M,7

To clarify the CSV data files do get larger. This is merely for testing purposes.

You may see that a few methods have not been implemented yet, this is merely so that I may adjust it according to any suggestions going forward. The return statements have been slightly modified from the assignment credentials, however we are allowed to do this as part of the "recommendations" is to work with ArrayLists, LinkedHashMaps, TreeMaps, etc.

Could I get some feed back on how I could improve the below code?

import edu.duke.*;
import org.apache.commons.csv.*;
import java.io.*;
import java.util.*;
/*import java.util.Array;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
*/

public class parseBabies{
    public static void main(String args[])
    {
            parseBabies namePopularity = new parseBabies();
            //namePopularity.testGetRank();
            //namePopularity.testGetName();
    //      namePopularity.testWhatIsNameInYear();
            namePopularity.testYearOfHighestRank();
    }

    void println(Object obj)
    {
            System.out.println(obj);
    }

    public CSVParser parseData(FileResource fr){
            // No header row in this CSV data, hence false
            CSVParser parser = fr.getCSVParser(false);
            return parser;
    }

    public LinkedHashMap<String,Integer> totalBirths(CSVParser parser)
    {
            LinkedHashMap<String, Integer> totalsList = new LinkedHashMap<String, Integer>();
            int totalFemales = 0;
            int totalMales = 0;
            int total = 0;
            int nameOccurances = 0;

            for(CSVRecord record : parser)
            {
                    String firstName = record.get(0);
                    String gender = record.get(1);
                    nameOccurances = Integer.parseInt(record.get(2));
                    if(gender == "M") totalMales += nameOccurances;
                    if(gender == "F") totalFemales += nameOccurances;
                    total += nameOccurances;
            }
            totalsList.put("Females",totalFemales);
            totalsList.put("Males",totalMales);
            totalsList.put("Total", total);

            println("Total females: "+ totalFemales);
            println("Total males: "+totalMales);
            println("Total births: "+ total);

            return totalsList;
    }

    public List<Map.Entry<String, Integer>> createNameRanks(FileResource fr, String gender)
    {
            CSVParser parser = parseData(fr);

            LinkedHashMap<String, Integer> maleRank = new LinkedHashMap<String, Integer>();
            LinkedHashMap<String, Integer> femaleRank = new LinkedHashMap<String, Integer>();
            LinkedHashMap<String, Integer> allNamesRank = new LinkedHashMap<String, Integer>();
            List<Map.Entry<String,Integer>> rankList = new ArrayList<Map.Entry<String, Integer>>();

            if(parser == null) return rankList;

            for(CSVRecord record : parser){
                    if(record.get(1).toUpperCase().equals("M")){
                            maleRank.put(record.get(0), Integer.parseInt(record.get(2)));
                    }
                    if(record.get(1).toUpperCase().equals("F")){
                            femaleRank.put(record.get(0), Integer.parseInt(record.get(2)));
                    }
                    allNamesRank.put(record.get(0), Integer.parseInt(record.get(2)));
            }
            switch(gender){
                    case("M"):
                            rankList = sortRankMaps(maleRank);
                            return rankList;
                    case("F"):
                            rankList = sortRankMaps(femaleRank);
                            return rankList;
                    default:
                            rankList = sortRankMaps(allNamesRank);
                            return rankList;
            }
    }

    public List<Map.Entry<String, Integer>> sortRankMaps(LinkedHashMap<String, Integer> names)
    {
            List<Map.Entry<String, Integer>> rankList = new ArrayList<Map.Entry<String, Integer>>(names.entrySet());
            Collections.sort(
                            rankList,
                            new Comparator<Map.Entry<String, Integer>>(){
                                    public int compare(Map.Entry<String, Integer> entry1,
                                                    Map.Entry<String, Integer> entry2)
                                    {
                                            return -entry1.getValue() + entry2.getValue();
                                    }
                            });
            //println(rankList);
            return rankList;
    }

    public LinkedHashMap<Integer,ArrayList<String>> getRank(FileResource fr, Integer year, String name, String gender)
    {
            List<Map.Entry<String, Integer>> nameRank = new ArrayList<Map.Entry<String, Integer>>();
            ArrayList<String> namesMatched = new ArrayList<String>();
            LinkedHashMap<Integer,ArrayList<String>> matches = new LinkedHashMap<Integer,ArrayList<String>>();

            if(fr == null){
                    namesMatched.add("NO MATCHES");
                    matches.put(0, namesMatched);
                    return matches;
            }

            int totalRanks = 0;

            int ranking = 0;

            nameRank = createNameRanks(fr, gender);

            for(Map.Entry<String,Integer> entry : nameRank)
            {
                    totalRanks++;
                    if(entry.getKey().equals(name))
                    {
                            for(Map.Entry<String,Integer> dupValues : nameRank)
                            {
                                    if(entry.getValue().equals(dupValues.getValue())
                                                    && !entry.getKey().equals(dupValues.getKey()))
                                    {
                                            namesMatched.add(dupValues.getKey());
                                    }
                                    ranking = totalRanks - (namesMatched.size());
                            }
                    }
            }
            namesMatched.add(name);
            //              println("Names Matched: " + namesMatched + " at rank: "+ranking);
            matches.put(ranking, namesMatched);
            return matches;
    }
    public void testGetRank()
    {
            FileResource fr = new FileResource();
            LinkedHashMap<Integer,ArrayList<String>> rank = getRank(fr, 2012, "Mason", "M");
    }

    public ArrayList<String> getName(Integer year, Integer rank, String gender)
    {
            FileResource fr = new FileResource();
            String nameVsRank = new String();

            List<Map.Entry<String, Integer>> nameRank = new ArrayList<Map.Entry<String, Integer>>();
            nameRank = createNameRanks(fr, gender);
            ArrayList<String> names = new ArrayList<String>();

            if(rank > nameRank.size()){
                    names.add("NO NAME");
                    return names;
            }

            int count = 0;

            for(Map.Entry<String,Integer> entry : nameRank){
                    count++;
                    if(count == rank){
                            for(Map.Entry<String, Integer> dupEntry : nameRank){
                                    if(entry.getValue().equals(dupEntry.getValue()) && !entry.getKey().equals(dupEntry.getKey()))
                                            names.add(dupEntry.getKey());
                            }
                            names.add(entry.getKey());
                    }
            }
            return names;
    }
    public void testGetName()
    {
            ArrayList<String> nameAtRank = getName(2014, 3, "M");
            println("Name at rank : "+ nameAtRank);
    }

    public ArrayList<String> whatIsNameInYear(FileResource fr, String name, Integer year, Integer newYear, String gender)
    {
            LinkedHashMap<Integer, ArrayList<String>> nameRank = getRank(fr, year, name, gender);
            ArrayList<String> namesAtRank = new ArrayList<String>();
            for(Integer key : nameRank.keySet())
            {
                    namesAtRank = getName(year, key, gender);
            }
            return namesAtRank;
    }

    public void testWhatIsNameInYear()
    {
            FileResource fr = new FileResource();
            ArrayList<String> nameInYear = whatIsNameInYear(fr,"Isabella", 2012, 2014, "F");
            println("Isabella in 2014 is equivalent to: " + nameInYear);
    }

    public ArrayList<Integer> yearOfHighestRank(Integer year, String name, String gender)
    {
            DirectoryResource dr = new DirectoryResource();
            LinkedHashMap<Integer,ArrayList<String>> nameRank = new LinkedHashMap<Integer, ArrayList<String>>();
            ArrayList<Integer> highestRankEver = new ArrayList<Integer>();

            int highestRanking = 99999;
            int highestRankingYear = year;

            if(name == null || gender == null){
                    highestRankEver.add(-1);
                    return highestRankEver;
            }

            for(File f :  dr.selectedFiles()){
                    FileResource fr = new FileResource(f);
                    nameRank = getRank(fr, year, name, gender);
                    println("FileResource: "+fr);
                    println("Name Rank: " + nameRank);
                    for(Map.Entry<Integer, ArrayList<String>> entries : nameRank.entrySet())
                    {
                          //highestRanking = entries.getKey();
                            println("Entries: " + entries);
                            println("Entries key: " + entries.getKey());

                            if(entries.getKey() < highestRanking)
                            {
                                    highestRanking = entries.getKey();
                                    highestRankingYear = year;
                                    println("Highest Rank so Far: "+ highestRanking + ", "+ highestRankingYear);
                            }
                    }
                    year++;
            }
            println("HighestRank: "+ highestRanking +" in year: "+highestRankingYear);
            highestRankEver.addAll(Arrays.asList(highestRanking, highestRankingYear));
            return highestRankEver;
    }

    public void testYearOfHighestRank(){
            ArrayList<Integer> mostPopularYear = yearOfHighestRank(2012,"Mason","M");
            println("Most Popular Year: " +mostPopularYear);
    }
}
\$\endgroup\$

3 Answers 3

1
\$\begingroup\$

For this assignment, you have been provided with a starter file which is:

/**
 * Print out total number of babies born, as well as for each gender, in a given
 * CSV file of baby name data.
 * 
 * @author Duke Software Team 
 */
import edu.duke.*;
import org.apache.commons.csv.*;

public class BabyBirths {
    public void printNames() {
        FileResource fr = new FileResource();
        for (CSVRecord rec : fr.getCSVParser(false)) {
            int numBorn = Integer.parseInt(rec.get(2));
            if (numBorn <= 100) {
                System.out.println("Name " + rec.get(0) + " Gender " + rec.get(1)
                        + " Num Born " + rec.get(2));
            }
        }
    }

    public void totalBirths(FileResource fr) {
        int totalBirths = 0;
        int totalBoys = 0;
        int totalGirls = 0;
        for (CSVRecord rec : fr.getCSVParser(false)) {
            int numBorn = Integer.parseInt(rec.get(2));
            totalBirths += numBorn;
            if (rec.get(1).equals("M")) {
                totalBoys += numBorn;
            } else {
                totalGirls += numBorn;
            }
        }
        System.out.println("total births = " + totalBirths);
        System.out.println("female girls = " + totalGirls);
        System.out.println("male boys = " + totalBoys);
    }

    public void testTotalBirths() {
        // FileResource fr = new FileResource();
        FileResource fr = new FileResource("data/yob2014.csv");
        totalBirths(fr);
    }
}

You have been tasked to write functions with a particular return type and particular parameters. Therefore, you should start from the above file, and implement functions with exactly the given signatures:

    public int getRank(int year, String name, String gender);

    public String getName(int year, int rank, String gender);

    public void whatIsNameInYear(String name, int year, int newYear, String gender);

    public int yearOfHighestRank(String name, String gender);

    public float getAverageRank(String name, String gender);

    public int getTotalBirthsRankedHigher(int year, String name, String gender);

These methods are public as they are the exposed functionality provided by your Class. Then it is a good idea if any other methods you introduce are marked as private, to make it clear that those are for internal use only and aren't intended to expose any new functionality on top of what has been asked.

So how can you modify the code you shared to have exactly those signatures?

The main change will be that you should use FileResource and load the specific filenames as needed in the code. Look at the given example of loading a specific file in testTotalBirths. Instead of passing the FileResource around in the parameters, any method that already knows the int year it needs could naturally make use of a method you could write that would take the year as a parameter and return a FileResource related to that year:

    private FileResource getFile(int year);
\$\endgroup\$
3
  • \$\begingroup\$ There is a section of the assignment that I cannot share as you require access credentials that mentioned to experiment with TreeMaps, LinkedHashMaps, etc. Hence the slight alterations. \$\endgroup\$ Apr 24 at 5:55
  • \$\begingroup\$ Oh additionally, if you take a look at the actually csv folder structure it will not grab the file according to year as the names of the relevant files will not match up. If I create a function to "Index" the relevant files, there is a fairly high probability it will not be in the order one intendes. The only way the variable year would have an impact is by the name of the directory the files are contained in but we have certain folders that go by month and day as well. Tbh I do not know why the year variable was included as it is not always unique to a file that contains a year worth of data. \$\endgroup\$ Apr 24 at 6:00
  • \$\begingroup\$ Interesting, I see the file you are referring too has different csv data from what we were given in the actual course itself. That data only makes use of yearly csv files. The files we were provided with in the actual course contain names for years(Broken up into days, months, years) and decades. \$\endgroup\$ Apr 24 at 6:17
1
\$\begingroup\$

This will be difficult to review without access to the edu.duke package so my review will be superficial.

parseBabies is a poor class name: first of all it has non-standard case, and it's made to look like a function when it's a class. Prefer instead BabyParser.

Commented-out code like

        //namePopularity.testGetRank();
        //namePopularity.testGetName();
//      namePopularity.testWhatIsNameInYear();

must be deleted.

The println function should be deleted. It offers nothing over System.out.println. If you're worried about the length of the latter, do a static import for out.

commons-csv includes a get by name. Use that instead of get by index.

Occurances is spelled Occurrences.

totalsList is not a list, so don't call it one. It's a map. But it shouldn't even be a map: the way you're using it, it should be a dedicated class with members of Females, Males and Total. A more flexible and useful method would not look specifically at F and M, but would sum up totals for any gender and return a map of gender string to total.

namesMatched.add("NO MATCHES"); seems like a bad idea, mixing error data in-band with domain data. If you need to indicate that there are no matches, this should probably be done elsewhere.

This loop:

for(Integer key : nameRank.keySet())
{
        namesAtRank = getName(year, key, gender);
}
return namesAtRank;

is strange: it reassigns namesAtRank unconditionally for every single entry in nameRank. Surely that isn't necessary, right? If you only care about the last key in the key set (for some arbitrary definition of "last"), there are better ways.

This:

println("HighestRank: "+ highestRanking +" in year: "+highestRankingYear);

should be converted to an out.printf.

\$\endgroup\$
2
  • \$\begingroup\$ Hey, You are welcome to download it if you wish. That they do allow anyone to download. Not sure how to hyperlink it but here ya go: dukelearntoprogram.com//downloads/bluej.php?course=2 Scroll down to the bottom and under cource code packages you will see the edu.duke library there. \$\endgroup\$ Apr 24 at 5:50
  • \$\begingroup\$ The get function we do have with Apache too, however if you take a look at the csv files. They do not contain header rows. You do make a perfectly valid point about my forloop. I think my logic was to merely access the key itself. \$\endgroup\$ Apr 24 at 6:05
0
\$\begingroup\$

There is some overlap with the other reviews, I've voted accordingly, if you found the other answers useful consider voting for them to let them know.

Expressiveness

Code should be expressive and focussed. Having unnecessary information in source files creates noise that distracts and hides the important elements. An easy win is removing commented out code. Having it there rarely adds any value. If you're worried about losing the code, and you're not already, you should look at using a source control system. If you haven't used any then a basic introduction to git will quickly pay back the time investment. Using an IDE will also quickly bring benefits, there's various free ones which will give you various hints to improve your programs (such as noticing variables that you declare or don't use).

parseBabies doesn't conform to the Java naming convention. It should really start with a capital ParseBabies. Classes are typically named as things, i.e. BabyParser, rather than actions (which tend to be used for methods). So, a BabyParser might have a parse method. That said, a classes name should also reflect its responsibility. Your parseBabies class doesn't just parse babies, it also performs various analytics and tests. These are different responsibilities and should really be in different classes. Breaking the program up can make it easier to reason about and understand. Method names should reflect what the method does. parseData doesn't do any parsing, it simply creates and returns a parser, this is misleading.

parseBabies has various methods named test.... These don't feel like they really belong on a parser. It's also really unclear what it is they're supposed to test. Most of them seem to do something, then print it to the screen. This might be useful to you, today, if you know what you're expecting to be output. A month down the line, or when somebody else looks at the code it's pretty useless. Is "Most Popular Year: [1, 2012]" really what's expected? Consider investing some time in learning a bit of JUnit (or similar). It allows you to programmitically state your expectations in your tests. This is useful to others and your future self. It also helps you to think about how to break up your logic, so that you're able to express what it is you're trying to test.

Mixed Responsibilities

Some of your methods like yearOfHighestRank interleave print statements with logic. Whilst this can make the program easier to understand while you're initially writing it, it is generally a bad idea. The code you end up with is usually less flexible. Seperating the logic that performs calculations from the logic that displays the calculations helps you to consider what you need to return in order to display the correct result. This makes it easier to write automated tests / change the user interface to something else (web page / forms app etc).

IDE warnings

Opening your code in an IDE (I used Intellij community edition), gives various hints that could improve your code. For example:

  • Redundant local variable. Rather than creating a variable that is immediately returned:

      CSVParser parser = fr.getCSVParser(false);
      return parser;
    

    directly return it:

      return fr.getCSVParser(false);
    
  • Method is never called. totalBirths is never called, if it's called from another source file then that can be ok, otherwise consider removing it.

  • Typo in word Occurances. Should be Occurences.

  • Variable 'nameOccurances' initializer '0' is redundant

    The value assigned in the initializer is never used, because all the code paths assign a different value into it before they use the value. The initial assignment just adds noise.

  • Variable firstName is never used. You're creating a variable and assigning it a value, but the value is never read. Do you really need the variable? Do you need to do the work required to get its value?

  • String values are compared using '==', not 'equals()'. This is performing a reference comparison, not comparing the values of the two strings. It can work, particularly with small hard-coded strings in small programs, but is unreliable. In most cases you should use .equals instead.

  • Explicit type argument String, Integer can be replaced with <>

    When you're declaring generic variables at the same time as initialising them, you don't gain anything by declaring the types on both sides of the assignment. It's generally preferable to do:

      LinkedHashMap<String, Integer> maleRank = new LinkedHashMap<>();
    
  • 'toUpperCase' call can be replaced with 'equalsIgnoreCase'

    Using the equalsIgnoreCase method makes your requirements more explicit.

  • Collections.sort could be replaced with List.sort

You know that rankList is a List, Collection.sort delegates to List.sort under the hood anyway.

  • Anonymous new Comparator<Map.Entry<String, Integer>>() can be replaced with lambda

    Newer versions of Java support lambda expressions, which are more concise, with less noise than using anonymous classes. You end up with:

     (entry1, entry2) -> -entry1.getValue() + entry2.getValue()
    

Another thing that IDEs can help with is consistent formatting. Whilst it may seem simple and unimportant, using consistent formatting makes your code easier to read. Using the automatted reformat adjusted 78 lines (that's about a third of the code).

Other thoughts

createNameRanks ultimately returns a list of sorted, gender switched ranks. It only sorts the relevant list, however does it really need to construct all three lists or could the decision be made earlier so that the irrelevant records are ignored?

There's quite a few references to year throughout the program. It's often not used at all, or assumes the user has corrected the correct files. There seems to be a missing concept to relate the year to the data being processed (it seems to be related to the name of the file). For example, yearOfHighestRank seems to assume that the user has selected a list of files in year order. Can this be relied on?

HashMaps?

It's really unclear what benefit you're currently getting from using a HashMap. It seems to have been used because it is a convenient way of associating two values (in which case you'd be better off creating a custom class just for those values). Typically, I'd expect a HashMap to be used when you're going to be performing lookups using the Key. This doesn't seem to be something you ever do. Looking at the current requirements if you want to use a HashMap, consider switching the value/key. You could then have a HashMap of <Integer, List>, i.e. a list of names at a given rank.

\$\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.