3
\$\begingroup\$

I am very new to Java and although my code works, I know that it must be possible to write this using fewer lines of code. I am serializing multiple objects, eventually I will look into serializing to multiple files, for now, I am using just one file, so it constantly gets over-written. How can I nest or reduce the number of if statements I am using. In my code snippet, I am using an if statement for each individual object, and duplicating my try/catch block. This is horrible code, but I am stumped as to how I can write this more efficiently.

In very simple terms, please give me an example of how to nest or reduce code duplication. I shouldn't need to re-do my try/catch block constantly!

public class Serializer
{
   public void Serialize()
   {

    MainMenu pass_choice = new MainMenu();
    int passed_choice = pass_choice.chooseTeam();


    if(passed_choice==1){
    ClubInfo club = new ClubInfo();
     club.teamName = "Arsenal";
     club.stadium = "Emirates";
     club.division = "Premier League";
      club.SSN = 11122333;
      club.stadiumCapacity = 60000;

    try
      {
         FileOutputStream fileOut =
         new FileOutputStream("/home/cg/root/club.ser");
         ObjectOutputStream out = new ObjectOutputStream(fileOut);

         out.writeObject(club);

         out.close();
         fileOut.close();
         System.out.printf("Serialized data is saved in C:/tmp/club.ser");
         System.out.println("Choice is :" + passed_choice);
      }catch(IOException i)
      {
          i.printStackTrace();
      }
} // end if 1
   else
    if(passed_choice==2){
    ClubInfo club1 = new ClubInfo();
    club1.teamName = "Aston Villa";
    club1.stadium = "Villa Park";
    club1.division = "Premier League";
    club1.SSN = 111223334;
    club1.stadiumCapacity = 40000;

    try
      {
         FileOutputStream fileOut =
         new FileOutputStream("/home/cg/root/club.ser");
         ObjectOutputStream out = new ObjectOutputStream(fileOut);

         out.writeObject(club1);

         out.close();
         fileOut.close();
         System.out.printf("Serialized data is saved in C:/tmp/club.ser");
         System.out.println("Choice is :" + passed_choice);
      }catch(IOException i)
      {
          i.printStackTrace();
      }
} // end if 2
   }
}
\$\endgroup\$
3
\$\begingroup\$

I'm going to assume Java 7 for this review. I'm sure somebody else will come along with a Java 8 version that does the whole thing in half a line. :-)

Also, pretty much everything I'm saying here has already been mentioned to you in answers to your earlier questions.

Naming

Methods in java start with a lowercase letter. Variables start with a lowercase letter (ssn, not SSN) and use camelCase, not snake_case.

Formatting

Curly braces should typically not be on their own line in cases like try { and } catch (.... Use whitespace consistently - your code is indented all over the place.

Correctness

Use the try-with-resources paradigm where possible to ensure that resources get cleaned up correctly.

SSN should be a String to preserve any leading zeros.

Design

You are just on the edge of having few enough variables in ClubInfo to have a constructor that just takes them all. That will help simplify the code. A builder would be another reasonable option.

Use methods! Short methods are easy to read and can greatly reduce repetition in your code.

It's generally, though not universally, considered a bad practice to have struct-like objects with public-visibility member variables. While it's easier to develop against, it can make life hard on you later. Using accessor methods is generally preferred.

Separate the building of ClubInfo objects from their serialization. Using a Map will trade off memory footprint and slower startup performance for faster runtime performance and easier-to-read code. Note that in this case the performance differences are insignificant and should be ignored. You might also consider an enum instead.

It's unlikely that ClubInfo initialization should be in your serialization class, but this project is probably extremely small, so it's not awful. Just bear in mind the preferred design practice would be to do it elsewhere.

Putting it all together, a simpler implementation might look more like:

import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.ObjectOutputStream;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

public final class Serializer {

    private static final File OUTPUT_FILE = new File("/home/cg/root/club.ser");

    private static final Map<Integer, ClubInfo> CLUBS = buildClubs();

    public void serialize() {

        final MainMenu passChoice = new MainMenu();
        final int passedChoice = passChoice.chooseTeam();

        final ClubInfo club = CLUBS.get(Integer.valueOf(passedChoice));
        if (club == null) {
            return;
        }

        try {
            this.writeToFile(club);
        } catch (final IOException e) {
            e.printStackTrace(System.err);
            return;
        }
        System.out.printf("Serialized data is saved in " + OUTPUT_FILE);
        System.out.println("Choice is :" + passedChoice);
    }

    private void writeToFile(final ClubInfo clubInfo) throws IOException {
        try (final FileOutputStream fileOut = new FileOutputStream(OUTPUT_FILE);
                final ObjectOutputStream out = new ObjectOutputStream(fileOut)) {
            out.writeObject(clubInfo);
        }
    }

    private static Map<Integer, ClubInfo> buildClubs() {
        final Map<Integer, ClubInfo> clubs = new HashMap<>();

        clubs.put(
                Integer.valueOf(1),
                new ClubInfo("Arsenal", "Emirates", "Premier League", "11122333", 60000));

        clubs.put(
                Integer.valueOf(2),
                new ClubInfo("Aston Villa", "Villa Park", "Premier League", "111223334", 40000));

        return Collections.unmodifiableMap(clubs);
    }
}
\$\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.