2
\$\begingroup\$

I did write the GitHub Auto Copy Committer.

The purpose is to copy an existing public GitHub repository to in another repository - commit per commit. It's reason is not a full copy of a repository. There are way easier ways for it. The key feature is the ability to redo the original commit behaviour as the original.

All necessary informations are hosted here:

https://github.com/BerndErnst/GitHubAutoCopyCommitter/

Now I would like to hear some feedback about my code.
The most interesting for me is the OOP part. What could I have done in which way better?

I'm not happy about my main function at all. It's too big and not clear to read. What are good ways to improve the main function?

Was it a good idea to store the links in a (non-included) text file?
Is it a good idea to read it linewise like this? Could have been another file structure better?

I tried to write a good JavaDoc. Every critique regarding that is appreciated.

Could I have done the try-catch-blocks better?

I'm really interested in an honest code-review. I want to improve myself.


Dump of sources:


Core.java

package gitHubAutoCopyCommitter;

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.io.InputStream;
import java.net.HttpURLConnection;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.ArrayList;

import org.joda.time.LocalTime;
import org.kohsuke.github.GHCommit;
import org.kohsuke.github.GHContent;
import org.kohsuke.github.GHRepository;
import org.kohsuke.github.GitHub;
import org.kohsuke.github.PagedIterable;

/** Core components for the whole task. The main method is also located here.
 * Necessary for working is a file which contains all the URLs including SHA of
 * the GitHub repo which should be copied.
 * 
 * @author Bernd Ernst */
public class Core {

    final private String localRepositoryName = "BerndErnst/Hanabu";
    final private String localFileName = "filelinks.txt";
    final private String commitMessageFile = "commitmessages.txt";

    /** Contains the while(true) loop.
     * 
     * @param args
     *            Requires the OAuth token as single argument */
    public static void main(String[] args) {

        if (args.length == 0) {
            System.out.println("You have to input your GitHub OAuth2 Token per command line argument!");
            System.out.println("Else this won't work!");
            System.out.println("Press enter to exit");
            try {
                System.in.read();
                System.exit(1);
            } catch (IOException e) {
                System.exit(1);
            }

        }

        Core core = new Core();
        Timer timer = new Timer();

        GitHub github = null;
        GHRepository meinRepo = null;
        String token = args[0];

        try {
            github = GitHub.connectUsingOAuth(token);
        } catch (IOException e) {
            System.out.println("Cannot access to GitHub with OAuth");
            e.printStackTrace();
        }

        try {
            meinRepo = github.getRepository(core.localRepositoryName);
        } catch (IOException e) {
            System.out.println("Cannot catch the Repo: BerndErnst/testrepos");
            e.printStackTrace();
        }

        ArrayList<LocalTime> postingTimes = timer.getPostingTimes();

        while (true) {

            if (!postingTimes.isEmpty()) {
                int secondsToNextPost = timer.getSecondsUntilNextPost(postingTimes.get(0));
                System.out.println("Minutes to next post: " + secondsToNextPost / 60);
                postingTimes.remove(0);

                try {
                    Thread.sleep(secondsToNextPost * 1000);
                } catch (InterruptedException e) {
                    System.out.println("Error at Thread.sleep...");
                    e.printStackTrace();
                }

                // ---------- Main Commit Block ------------

                if (secondsToNextPost > 1) {
                    String remoteFileName = core.getFirstLine();
                    byte[] remoteFileContent = core.getRemoteFileContent(remoteFileName);

                    core.makeCommit(meinRepo, remoteFileName, remoteFileContent, core.getCommitMessage());

                    core.removeLine();

                } else {
                    System.out.println("Skipping this post because it's already in the past!");
                    System.out.println("-------------------------------");
                }

                // ------- END Main Commit Block END ---------

            } else {
                try {
                    System.out.println("The next posting time calculation will be made in " + timer.getSecondsUntilNextPostingTimeGeneration() / 60 + " minutes");
                    Thread.sleep(timer.getSecondsUntilNextPostingTimeGeneration() * 1000);
                } catch (InterruptedException e) {
                    System.out.println("Error at Thread.sleep...");
                    e.printStackTrace();
                }
                postingTimes = timer.getPostingTimes();
            }

        }

    }

    /** Returns the next line of the file which should be commited
     * 
     * @return The next line of the file which should be commited */
    private String getFirstLine() {
        try (BufferedReader br = new BufferedReader(new FileReader(localFileName))) {

            if (br.ready()) {
                return br.readLine();
            }

        } catch (Exception e) {
            System.out.println("ERROR: Couldn't fetch the first line of the localFile");
            e.printStackTrace();
        }
        return null;
    }

    /** Get the content of the file from source repository.
     * 
     * @param remoteFileName
     *            Name of file on the source repository.
     * @return content of the file to be commited. */
    private byte[] getRemoteFileContent(String remoteFileName) {

        try {
            InputStream input = new URL(remoteFileName).openStream();
            byte[] content = new byte[getFileSize(remoteFileName)];
            input.read(content);
            return content;
        } catch (Exception e) {
            System.out.println("Error with getting the content of the remote file!");
            e.printStackTrace();
        }

        return null;
    }

    /** Get the filesize of the file to be commit for buffering purposes.
     * 
     * @param remoteFileName
     *            file on the other repository-
     * @return Size in byte of the file who should be commited. -1 if failed. */
    private int getFileSize(String remoteFileName) {
        HttpURLConnection conn = null;
        URL url = null;
        try {
            url = new URL(remoteFileName);
        } catch (MalformedURLException e1) {
            // TODO Auto-generated catch block
            e1.printStackTrace();
        }
        try {
            conn = (HttpURLConnection) url.openConnection();
            conn.setRequestMethod("HEAD");
            conn.getInputStream();
            return conn.getContentLength();
        } catch (IOException e) {
            e.printStackTrace();
            return -1;
        } finally {
            conn.disconnect();
        }
    }

    /** Picks a random commit message from a predefined file.
     * 
     * @return String of a random commit message. */
    private String getCommitMessage() {
        String commitMessage = "auto-commit";
        final int MAX_LINES_OF_FILE = 2022;

        try (BufferedReader br = new BufferedReader(new FileReader(commitMessageFile))) {

            int randomCommitLine = (int) (Math.random() * MAX_LINES_OF_FILE);

            while (randomCommitLine > 1) {
                br.readLine();
                randomCommitLine--;
            }
            commitMessage = br.readLine();
            br.close();

        } catch (Exception e) {
            System.out.println("Error in getCommitMessage()!");
            e.printStackTrace();
        }
        return commitMessage;
    }

    /** The core of the whole program. Does the actual commit in the desired
     * repository. This is the class which tends to make the most problems. Lot
     * of Debug information if something did fail.
     * 
     * @param ownRepository
     *            Repository in which the commits should happen.
     * @param remoteFilename
     *            Name of file on the own repository.
     * @param newContentBytes
     *            Actual data of an file of the commit.
     * @param commitMessage
     *            Message of the commit.
     * @return true for successful, false for commit failed */
    private boolean makeCommit(GHRepository ownRepository, String remoteFilename, byte[] newContentBytes, String commitMessage) {

        String filename = remoteFilename.substring(79);
        System.out.println(filename);

        try {
            // Update
            GHContent fileContent = ownRepository.getFileContent(filename);
            System.out.println("File is already existing. Overwriting it...");
            fileContent.update(newContentBytes, commitMessage);
            System.out.println("Successful updated the file!");
            System.out.println("------------------------------------");
            return true;
        } catch (IOException e1) {
            try {
                // make a new file
                System.out.println("Creating a new file...");
                ownRepository.createContent(newContentBytes, commitMessage, filename);
                System.out.println("Successful created the file!");
                System.out.println("------------------------------------");

                return true;
            } catch (IOException e2) {
                // failed
                System.out.println("Creating a new file failed too!");
                System.out.println("Debug of overwriting old file:");
                System.out.println(e1.getMessage());
                System.out.println("Filename: " + filename);
                e1.printStackTrace();
                System.out.println("EXCEPTION!");
                System.out.println("Cannot create the file on server. At makeCommit");
                System.out.println("CommitMessage: " + commitMessage);
                System.out.println("Filename: " + remoteFilename.substring(79));
                System.out.println("Lenght of Content: " + newContentBytes.length);
                e2.printStackTrace();
                System.out.println("------------------------------------");
                return false;
            }
        }
    }

    /** Removes a line from the local file with the URLs of the repository file */
    private void removeLine() {

        java.io.File tmp = null;
        try {
            tmp = java.io.File.createTempFile("tmp", "");
        } catch (IOException e1) {
            System.out.println("Cannot create temporal file in removeLine");
            e1.printStackTrace();
        }

        try (BufferedReader br = new BufferedReader(new FileReader(localFileName)); BufferedWriter bw = new BufferedWriter(new FileWriter(tmp));) {

            br.readLine();

            String l;
            while (null != (l = br.readLine())) {
                bw.write(String.format("%s%n", l));
            }
        } catch (Exception e) {
            System.out.println("ERROR: At removeLine.");
            e.printStackTrace();
        }

        java.io.File oldFile = new java.io.File(localFileName);
        if (oldFile.delete())
            tmp.renameTo(oldFile);

    }

    /** Not used call for debugging purposes.
     * 
     * @return the last Commit which was made. Null by error. */
    private String getLastOwnCommit() {
        try {
            GitHub github = GitHub.connectUsingOAuth("fake");
            GHRepository repository = github.getRepository("fake");
            PagedIterable<GHCommit> pagedIterableCommits = repository.listCommits();
            GHCommit lastCommit = pagedIterableCommits.iterator().next();
            lastCommit.getSHA1();
            if (lastCommit.getFiles().isEmpty() || lastCommit.getFiles().size() != 1) {
                System.out.println("The last own commit has no or more than 1 file inside. This shouldn't be the case. Please check for errors!");
                return null;
            } else {
                return lastCommit.getFiles().get(0).getFileName();
            }

        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
            return null;
        }

    }

}

Timer.java

package gitHubAutoCopyCommitter;

import java.util.ArrayList;
import java.util.Collections;

import org.joda.time.DateTimeZone;
import org.joda.time.LocalDate;
import org.joda.time.LocalTime;
import org.joda.time.Seconds;


/**
 * Responsible for all timing relevant calculations.
 * @author Bernd Ernst
 *
 */
public class Timer {

    /** After the posts for this day are finished, wait until the next day and
     * calculate new times to post over the day.
     * 
     * @return Duration until the next posting time generation should be
     *         triggered. */
    public int getSecondsUntilNextPostingTimeGeneration() {

        final int SECONDS_FROM_MIDNIGHT = 60 * 60 * 3; // Should always do at 3am
        int secondsUntilMidnight = Seconds.secondsBetween(getCurrentTime(), new LocalTime(23, 59, 59)).getSeconds();
        return secondsUntilMidnight + SECONDS_FROM_MIDNIGHT; //
    }

    /** Calculates the duration to wait until the next posting time should occur.
     * 
     * @param nextTime
     *            The next point in time in which a post should occur.
     * @return Duration to wait until the next posting time should occur. */
    public int getSecondsUntilNextPost(LocalTime nextTime) {
        System.out.println("nextTime: " + nextTime);
        System.out.println("currentTime: " + getCurrentTime());
        if (nextTime.isBefore(getCurrentTime())) {
            System.out.println("It's already over!");
            return 1;
        } else {
            System.out.println("It is upcoming!");
            return Seconds.secondsBetween(getCurrentTime(), nextTime).getSeconds();
        }
    }

    /** Calculates in a certain time frame. Start time: Between 8 AM and 2 PM.
     * End time: Between start time and up to 6 hours. Latest 10 PM.
     * 
     * @return The sorted filled list with the times in which postings should
     *         happen */
    public ArrayList<LocalTime> getPostingTimes() {
        int postsPerDay = getPostsPerDay();

        ArrayList<LocalTime> collectionOfPostingTimes = new ArrayList<LocalTime>();
        LocalTime randomStartTime = new LocalTime(8, 0, 0).plusSeconds((int) ((60 * 60 * 6) * Math.random()));
        for (int i = 0; i < postsPerDay; i++) {
            collectionOfPostingTimes.add(randomStartTime.plusSeconds((int) ((60 * 60 * 6) * Math.random())));
        }
        Collections.sort(collectionOfPostingTimes);
        return collectionOfPostingTimes;
    }

    /** Calculates the number of posts. It depends on the working day and
     * includes an random amount.
     * 
     * @return the amount of posts which should be achieved per day */
    private int getPostsPerDay() {

        final int MONDAY = 1;
        final int TUESDAY = 5;
        final int FRIDAY = 6;
        final int SUNDAY = 7;
        final int NUMBER_OF_POSTS_PER_DAY = 10;

        LocalDate localdate = new LocalDate();

        int dayOfTheWeek = localdate.getDayOfWeek();

        if (dayOfTheWeek >= MONDAY && dayOfTheWeek <= TUESDAY) {
            return (int) (NUMBER_OF_POSTS_PER_DAY * Math.random() + 1); //+1 for minimum 1 post per day
        } else if (dayOfTheWeek == FRIDAY) {
            return (int) (NUMBER_OF_POSTS_PER_DAY / 2 * Math.random());
        } else if (dayOfTheWeek == SUNDAY) {
            return 20;
            //TODO 0 eintragen beim Sonntag
        } else {
            System.out.println("ERROR: dayOfTheWeek is not between 1-7: " + dayOfTheWeek);
            return -1;
        }

    }

    /** Return the current time in Bangkok timezone
     * 
     * @return The current time in Bangkok timezone */
    private LocalTime getCurrentTime() {

        // +7 is Timezone for Bangkok
        return new LocalTime(DateTimeZone.forOffsetHours(+7));

    }
}
\$\endgroup\$
2
\$\begingroup\$

Regarding your questions:

  • There just is not much OO going on here. Grouping related functions in a class doesn't make it OO. To be fair, I'm not saying it has to be OOP, but generally you'd decompose functionality into much smaller components, making it more testable.
  • The main function is indeed huge. I'd suggest splitting into more granular functionality, e.g. options and argument parsing, authentication, the actual processing, API access, helper functions, ...
  • Sure, storing input in a file is reasonable,
  • and each entry on a new line is again absolutely fine. Just be aware that the way lines are removed after each loop isn't efficient. I'd say that reading the file once, then removing entries on each loop and only writing back once the program is done would be better, unless of course you really want to make sure that "done" lines are removed immediately.
  • The docstrings are not the worst, but also not great. Consider the one for main: The reader doesn't really care about implementation details like this. If there's nothing special about the main method, except that it's obviously called first, don't write much of a text for it. OTOH how the OAuth token is passed should be explained in more detail (or use a library for argument parsing). The one for getFirstLine has the same text twice.
  • Yes, the try/catch blocks aren't great, see below for more, but in general I doubt most of the exception handling is correct. Keep in mind that it might be better to either not catch every exception, but handle them only when it's actually clear what should happen and that just printing the stack trace and returning invalid values is hurting your ability to debug the program (imagine getting a couple of these stack traces while the program happily marches on as the exceptions are immediately eaten and essentially discarded).
  • Not major, but there a few remaining German phrases left; I'd translate them as well.

Random remarks about the code:

  • Since Joda-Time is already a dependency, it would be good to use it too. That is, Timer contains way to many instances of raw numeric calculations of time durations. That is exactly the kind of calculation this library could make look better (and possibly more accurate, if that were a concern). So, instead of .plusSeconds(60 * 60 * 6 * ...), using .plusHours(6 * ...) would be much more readable. In the same way I'd rather lookup the timezone for Bangkok by name (forID), instead of hardcoding it to +7.
  • It's generally better to return the interface for generic collections, not the specific implementation, e.g. List<LocalTime>. That way the implementation can be changed while the method signature stays the same.
  • getPostsPerDay has a number of problems: The constants are redundant (org.joda.time.DateTimeConstants), they are wrong (TUESDAY should be THURSDAY) and after fixing the Thursday bug, the function will never return -1, as the library won't give out an instance where that value is outside of the range. Even if it where, returning -1 while printing to standard output would be wrong - the better option is to raise an exception and deal with it (at least that would currently be fine as the program would just do nothing afterwards). In any case errors should be printed to standard error (stderr), or rather, using a logging library would be good. Also consider using a switch statement here with fall-through for the first four days - that way you save a bit of typing/reading.

That said, if the code is improved a bit, consider posting it here again to get more in-depth feedback - at this point I haven't even started to look into the logic much.

Also consider looking into more libraries, e.g. a task scheduler which could make the main loop redundant, file handling for the input, logging, testing(!), ...

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