Issue 208 project submissions scoring ui by susham · Pull Request #218 · JoyOfCodingPDX/JoyOfCoding

susham · 2017-08-30T16:52:47Z

For retrieving the student Id and Project Name, I think method I have written checks for the string CS410. I think, this should be changed. As the summer term had both CS410 and CS510.

…s-scoring-ui Issue 208 project submissions scoring ui

…s-scoring-ui Get upstream changes from Dave

…s-scoring-ui Get latest changes from DavidWhitlock's issue 208 branch

DavidWhitlock

Thank you for making these changes, @susham. Overall, they are very good. I have some suggestions that I think would improve the code. After we discuss those some more, I'd be happy to merge this code into my repository.

DavidWhitlock · 2017-08-31T12:23:22Z

grader/src/main/java/edu/pdx/cs410J/grader/scoring/ProjectSubmission.java

  private Double score;
  private String studentName;
  private Date submissionTime;
+  private Date gradedTime;


Oh. Good call. Thank you for adding this.

DavidWhitlock · 2017-08-31T12:24:06Z

grader/src/main/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionOutputFileParser.java

-  public ProjectSubmissionOutputFileParser(Reader source) {

+  public ProjectSubmissionOutputFileParser(Reader source) {
+      if(source != null) {


If source is null, it's probably better to throw a NullPointerException.

DavidWhitlock · 2017-08-31T12:25:00Z

grader/src/main/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionOutputFileParser.java

+      return date;
  }
+
+    private BufferedReader bufferedReader=null;


Please declare field at the beginning of the class before the constructor and methods.

Also, since bufferedReader is always initialized in the constructor and is never modified, you can make the field final.

DavidWhitlock · 2017-08-31T12:25:39Z

grader/src/main/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionOutputFileParser.java

  }
+
+    private BufferedReader bufferedReader=null;
+    private ProjectSubmission projectSubmission=null;


You don't need to initial this field to null. That is the default value it will receive.

DavidWhitlock · 2017-08-31T12:27:43Z

grader/src/main/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionOutputFileParser.java

+          }
+
+      } catch (IOException e) {
+          e.printStackTrace();


Please don't "swallow" this exception. Instead, declare that this method throws an IOException and let the calling code deal with it.

DavidWhitlock · 2017-08-31T12:30:00Z

grader/src/main/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionOutputFileParser.java

+     projectSubmission = new ProjectSubmission();
+     String fileContents=null;
+
+      try {


You want to be sure to close the BufferedReader when your done with it. I recommend using the "enhanced try/catch" syntax described here:

https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

DavidWhitlock · 2017-08-31T12:31:19Z

grader/src/main/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionOutputFileParser.java

+      try {
+          while((fileContents= bufferedReader.readLine()) !=null) {
+            fileContents=fileContents.trim();
+          if(fileContents.contains("CS410J")){              // fileContents has the student Id and Project Name.


At the moment, my grading scripts don't differentiate between the undergraduate and graduate sections of the course. So, it is fine to have this hard-coded "magic string" of "CS410J".

DavidWhitlock · 2017-08-31T12:32:46Z

grader/src/main/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionOutputFileParser.java

+            fileContents=fileContents.trim();
+          if(fileContents.contains("CS410J")){              // fileContents has the student Id and Project Name.
+              String[] splitContent=fileContents.split(Pattern.quote("."));
+              projectSubmission.setProjectName(splitContent[splitContent.length -1]);


If the output file being parsed is bogus, we'll end up with ArrayIndexOutOfBoundsExceptions, but I don't know what we could do about it. Maybe an answer will be become apparent later as we build more of the class..

Yes, bogus output file is always going to be an issue. probably I can write a test case to handle the bogus output file.

As of the first iteration, handling this scenario should help to move on. But, we need to find out a better way or use a different type.

DavidWhitlock · 2017-08-31T12:33:40Z

grader/src/main/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionOutputFileParser.java

+
+            }
+
+            if(fileContents.contains("Submitted by")){ //fileContents has StudentName


I recommend that you move "Submitted by" into a constant (a private static final field whose name is ALL_CAPS) so that this "magic" string doesn't appear twice.

Do you think, its better to create final fields whose names are ALL_CAPS, so that the magic string doesn't appear twice?

Yes. That way, if the "magic" string ever changes in the output file, we only need to change one place in the code.

DavidWhitlock · 2017-08-31T12:35:38Z

grader/src/main/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionOutputFileParser.java

+                projectSubmission.setGradedTime(parseGradingTime(splitContent[splitContent.length -1].trim()));
+            }
+              if(fileContents.contains("out of")){ //fileContents has Total Points and Awarded Points
+                  String [] splitContent=fileContents.split("out of");


This works, but you might also want to consider using two "capturing groups" in a regular expression:

https://alvinalexander.com/blog/post/java/how-extract-multiple-groups-patterns-string-regex

Is it possible to create a regexp to get the left and right side of the matched string? Right now, I am able to get both the left and right sub string of the matched string using a separate Regexp

susham · 2017-09-01T16:13:29Z

@DavidWhitlock

I have made changes as per the code review. Do I have to raise another pull request, to reflect my changes?

DavidWhitlock

Thank you for addressing my earlier feedback. I had some questions about the most recent changes, though.

DavidWhitlock · 2017-09-02T21:48:24Z

grader/src/main/java/edu/pdx/cs410J/grader/scoring/InvalidFileContentException.java

+ * Created by sushamkumar on 9/1/17.
+ */
+public class InvalidFileContentException extends Exception {
+    public InvalidFileContentException() { super("Out file is corrupted"); }


Is this constructor ever called? If not, let's delete it.

No, this constructor is never called for now. But, I have kept this so that If there isn't any message to provide then we could use this constructor. I will remove this, so that custom message makes more sense.

DavidWhitlock · 2017-09-02T21:50:15Z

grader/src/main/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionOutputFileParser.java

+
+            if(fileContents.contains("out of")){ //fileContents has Total Points and Awarded Points
+
+                  Pattern rightpattern = Pattern.compile("(?<=out of).*");


I thought there was a way to go this with a single regular expression. But maybe not.

DavidWhitlock · 2017-09-02T21:52:00Z

grader/src/main/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionXmlConverter.java


  private void convertOutFileToXml(File outFile) {
+
+     File xmlFile = null;


Is the only change that you made to this method is to move this variable declaration to the top? If so, I'm not sure if that's necessary. This isn't C where you need to declare all variables at the beginning of a function. In Java, it's preferred to declare variables close to where they are used.

I had to move the variable declaration to the top, because the variable scope wasn't sufficient. That was because of the try, catch block. I think, I have to refactor the code to make the unit test cases pass.

DavidWhitlock · 2017-09-02T21:53:01Z

grader/src/main/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionXmlConverter.java

-      submission = parser.parse();

-    } catch (FileNotFoundException e) {
-      logger.error("Could not file file " + outFile, e);


I liked having this specific error message when the file could not be found.

DavidWhitlock · 2017-09-02T21:53:45Z

grader/src/test/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionOutputFileParserTest.java

    file.line("*****  Test 1: No arguments");

-    ProjectSubmission submission = parse(file);
+    ProjectSubmission submission = null;


Do you think that splitting the declaration and assignment make the code easier to read? I'm not so sure.

DavidWhitlock · 2017-09-02T21:54:42Z

grader/src/test/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionOutputFileParserTest.java

+    projectSubmission= parser.parse();
+
+       parser.parse();
+    } catch (ParseException e) {


You might as well have the parse() method declare that it should throw the ParseException.

DavidWhitlock · 2017-09-02T21:55:38Z

grader/src/test/java/edu/pdx/cs410J/grader/scoring/ProjectSubmissionOutputFileParserTest.java

+       parser.parse();
+    } catch (ParseException e) {
+      e.printStackTrace();
+    } catch (IOException e) {


I'm not sure that this is what you want to do. Yes, the stack trace will be printed, but null will be returned from the method. That will probably cause the test to fail, but perhaps not in the way that anyone would expect.

DavidWhitlock · 2017-09-02T22:20:14Z

Also, @susham, I noticed that the Travis build failed. It looks like there was a problem with the grader application. The Travis output wasn't very helpful, but when I made a local checkout of your branch, I saw these unit tests fail:

Tests run: 11, Failures: 0, Errors: 2, Skipped: 3, Time elapsed: 0.023 sec <<< FAILURE! - in edu.pdx.cs410J.grader.scoring.ProjectSubmissionOutputFileParserTest
parseEmptyGrade(edu.pdx.cs410J.grader.scoring.ProjectSubmissionOutputFileParserTest)  Time elapsed: 0.016 sec  <<< ERROR!
java.lang.NumberFormatException: empty String
	at edu.pdx.cs410J.grader.scoring.ProjectSubmissionOutputFileParserTest.parse(ProjectSubmissionOutputFileParserTest.java:242)
	at edu.pdx.cs410J.grader.scoring.ProjectSubmissionOutputFileParserTest.parseEmptyGrade(ProjectSubmissionOutputFileParserTest.java:116)

parseTotalPoints(edu.pdx.cs410J.grader.scoring.ProjectSubmissionOutputFileParserTest)  Time elapsed: 0.001 sec  <<< ERROR!
java.lang.NumberFormatException: empty String
	at edu.pdx.cs410J.grader.scoring.ProjectSubmissionOutputFileParserTest.parse(ProjectSubmissionOutputFileParserTest.java:242)
	at edu.pdx.cs410J.grader.scoring.ProjectSubmissionOutputFileParserTest.parseTotalPoints(ProjectSubmissionOutputFileParserTest.java:100)

susham added 4 commits July 29, 2017 09:59

Merge pull request #1 from DavidWhitlock/issue-208-project-submission…

4e5ec24

…s-scoring-ui Issue 208 project submissions scoring ui

Merge pull request #2 from DavidWhitlock/issue-208-project-submission…

323505e

…s-scoring-ui Get upstream changes from Dave

Implementation of ProjectSubmissionOutputFileParser

76407f0

Merge pull request #3 from DavidWhitlock/issue-208-project-submission…

fffa086

…s-scoring-ui Get latest changes from DavidWhitlock's issue 208 branch

DavidWhitlock requested changes Aug 31, 2017

View reviewed changes

Made few changes as per the code review

407599c

DavidWhitlock mentioned this pull request Sep 2, 2017

Issue 208 project submissions scoring ui #217

Closed

DavidWhitlock reviewed Sep 2, 2017

View reviewed changes


		}

		if(fileContents.contains("Submitted by")){ //fileContents has StudentName


		if(fileContents.contains("out of")){ //fileContents has Total Points and Awarded Points

		Pattern rightpattern = Pattern.compile("(?<=out of).*");


		private void convertOutFileToXml(File outFile) {

		File xmlFile = null;

Conversation

susham commented Aug 30, 2017

Uh oh!

DavidWhitlock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

susham commented Sep 1, 2017

Uh oh!

DavidWhitlock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidWhitlock commented Sep 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants