3
\$\begingroup\$

The function hash_blog is inspired by "how git stores its objects". It is supposed to take a String, append a header to it, compute the SHA1 of it, then compress it and write it to a file. The first two characters of SHA1 is a directory name, and the rest of the characters are the file name.

Since I am learning the language, I am looking for general review. Is my code idiomatic Java code? Are there parts where I could make the code better?

package yas;

import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStream;

import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.zip.DeflaterOutputStream;
// import java.util.zip.Inflater;

public class Yas {

    public static void main(String[] args) {
        try {
            hash_blob("yasar arabaci");
        } catch (NoSuchAlgorithmException e) {
            e.printStackTrace();
        }
    }

    /**
     * @param args
     * @throws NoSuchAlgorithmException
     */
    public static String hash_blob(String content) throws NoSuchAlgorithmException {

        String hash_string; // hold sha1 hash
        String input = "blob " + content.length() + '\0' + content;

        // Step 1 is to compute sha1
        MessageDigest mDigest = MessageDigest.getInstance("SHA1");
        byte[] result = mDigest.digest(input.getBytes());
        { // to create namespace for StringBuilder
            // This code block was borrowed from internet ;)
            StringBuilder sb = new StringBuilder();
            for (int i = 0; i < result.length; i++) {
                sb.append(Integer.toString((result[i] & 0xff) + 0x100, 16).substring(1));
            }
            hash_string = sb.toString();
        }
        System.out.println("hash string is " + hash_string);
        // Step 3 is to write to a file

        File dest_file;
        {
            File yas_dir = new File(System.getProperty("user.dir"), ".yas");
            File object_dir = new File(yas_dir, "objects");
            File dest_dir = new File(object_dir, hash_string.substring(0, 2));
            dest_dir.mkdirs();
            dest_file = new File(dest_dir, hash_string.substring(2));
            System.out.println("dest file = " + dest_file);
        }
        try {
            if (!dest_file.exists()) {
                dest_file.createNewFile();
            }
            try (OutputStream out = new DeflaterOutputStream(new FileOutputStream(dest_file))) {
                out.write(input.getBytes());
            }


        } catch (IOException e) {
            e.printStackTrace();;
        }
        return "Dummy Return";

    }
}
\$\endgroup\$
  • \$\begingroup\$ Beginning to learn the language or beginning to learn Object Oriented programming as well? \$\endgroup\$ – bowmore Aug 7 '13 at 17:57
  • \$\begingroup\$ @bowmore I have used Python before, I know the basics of OOP, however I am not much of a fan of OOP :) \$\endgroup\$ – yasar Aug 7 '13 at 18:02
  • 1
    \$\begingroup\$ Then why adopting an OO language? \$\endgroup\$ – bowmore Aug 7 '13 at 18:13
  • \$\begingroup\$ @bowmore Because I was curious what Java would be like since it is a quite popular language. \$\endgroup\$ – yasar Aug 7 '13 at 18:23
2
\$\begingroup\$

Problems :

  • methods mkdirs() and createNewFile() return booleans to indicate whether they are successful. You should check and handle if they return false.
  • IOException gets ignored, you print the stacktrace but clients have no way of knowing the method failed.

Style :

  • This is one procedural block in which different concerns ( finding and creating files, directories, computing hash, converting byte[] to a String, ... ) are handled. Despite an attempt to limit the of variables scope with a block statement, variable scope is unclear and often wider than strictly needed. There is but one cure : refactor : extract method.
  • variable naming does not follow Java conventions.
  • Don't return a dummy return value, declare the method to have void as return value, and you don't need to return a value.
  • Comments : step 1, step 3 what happened to step 2?
  • printStackTrace() statement ends with ;; only one is needed. (probably a typo)
  • loop over the byte array can be a foreach loop :

like this :

for (byte aResult : computeSha1Hash(input)) {
    builder.append(Integer.toString((aResult & 0xff) + 0x100, 16).substring(1));
}
  • coupling should be reduced. The hashing algorithm (sha1), output mechanism (files) and byte[] to String encoding are all hardwired.
  • use of magic numbers; replace with explaining constants.
\$\endgroup\$
  • \$\begingroup\$ Thanks for the input. I have refactored the code and added second version. \$\endgroup\$ – yasar Aug 8 '13 at 15:12

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.