Code review suggestions #1
Conversation
|
|
||
|
|
||
|
|
||
| public static final List<String> ALL_STUDY_GUIDES = Arrays.asList( |
There was a problem hiding this comment.
Two things here:
- A
Listwould be the preferred approach here. Although aSetdoes work, the data lends itself closer to aList, partly because you are actually listing it out here, and theListtype will keep the order when you process the results, unlike theSet. - As of Java 8 you can create an inline
Listby using theArrays.asList()method, which will take a variable length of parameters, and return them all in aListobject.
| @@ -1,232 +0,0 @@ | |||
| package com.base64.d; | |||
There was a problem hiding this comment.
Other than the name change, this file was changed in the following ways:
- Remove all commented out code that is not needed.
- Remove blank lines when there are more than one consecutive blank lines.
- Cleanup
importstatements, by removing unused imports.
|
|
||
| public class WriteToFile { | ||
|
|
||
| String date = new Date().toString(); |
There was a problem hiding this comment.
By defining the object here, you are specifying an instance variable. This will be set for each instance of WriteToFile. You typically want to push variables into the smallest scope possible. The scope going from "class" to "instance" to "method". So I made this a method level variable by placing it inside the method. This keeps the variables as close to the place that they are used.
| // Find all links within a Study Guide | ||
| try { | ||
| // | ||
| // Connect to the study guide url |
There was a problem hiding this comment.
It's good to have code comments, especially any area where the functionality is not obvious. The double forward slashes for these comments appear to be created with the "comment out code" shortcut in IntelliJ, which starts the slashes at the first character of the line. This is more useful when commenting out a block of code (to really help draw attention to the full block that is commented out, since all of the characters are on the left side of the screen, which is usually blank). What is preferred for code comments is to have the slashes justified with the rest of the code. You can see below how the two forward slashes have been moved to be more inline with the code.
| Document wraperDocument = Jsoup.connect(studyGuideLink).get(); | ||
|
|
||
| // grab the iframe | ||
| Element iframe = doc3.select("iframe").first(); |
There was a problem hiding this comment.
A lot of the variables have more descriptive names, which is helpful. I've renamed doc3 and link2 to be more descriptive of what they will actually hold. Good naming is a best practice in general, and is really helpful when others read your code.
| String jsScripts = scriptTag.toString(); | ||
|
|
||
| // match any string of characters over 100 in length. | ||
| String patternString = "(\\w{100,}.+)"; |
There was a problem hiding this comment.
This regex definitely works, and although it is unlikely to have more than 100 characters together in this class, it would be considered brittle, because some data could be included that would cause problems.
I've changed it to match on the JavaScript variable name, and then reference the double quotes that will be around this object. This would be considered more robust because it is much less likely to fail if the data changes (it's unlikely that a second window.courseData object will be created in this class).
There was a problem hiding this comment.
I couldn't get either version of that regex to work, so have left the 100 character one.
| } | ||
|
|
||
|
|
||
| // Test all links in a Study Guide |
There was a problem hiding this comment.
There are two big blocks of code in this method. And at first I was confused what they were doing, so I just added a comment before the second block here, and also the block above. This is the type of thing that could be extracted into separate methods, to improve readability. But this would also require restructuring the code, because it currently is gathering all of the links in a study guide (first block), and then processing all links for the study guide (second block). It's possible that restructuring might be to check each link after it is retrieved (and just keep track of links that have already been checked, to prevent checking duplicates).
| .statusCode(); | ||
| // Although the `linkContents` is never utilized, it is a | ||
| // required call. This is because Jsoup expects that `get()` | ||
| // is called before the `response()` object is available. |
There was a problem hiding this comment.
Because I've been cleaning deleting all unused variables, I added a comment here to provide an explanation for why this one exists. Since I initially deleted this variable and then found that response() is never populated unless you first call get(). Which I then remembered we talked about over IM a while back 🤦 :haha:
No description provided.