Skip to content

Mocking lab#14

Open
kaeaton wants to merge 20 commits into
MillsMCS:mocking-basefrom
kaeaton:mockingLab
Open

Mocking lab#14
kaeaton wants to merge 20 commits into
MillsMCS:mocking-basefrom
kaeaton:mockingLab

Conversation

@kaeaton
Copy link
Copy Markdown

@kaeaton kaeaton commented Dec 21, 2020

No description provided.

@ellenspertus ellenspertus changed the base branch from complete to mocking-base December 21, 2020 18:37
Copy link
Copy Markdown
Contributor

@ellenspertus ellenspertus left a comment

Choose a reason for hiding this comment

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

Good job! There are just minor items to fix.

Comment thread src/main/java/edu/mills/cs180a/wordui/model/SampleData.java
* Downloads the Wordnik Word of the Day and adds it to a list.
*
* @param the list to add the word to
* @return the list the word was added to
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function is void, so don't use the @return tag.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed.

}

/**
* Populates the passed list, with sample words and Wordnik's Word of the Day.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Excellent wording! Just drop the comma.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed.

/**
* Populates the passed list, with sample words and Wordnik's Word of the Day.
*
* @param the list of word records to be populated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since the type is shown in the signature, I would just say "the list to be populated" or "the list".

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed.

System.err.println("Unable to get API key.");
WordApi wordApi = client.buildClient(WordApi.class);
addWordOfTheDay(backingList, wordsApi, wordApi);
} catch (Exception ex) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't like such a wide catch. Only catch what might be thrown, and display an appropriate error message.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed

return Map.of("text", definition);
}

// make var args for any number of definitions instead of array
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Either do this or remove the comment.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Comment removed.

return fs;
}

// add test for Yucca
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Comment removed.

Comment thread src/test/java/edu/mills/cs180a/wordui/model/SampleDataTest.java
Copy link
Copy Markdown
Contributor

@ellenspertus ellenspertus left a comment

Choose a reason for hiding this comment

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

You accidentally checked in your javadoc with the last commit. Do you know how to get rid of it and keep it from being re-added?

Comment thread src/test/java/edu/mills/cs180a/wordui/model/SampleDataTest.java
WordApi wordApi = client.buildClient(WordApi.class);
addWordOfTheDay(backingList, wordsApi, wordApi);
} catch (IOException ex) {
System.out.println("Probably couldn't access the API client: " + ex);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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