Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review Task 2 #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Review Task 2 #2

wants to merge 1 commit into from

Conversation

fracz
Copy link

@fracz fracz commented Mar 24, 2020

Find as many defects and problems as possible!

You can skip the Line.java as you have already saw it in the Task 1.

import java.text.SimpleDateFormat;
import java.util.Date;

public class AbstractComment {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Myląca nazwa, może sugerować że klasa jest abstrakcyjna

public Type getType() {
return type;
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Po co zwracać niesformatowaną datę w getterze

protected String getAuthor() {
return author;
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enum zadeklarowany na drugim końcu pliku, kiedy Type wykorzystywane jest na początku

@@ -0,0 +1,4 @@
package pl.fracz.mcr.comment;

public class CommentNotAddedException extends Exception {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

???????? Pusta klasa dziedzicząca po exception

public class Comment extends AbstractComment {
private String text;

private File file;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nazwa powinna mówić po co jest określona zmienna, file niewiele mówi ale mogłoby być wykorzystywane np w iteracji kiedy wynika z kontekstu

public static SourceFile createFromString(String sourceCode, String language) {
return new SourceFile(sourceCode, language);
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bezużyteczny komentarz

*/
public static SourceFile createFromFile(File sourceFile) throws IOException {
String sourceCode = FileUtils.read(sourceFile);
return createFromString(sourceCode, FileUtils.getExtension(sourceFile.getName()));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Godline

}

public void addTextComment(String comment) throws CommentNotAddedException {
if (selectedLine == null) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Niepotrzebny null


// public void addVideoComment(File videoFile) throws CommentNotAddedException {
// }

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Niejednolite nazewnictwo, parametr bool

return code;
}
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Można prościej

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants