Αρχιτεκτονική με Java κληρονομικότητα

[Pekovits]

Χαίρετε παιδιά,

Έχω 3 Java κλάσεις οι οποίες ανοίγουν ένα αρχείο, διαβάζουν τα περιεχόμενα του και κάποια συγκεκριμένα που δεδομένα που θέλω από το αρχείο τα γράφω σε ενα καινούργιο. Ο κώδικας είναι ο παρακάτω:

 public class RatingsDistribution {
	
	public static void main(String[] args){
		// file to read the data from
		String filename = "/home/fykos/Documents/LancasterMSc/Programming/Java/FinalCoursework/MovieLens/u.data";
		
		//file to write the new data to
		String newfile = "/home/fykos/Documents/LancasterMSc/Programming/Java/FinalCoursework/ratingmovies.csv";
		
		try {
			BufferedReader reader = new BufferedReader(new FileReader(filename));
			HashMap<String, Integer> counts = new HashMap<String, Integer>();
			CSVWriter writer = new CSVWriter(new FileWriter(newfile), ',');
			String line;
			int sum = 0;
			/**
			 * read the file until there is not line
			 * read the second column which is the movies
			 * and count the frequency of the movies which means that the times a movie exists the relevant ratings
			 * it will have
			 */
			while((line = reader.readLine()) != null){
				String tok[] = line.split("\\s");
				String movie = tok[1];
				Integer frequency = 0;
				
				// count and store in hashmap
				if (counts.containsKey(movie)) {
					frequency = counts.get(movie);
					counts.put(movie, frequency + 1);
				} else {
					counts.put(movie, new Integer(1));
				}
				
				// sum of all the ratings over time
				int rating = Integer.parseInt(tok[2]);
				sum += rating; 
				
			}
			System.out.println(sum);
			
			Set set = counts.entrySet();
			Iterator iterator = set.iterator();
			while (iterator.hasNext()) {
				Map.Entry mentry = (Map.Entry) iterator.next();
				System.out.println("key: " + mentry.getKey() + " || entry: " + mentry.getValue());
				
				// cast the movie which is an object at first to a string
				String movies = (String) mentry.getKey();
				
				// mentry.getValue() is of type Object and cast it to int
				// convert integer to string
				String ratings = Integer.toString((int)mentry.getValue());
				// concatenate the string and split it in the comma and write it to the file	
				String rec = movies + "," + ratings;
				String[] record = rec.split(",");
				writer.writeNext(record);
			}	
			reader.close();
			writer.close();
		} catch (Exception e) {
			System.out.println(e.getMessage());
			e.printStackTrace();
		}
	}

}

Οι άλλες δυο κλάσεις είναι αρκετά όμοιες. θα ήθελα να ξέρω αν μπορώ να συμπεριλάβω κληρονομικότητα σε αυτό το προτζεκτ ώστε αργότερα να μπορώ να βάλω και άλλες κλάσεις που κάνουν κάτι παρομοιο. Η ιδέα που έχω είναι να βάλω το HashMap, BufferedReader, CSVWriter σε ενα interface και οι άλλες κλάσεις θα πρέπει να το κάνουν implements ώστε να τα χρησιμοποιήσουν.

[konc]

Η ιδέα και η προσέγγιση γενικά είναι η σωστή.
Μερικές παρατηρήσεις:
-Στα interfaces βάζουμε method definitions (και σταθερές) που οι classes που τα κάνουν implement πρέπει να υλοποιήσουν εξ'ολοκλήρου.
-Εφόσον θέλεις να επαναχρησιμοποιήσεις κώδικα, ο τρόπος είναι να έχεις τον κοινό κώδικα σε μια class που θα κάνεις subclass (extends...). Ανάλογα το ποσοστό του κοινού κώδικα μπορείς μετά να τον σπάσεις σε περισσότερες methods για να σε διευκολύνει να κάνεις override κάποιες στην parent class.

[WAntilles]

Μία παρατήρηση που μπορεί να είναι και λάθος:

Τα reader.close και writer.close δεν θα έπρεπε να τα έχεις στο (προαιρετικό) τμήμα finally του try, που τώρα δεν υπάρχει καθόλου;

[konc]

Μία παρατήρηση που μπορεί να είναι και λάθος:

Τα reader.close και writer.close δεν θα έπρεπε να τα έχεις στο (προαιρετικό) τμήμα finally του try, που τώρα δεν υπάρχει καθόλου;

Καθόλου λάθος, είναι και άλλα διάφορα μικρά αλλά εντάξει η ερώτηση αφορούσε πιο γενικό design θέμα

[Pekovits]

Καλησπέρα,

Άκουσα τις προτάσεις σας και τις συμβουλές και σχεδίασα τις classes μου αναλόγως. Διόρθωσα και την παρατήρηση που έκανε το WAntilles.
Παρακάτω είναι ένα μικρό μέρος της εργασίας μου, πιστεύετε ότι μπορεί να βελτιωθεί πιο πολύ; Υπάρχουν κάποια κομμάτια κώδικα που μπορώ να τα σπάσω σε ποιο μικρά αλλά δεν είμαι σίγουρος αν χρειάζεται.

Έχω ορίσει αυτή την κλάση σαν superclass

package finalCourseworkNew;

import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;

import au.com.bytecode.opencsv.CSVWriter;

public class NewRatingsDistribution {
	private String filename = "/home/fykos/Documents/LancasterMSc/Programming/Java/FinalCoursework/MovieLens/u.data";
	
	public NewRatingsDistribution() {

	}
	
	public String getFile(){
		return filename;
	}
	
	public void setFile(String filename){
		this.filename = filename;
	}

	/**
	 * This method reads a file with movie data and uses a HashMAp to find the frequency of the
	 * movies in the file
	 * @return
	 */
	public HashMap<String, Integer> readFile() {
		HashMap<String, Integer> counts = new HashMap<String, Integer>();
		BufferedReader reader = null;
		try {
			reader = new BufferedReader(new FileReader(filename));
			String line;

			while ((line = reader.readLine()) != null) {
				String tok[] = line.split("\\s");
				String movie = tok[1];
				Integer frequency = 0;

				if (counts.containsKey(movie)) {
					frequency = counts.get(movie);
					counts.put(movie, frequency + 1);
				} else {
					counts.put(movie, new Integer(1));
				}
				
				// sum of all the ratings over time
//				int rating = Integer.parseInt(tok[2]);
//				sum += rating; 
			}
			// using the generic Exception class to catch all the occuring exceptions
		} catch (Exception e) {
			e.printStackTrace();
			System.out.println(e.getMessage());
		}finally {
			try {
				reader.close();
			} catch (IOException e) {
				e.printStackTrace();
			}
		}
		return counts;
	}
	
	/**
	 * This method goes through the HashMap supplied and extract the keys and values and writes 
	 * them to a csv file for further processing.
	 * @param map
	 */
	public void writeToFile(HashMap<String, Integer> map, String newfile) {
		CSVWriter writer = null;
		
		try {
			writer = new CSVWriter(new FileWriter(newfile), ',');
			
			for(Map.Entry e : map.entrySet()){
//				System.out.println(e.getKey() + "-->" + e.getValue());
				
				// cast the movie which is an object at first to a string
				String movies = (String) e.getKey();
				
				// mentry.getValue() is of type Object and cast it to int
				// convert integer to string
				String ratings = Integer.toString((int)e.getValue());
				
				// concatenate the string and split it in the comma and write it to the file	
				String rec = movies + "," + ratings;
				String[] record = rec.split(",");
				writer.writeNext(record);
			}
		} catch (IOException e) {
			e.printStackTrace();
			System.out.println(e.getMessage());
		}finally {
			try {
				writer.close();
			} catch (IOException e) {
				e.printStackTrace();
			}
		}
	}
}

Αυτή είναι μια απο τις subclasses

package finalCourseworkNew;

import java.io.BufferedReader;
import java.io.FileReader;
import java.io.IOException;
import java.util.HashMap;

public class NewUserRatings extends NewRatingsDistribution {
	
	public NewUserRatings(){
		super();
	}
	
	public HashMap<String, Integer> readFile() {
		HashMap<String, Integer> counts = new HashMap<String, Integer>();
		BufferedReader reader = null;
		String filename = getFile();
		try {
			reader = new BufferedReader(new FileReader(filename));
			String line;

			while ((line = reader.readLine()) != null) {
				String tok[] = line.split("\\s");
				String movie = tok[0];
				Integer frequency = 0;

				if (counts.containsKey(movie)) {
					frequency = counts.get(movie);
					counts.put(movie, frequency + 1);
				} else {
					counts.put(movie, new Integer(1));
				}
			}
			// using the generic Exception class to catch all the occuring exceptions
		} catch (Exception e) {
			e.printStackTrace();
			System.out.println(e.getMessage());
		}finally {
			try {
				reader.close();
			} catch (IOException e) {
				e.printStackTrace();
			}
		}
		return counts;
	}
}

Κοινή main που τρέχει όλες τις classes

package finalCourseworkNew;

import java.util.HashMap;

public class MainClass {
	public static void main(String[] args){
		NewRatingsDistribution rt = new NewRatingsDistribution();
		HashMap<String, Integer> map = rt.readFile();
		String newfile = "/home/fykos/Documents/LancasterMSc/Programming/Java/ratingmovies.csv";
		rt.writeToFile(map, newfile);
		
		NewUserRatings user = new NewUserRatings();
		HashMap<String, Integer> map2 = user.readFile();
		String file = "/home/fykos/Documents/LancasterMSc/Programming/Java/userratings.csv";
		user.writeToFile(map2, file);
	}
}

[konc]

Εμένα μου κάνει για εργασία, δε βλέπω κάτι σοβαρά λάθος. Από παρατηρήσεις/γενικά σχόλια:
-Γενικά προτίμησε το χαμηλότερο δυνατό scope σε μεταβλητές/συναρτήσεις (εκτός getters/setters) και αν υπάρχει ανάγκη τότε αύξανέ το. Όχι όλα public
-Στην ερώτησή σου αν χρειάζεται να σπάσεις περισσότερο τον κώδικα: Οτιδήποτε είναι >2 γραμμές και χρησιμοποιείται περισσότερες από μια φορές, καλό είναι να γίνεται χωριστή μέθοδος. Προσωπικά την πρώτη φορά που θα χρειαστεί να επαναχρησιμοποιήσω κώδικα, τότε τον σπάω.
-Εκτός από το ότι έκανες και μια design άσκηση χρησιμοποιώντας superclasses-subclasses, στο συγκεκριμένο παράδειγμα οι διαφορές στην overriden μέθοδο είναι τόσο μικρές που θα μπορούσε να είχε γίνει και χωρίς όλο αυτό, απλά παραμετροποιώντας την, πχ
public HashMap<String, Integer> readFile(int movieIndex, boolean shouldCalculateSum)
-Κάνε συνήθεια τα null checks. Μπόλικα πράγματα μπορούν να πάνε λάθος εδώ, μαζί και μερικά που ίσως δεν είχες φανταστεί, πχ ότι String + String + null = null

[WAntilles]

Για το finally που είχα πει δεν εννοούσα να κάνεις νέο try.

Εννούσα κάτι τέτοιο:

finally
{
  if reader!=null then
    reader.close;
  if writer!=null then
    writer.close;
}

Αυτά με την προϋπόθεση ότι τα αντίστοιχα open στα reader και writer γίνονται στους κονστράκτορές τους (γιατί δεν βλέπω πουθενά να τα κάνεις εσύ - δεν έχω μπροστά μου τώρα και το documentation της Java), οπότε ουσιαστικά με τα παραπάνω if ελέγχεις αν έχει γίνει το open.

Πάντως την λογική την κατάλαβες πιστεύω, ακόμα κι αν δεν βρίσκονται στους κονστράκτορες, θα πρέπει να ελέγξεις αν έχει γίνει το open και τότε μόνο να κάνεις close. Θεωρώ ότι δεν χρειάζεσαι νέο try μέσα στο finally.

[nikospara]

Μία παρατήρηση που μπορεί να είναι και λάθος:

Τα reader.close και writer.close δεν θα έπρεπε να τα έχεις στο (προαιρετικό) τμήμα finally του try, που τώρα δεν υπάρχει καθόλου;

Όχι απλά δεν είναι λάθος, είναι απαραίτητο σαν πρακτική. Μάλλιστα αν χρησιμοποιείς Java >= 7 (δε βλέπω το λόγο να μην χρησιμοποιείς) έχουν ειδική σύνταξη για να διευκολύνουν αυτήν ακριβώς την περίπτωση - try with resources:

try( 	FileReader fr = new FileReader(filename);
	BufferedReader reader = new BufferedReader(fr);
	FileWriter fw = new FileWriter(newfile);
	CSVWriter writer = new CSVWriter(fw, ',')
) {
	...
}

Το παραπάνω θα κλείσει αυτόματα τα resources σε finally, χωρίς πολυλογίες στον κώδικά μας.