Added TODOs, some minor cosmetic improvements

8 files changed, 30 insertions, 20 deletions

diff --git a/src/main/java/org/openslx/imagemaster/App.java b/src/main/java/org/openslx/imagemaster/App.java
index c2b36c0..ea29e5c 100644
--- a/src/main/java/org/openslx/imagemaster/App.java
+++ b/src/main/java/org/openslx/imagemaster/App.java
@@ -41,8 +41,6 @@ public class App
 		}
 		log.info( "Loaded config file" );
 		
-		ImageProcessor.checkUploading();
-
 		// Create binary listener
 		Thread t;
 		t = new Thread( new BinaryListener(), "BinaryListener" );
diff --git a/src/main/java/org/openslx/imagemaster/Globals.java b/src/main/java/org/openslx/imagemaster/Globals.java
index 3eca269..f1be4a6 100644
--- a/src/main/java/org/openslx/imagemaster/Globals.java
+++ b/src/main/java/org/openslx/imagemaster/Globals.java
@@ -33,6 +33,9 @@ public class Globals
 
 	public static boolean propertiesValid()
 	{
+		// TODO: Some of these might legitimately be empty (but not null).
+		// Maybe use Util.notNullFatal on those so you can easily add an error message
+		// telling which option is missing. Add Util.notNullOrEmptyFatal if you feel like it...
 		if ( getImageDir() == null
 				|| getImageDir().isEmpty()
 				|| getLdapHost() == null
@@ -97,6 +100,9 @@ public class Globals
 	}
 	
 	/* INTEGERS */
+	// TODO: Use parseInt not valueOf so we don't instantiate Integers all the time
+	// TODO: Either way might throw an exception if not parsable as integer.
+	// Maybe write a Util method that tries Integer.parseInt and returns 0/-1 on exception.
 	
 	public static int getLdapPort() {
 		return Integer.valueOf( properties.getProperty( "ldap_port" ) );
diff --git a/src/main/java/org/openslx/imagemaster/crcchecker/CRCChecker.java b/src/main/java/org/openslx/imagemaster/crcchecker/CRCChecker.java
index 29b7090..1baa994 100644
--- a/src/main/java/org/openslx/imagemaster/crcchecker/CRCChecker.java
+++ b/src/main/java/org/openslx/imagemaster/crcchecker/CRCChecker.java
@@ -8,6 +8,7 @@ import java.util.zip.CRC32;
 import org.apache.log4j.Logger;
 import org.openslx.imagemaster.Globals;
 
+// TODO: Move to master-sync-shared, sattelite daemon might want to check images too
 
 public class CRCChecker
 {
@@ -19,7 +20,7 @@ public class CRCChecker
 	 * @param imageFile The imageFile to check
 	 * @param crcFile The crcFile to check against
 	 * @param blocks The blocks to check
-	 * @return The blocks where the crc was right
+	 * @return List of blocks where the crc matches, or null if the crc file is corrupted
 	 */
 	public static List<Integer> checkCRC(String imageFile, String crcFile, List<Integer> blocks) throws IOException
 	{
@@ -30,12 +31,15 @@ public class CRCChecker
 		
 		log.debug( "Checking image file: '" + imageFile + "' with crc file: '" + crcFile + "'");
 		try {
-			if (!crc.isValid()) return result;	// TODO: this needs to be handled in another way
+			if (!crc.isValid()) return null;
+			// TODO: also return null if the crc file contains the wrong number of checksums (only makes sense if the upload is complete)
 		} catch (IOException e) {
 			throw new IOException( "Could not read CRC file", e );
 		}
 		
 		// file is smaller than one block - no need to check crc yet
+		// TODO: Needs more logic, if the image is complete and < 16MB, we still need to check the block.
+		// The caller should make sure to only check crc of blocks that are finished downloading
 		if (image.length() < Globals.blockSize) {
 			return result;
 		}
@@ -43,6 +47,9 @@ public class CRCChecker
 		for (Integer blockN : blocks) {
 			byte[] block;
 			try {
+				// TODO: For performance improvements maybe prealloc the array outside the loop with a size of Globals.BLOCK_SIZE
+				// and then make getBlock(blockN, block) return the actual size of the block (Should always be BLOCK_SIZE except
+				// for the last block), so you never need to create fresh byte arrays inside the loop
 				block = image.getBlock( blockN );
 			} catch ( IOException e ) {
 				throw new IOException( "Could not read image file", e );
diff --git a/src/main/java/org/openslx/imagemaster/db/DbFtpUser.java b/src/main/java/org/openslx/imagemaster/db/DbFtpUser.java
index 088109e..b30f504 100644
--- a/src/main/java/org/openslx/imagemaster/db/DbFtpUser.java
+++ b/src/main/java/org/openslx/imagemaster/db/DbFtpUser.java
@@ -3,6 +3,7 @@ package org.openslx.imagemaster.db;
 import java.sql.Timestamp;
 import java.util.List;
 
+// TODO: Still needed?
 
 public class DbFtpUser
 {
diff --git a/src/main/java/org/openslx/imagemaster/db/DbImage.java b/src/main/java/org/openslx/imagemaster/db/DbImage.java
index e3a8888..caa0f7c 100644
--- a/src/main/java/org/openslx/imagemaster/db/DbImage.java
+++ b/src/main/java/org/openslx/imagemaster/db/DbImage.java
@@ -89,11 +89,7 @@ public class DbImage
 	 */
 	public static boolean exists( String uuid )
 	{
-		if ( MySQL.findUniqueOrNull( DbImage.class, "SELECT images.UUID FROM images WHERE images.UUID = ?", uuid ) == null ) {
-			return false;
-		} else {
-			return true;
-		}
+		return getImageByUUID( uuid ) != null;
 	}
 
 	/**
@@ -135,11 +131,14 @@ public class DbImage
 		return this.UUID;
 	}
 
+	// TODO: updateLocation? Also make all these update/delete methods non-static,
+	// so you can call them on an instance you retreived earlier.
 	public static int update( String uuid, String location )
 	{
 		return MySQL.update( "UPDATE images SET images.image_path = ? WHERE images.UUID = ?", location, uuid );
 	}
 	
+	// TODO: Consistency: variable names should be lowerCamelCase
 	public static int updateMissingBlocks( String UUID, List<Integer> missingBlocks)
 	{
 		String missingBlocksList = "";
@@ -166,7 +165,7 @@ public class DbImage
 	}
 	
 	/**
-	 * Creates a package of image data of this DbImage object.
+	 * Creates an instance of the thrift ImageData class of this DbImage object.
 	 * @return The corresponding image data
 	 */
 	public ImageData getImageData() {
diff --git a/src/main/java/org/openslx/imagemaster/db/DbKey.java b/src/main/java/org/openslx/imagemaster/db/DbKey.java
index b57065f..837b9de 100644
--- a/src/main/java/org/openslx/imagemaster/db/DbKey.java
+++ b/src/main/java/org/openslx/imagemaster/db/DbKey.java
@@ -10,6 +10,7 @@ public class DbKey
 		this.bytes = bytes;
 	}
 	
+	// TODO: One class per db-table (or per join). This should just be part of DbSatellite
 	public static DbKey fromOrganization(String organization) {
 		return MySQL.findUniqueOrNull( DbKey.class, "SELECT publickey FROM satellite WHERE organization = ?",	organization );
 	}
diff --git a/src/main/java/org/openslx/imagemaster/server/ApiServer.java b/src/main/java/org/openslx/imagemaster/server/ApiServer.java
index 9c3dfb4..4fba179 100644
--- a/src/main/java/org/openslx/imagemaster/server/ApiServer.java
+++ b/src/main/java/org/openslx/imagemaster/server/ApiServer.java
@@ -133,13 +133,6 @@ public class ApiServer
 	
 	public static DownloadInfos getImage( String uuid, String serverSessionId ) throws AuthorizationException, ImageDataException
 	{
-		/*
-		 * TODO:													done
-		 * Check if server has active session			
-		 * Check if UUID exists								
-		 * Create token for downloading					
-		 */
-		
 		// first check session of server
 		if ( ServerSessionManager.getSession( serverSessionId ) == null ) {
    		throw new AuthorizationException( AuthorizationError.NOT_AUTHENTICATED, "No valid serverSessionId" );
diff --git a/src/main/java/org/openslx/imagemaster/serverconnection/ImageProcessor.java b/src/main/java/org/openslx/imagemaster/serverconnection/ImageProcessor.java
index d4ae717..f828633 100644
--- a/src/main/java/org/openslx/imagemaster/serverconnection/ImageProcessor.java
+++ b/src/main/java/org/openslx/imagemaster/serverconnection/ImageProcessor.java
@@ -14,7 +14,7 @@ import org.openslx.imagemaster.util.RandomString;
 
 public class ImageProcessor
 {
-	private static Logger log = Logger.getLogger( ImageProcessor.class );
+	private static final Logger log = Logger.getLogger( ImageProcessor.class );
 
 	/**
 	 * The amount of blocks that is return in UploadInfos (after request of satellite)
@@ -41,7 +41,7 @@ public class ImageProcessor
 		
 		String uuid = imageData.uuid;
 		
-		// check if image is already uploading
+		// check if image is already uploading TODO: what if two clients call this at the same time?
 		if ( uploadingImages.containsKey( uuid ) ) {
 			List<Integer> missing = getMissingBlocks( uuid, AMOUNT );
 			if ( missing.isEmpty() ) {
@@ -49,6 +49,9 @@ public class ImageProcessor
 				uploadDone( uuid );
 				return new UploadInfos( token, missing );
 			}
+			// TODO: proper synchronization, interface is multi threaded.
+			// should synchronize operations on the map (use concurrent map) and then synchronize on the uploading image
+			// when handing the missing blocks etc...
 			uploadingImages.get( uuid ).setLastSentBlocks( missing );
 			return new UploadInfos( uploadingImages.get( uuid ).getToken(), missing );
 		}
@@ -67,6 +70,7 @@ public class ImageProcessor
 			
 			List<Integer> missing = getMissingBlocks( uuid, AMOUNT );
 			if ( missing.isEmpty() ) {
+				// TODO: if this is empty, check if there are pendig blocks and if so, request them again
 				uploadDone( uuid );
 			}
 			uploadingImages.get( uuid ).setLastSentBlocks( missing );
@@ -110,7 +114,8 @@ public class ImageProcessor
 	/**
 	 * Checks pending uploads in database and adds them to process list again.
 	 */
-	public static void checkUploading() {
+	static
+	{
 		List<DbImage> list = DbImage.getUploadingImages();
 		for (DbImage image : list) {
 			// TODO: init updownloader class