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

[AY1920S1-CS2113-T14-3] WordUp #96

Open
wants to merge 526 commits into
base: master
Choose a base branch
from

Conversation

tessa-z
Copy link

@tessa-z tessa-z commented Oct 18, 2019

Copy link

@leowweixiang leowweixiang left a comment

Choose a reason for hiding this comment

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

General:
In terms of features and code I think y'all are on a good track :) What's lacking are unit tests (important!!) and header comments on public methods.

* String represents the closest time that user search
* for a specific word
*/
private String closetSearch;

Choose a reason for hiding this comment

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

closestSearch you mean?
To clarify, closest time -> this is a string containing a time format?

}

/**
* Looks up for meaning of a specific word

Choose a reason for hiding this comment

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

Good to add here that "if word is not in Wordbank, call OxfordCall to find the word"

import storage.Storage;
import ui.Ui;

/**
Copy link

@leowweixiang leowweixiang Oct 20, 2019

Choose a reason for hiding this comment

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

Add such headers for other command subclasses. Also, if the execute() is nontrivial (long code), do add a header explain what it does and how

src/main/java/command/ExitCommand.java Outdated Show resolved Hide resolved
return "https://od-api.oxforddictionaries.com/api/v2/entries/" + language + "/" + word_id + "?" + "fields=" + fields + "&strictMatch=" + strictMatch;
}

public static String doInBackground(String word) throws NoWordFoundException {

Choose a reason for hiding this comment

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

Add header comment for these public methods

throw new WrongDeleteFormatException();
}
String[] wordAndTags = taskInfo[1].split("t/");
//delete word

Choose a reason for hiding this comment

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

Good, short comment that lets others zoom into specific code

if (taskInfo.length == 1) {
throw new WrongAddFormatException();
}
String[] wordDetail = taskInfo[1].split("w/");

Choose a reason for hiding this comment

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

Again consider putting your "w/" etc as private static final attribs

src/main/java/parser/Parser.java Outdated Show resolved Hide resolved
}
}

public Stack<Word> loadHistoryFromFile() {

Choose a reason for hiding this comment

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

Header comment. Make explicit the difference between loadHistory and loadWordBank (Stack structure, why)
Also, the code is very similar. Can both be done in one method, or are there times you have to load each separately?

}
return wordBank;
} catch (IOException e) {
e.printStackTrace();
Copy link

@leowweixiang leowweixiang Oct 20, 2019

Choose a reason for hiding this comment

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

You may want to feedback something more useful (to the user)

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

Successfully merging this pull request may close these issues.

7 participants