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 #9

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

Review Task 2 #9

wants to merge 1 commit into from

Conversation

fracz
Copy link
Owner

@fracz fracz commented Mar 17, 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

Choose a reason for hiding this comment

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

Abstract class is not abstract

}

public void markConversation(Line line) {
// Nothing to do, it's perfect.
Copy link

Choose a reason for hiding this comment

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

Empty method

}

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

Choose a reason for hiding this comment

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

Unnscessary, just repeats constructor

/**
* Creates source file based on a file reference.
*/
public static SourceFile createFromFile(File sourceFile) throws IOException {
Copy link

Choose a reason for hiding this comment

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

createSourceFileFromFile or copySourceFile

Comment on lines +26 to +33
public void setFile(File file) {
checkValidType(AbstractComment.Type.VOICE);
this.file = file;
}

public File getFile() {
checkValidType(AbstractComment.Type.VOICE);
return file;

Choose a reason for hiding this comment

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

checkValiType() powinna byc w innym miejscu, setter i getter powinny tylko ustawiac i pobierac wartosc zmiennej


import android.content.Context;
import android.graphics.Color;
import android.view.View;

Choose a reason for hiding this comment

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

przerwa pomidzy importami

Comment on lines +26 to +30
if (selectedLine != null) {
selectedLine.setBackgroundColor(Color.TRANSPARENT);
}
selectedLine = (Line) view;
selectedLine.setBackgroundColor(SELECTED_LINE_COLOR);

Choose a reason for hiding this comment

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

można by wydzielić do innej metody

Comment on lines +54 to +56
for (int i = 0; i < digest.length; i++) {
sb.append(Integer.toString((digest[i] & 0xff) + 0x100, 16).substring(1));
}

Choose a reason for hiding this comment

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

rowniez mozna wydzielic do prywatnej metody

Comment on lines +66 to +70
while (tokenizer.hasMoreTokens()) {
Line line = new Line(context, this, lines.size() + 1, tokenizer.nextToken());
line.setOnClickListener(lineHighlighter);
lines.add(line);
}

Choose a reason for hiding this comment

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

również wydzielić do osobnej metody

Comment on lines +83 to +87
if (ApplicationSettings.highlightSources()) {
return SYNTAX_HIGHLIGHTER.highlight(code, language);
} else {
return code;
}

Choose a reason for hiding this comment

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

zamienić na operator trinarny ? :

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.

3 participants