Skip to content

Instantly share code, notes, and snippets.

@Michael0x2a

Michael0x2a/Dog-feedback.html

Created September 13, 2015 15:48
Show Gist options
  • Save Michael0x2a/55ca0a682cf75067d62b to your computer and use it in GitHub Desktop.
Save Michael0x2a/55ca0a682cf75067d62b to your computer and use it in GitHub Desktop.
Download ZIP
Dog.java Feedback
Raw
Dog-feedback.html
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Feedback: Dog.java</title>
<meta name="viewport" content="width=device-width, initial-scale=1" />
<link href="https://cdnjs.cloudflare.com/ajax/libs/prism/0.0.1/prism.min.css" rel="stylesheet" />
<style type="text/css">
* {
box-sizing: border-box;
}
body {
padding:1em;
font-family: Arial, sans-serif;
font-size: 16px;
}
.wrapper {
display: block;
min-width: 40em;
max-width: 60em;
margin: 0 auto;
}
h1 {
margin-bottom: 0.25em;
}
h1 + p {
margin-bottom: 2em;
margin-left: 0;
color: #888;
}
.line {
display: block;
width: 100%;
overflow: auto;
}
.codeline:hover .lineno {
color: #111;
}
.codeline {
overflow: hidden;
}
.lineno {
font-family: Consolas,Monaco,"Andale Mono",monospace;
padding: 0.25em; 0.5em;
width: 2.7em;
float: left;
color: #888;
background-color: #f5f2f0;
border-left: 4px solid #111;
border-right: 1px solid #e0e0e0;
text-align: right;
transition: color 0.25s ease;
-webkit-touch-callout: none;
-webkit-user-select: none;
-khtml-user-select: none;
-moz-user-select: none;
-ms-user-select: none;
user-select: none;
}
.lineno > span {
font-size: 0.8em;
}
.line:hover .lineno > span {
font-weight: bold;
}
pre {
position: relative;
font-family: Consolas,Monaco,"Andale Mono",monospace;
background-color: #f5f2f0;
color: #000;
text-shadow: 0px 1px #fff;
padding: 0.25em 1em;
direction: ltr;
white-space: pre;
word-spacing: normal;
word-break: normal;
-moz-tab-size: 4;
-moz-hyphens: none;
}
.codeline > pre {
margin-left: 2.7em;
margin-top: 0;
margin-bottom: 0;
}
pre[class*="language-"] {
margin: 0;
padding: 0.25em 1em;
}
pre[class*="language-"] > code[data-language] {
overflow: hidden;
}
.button {
position: relative;
width: 2.7em;
padding: 0.2em 0.2em;
padding-left: 0.4em;
cursor: pointer;
z-index: 5;
margin-top: 0.1em;
font-weight: 700;
color: #888;
transition: color 0.25s ease;
}
.button:hover {
color: #111;
}
.button span {
font-size: 0.7em;
}
.commentary {
position: relative;
padding-left: 3.2em;
overflow: hidden;
}
.hidden {
}
.annotated .commentary {
padding-bottom: 1em;
margin-top: -1.5em;
}
.commentary pre {
display: block;
}
.commentary pre:last-child {
margin-bottom: 1em;
}
code:before {
display: none !important;
}
p {
padding: 0em;
margin: 0.5em;
}
</style>
</head>
<body>
<div class="wrapper">
<h1>Feedback: Dog.java</h1>
<p>(Generated on Sunday, September 2015 at 8:45:38 AM, Pacific Daylight Time)</p>
<div class="feedback"> <div class="line annotated">
<div class="codeline">
<div class="lineno"><span>1</span></div>
<pre><code class="language-java">public class Dog{</code></pre>
</div>
<div class="commentary"><p>Always include one space before each curly bracket. So, do:</p>
<pre><code class="language-java">public class Dog {
public void myMethod {
if (foo == bar) {</code></pre>
<p>...instead of doing:</p>
<pre><code class="language-java">public class Dog{
public void myMethod{
if (foo == bar){</code></pre></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>2</span></div>
<pre><code class="language-java"> String dogName;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>3</span></div>
<pre><code class="language-java"> int dogAge;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line annotated">
<div class="codeline">
<div class="lineno"><span>4</span></div>
<pre><code class="language-java"> String gender;</code></pre>
</div>
<div class="commentary"><p>You should probaby use an enum or a class to represent gender.</p>
<p>If you want to keep things simple, use an enum. If you want to be <a href="http://www.thedailybeast.com/articles/2014/02/15/the-complete-glossary-of-facebook-s-51-gender-options.html">fully inclusive</a> and allow for the user to pick any arbitrary gender, then perhaps use a class.</p></div>
</div>
<div class="line annotated">
<div class="codeline">
<div class="lineno"><span>5</span></div>
<pre><code class="language-java"> DogActions actions;</code></pre>
</div>
<div class="commentary"><p>These fields should be set to private. Currently, they're considered &quot;package-protected&quot;, which means that any other class within this current package (ie is inside your project) can access these fields directly.</p>
<p>This is probably not what you want.</p></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>6</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>7</span></div>
<pre><code class="language-java"> String[] maleThoughts = {"games", "chasing cats", "blue"};</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>8</span></div>
<pre><code class="language-java"> String[] femaleThoughts = {"games", "taking a bath", "pink"};</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>9</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>10</span></div>
<pre><code class="language-java"> String determineGender1; //He or She. For using proper words in sentences that involve gender.</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>11</span></div>
<pre><code class="language-java"> String determineGender2; //His or Her</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line annotated">
<div class="codeline">
<div class="lineno"><span>12</span></div>
<pre><code class="language-java"> String determineGender3; //him or her</code></pre>
</div>
<div class="commentary"><p>Rather then hard-coding this information into your <code>Dog</code> class, place it inside the <code>Gender</code> enum or class. (Enums can have fields and methods too, just like classes).</p>
<p>Also, come up with more descriptive names -- here's <a href="http://www.grammar-monster.com/lessons/pronouns_different_types.htm">a refresher on pronoun types</a> if you don't remember what each kind of pronoun is called.</p></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>13</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>14</span></div>
<pre><code class="language-java"> public Dog(){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>15</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line annotated">
<div class="codeline">
<div class="lineno"><span>16</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"><p>This constructor is completely useless, breaks your code, and should be removed. When you call this constructor, you'll end up leaving your object in an undetermined state. That is, if somebody calls this constructor, your fields will be null, potentially breaking the rest of the code.</p>
<p>You should either delegate to your 2-arg constructor (ie do <code>this(..., ...)</code>) or remove this entirely.</p></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>17</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>18</span></div>
<pre><code class="language-java"> public Dog(String name){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>19</span></div>
<pre><code class="language-java"> this();</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>20</span></div>
<pre><code class="language-java"> dogName = name;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line annotated">
<div class="codeline">
<div class="lineno"><span>21</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"><p>Same thing: you don't set <code>action</code> here.</p></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>22</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>23</span></div>
<pre><code class="language-java"> public Dog(String name, DogActions actions){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line annotated">
<div class="codeline">
<div class="lineno"><span>24</span></div>
<pre><code class="language-java"> this(name);</code></pre>
</div>
<div class="commentary"><p>Rather then delegating a portion of the task to your 1-arg constructor, you should instead have this constructor do 100% of the work, and make the <em>other</em> constructors delegate to this one.</p>
<p>That way, you can keep all of the logic in exactly one place + it becomes easier for the simpler constructors to specify sensible defaults.</p>
<p>In addition, you should take care of setting all of the other fields (gender, age, etc) in your constructor. Don't leave that for later: initialize everything now.</p>
<p>If you start getting the feeling that your constructor accepts too many parameters, then here's a good <a href="http://www.javaworld.com/article/2074932/core-java/too-many-parameters-in-java-methods-part-1-custom-types.html">seven-part article series</a> on reducing the number of parameters in Java.</p>
<p>So, keeping in mind all of my previous feedback, I would restructure your constructors to look like the following:</p>
<pre><code class="language-java">public Dog() {
this(&quot;Default name&quot;, 1, DogActions.SITTING, Gender.UNKNOWN);
}
public Dog(String name) {
this(name, 1, DogActions.SITTING, GENDER.UNKNOWN);
}
public Dog(String name, int age, DogActions actions, Gender gender) {
this.name = name;
this.age = age;
this.actions = actions;
this.gender = gender;
}</code></pre>
<p>If the defaults chosen in the first two constructors aren't good defaults, and if you're finding it difficult to think of what might be good defaults, then that's a sign that those two constructors might need to be removed altogether.</p></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>25</span></div>
<pre><code class="language-java"> this.actions = actions;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>26</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>27</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>28</span></div>
<pre><code class="language-java"> public enum DogActions{</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>29</span></div>
<pre><code class="language-java"> SITTING, WALKING, RUNNING, SLEEPING, PLAYING</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line annotated">
<div class="codeline">
<div class="lineno"><span>30</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"><p>I would add <code>NOTHING</code> to this enum. I see that in some cases, you interpret cases where <code>action</code> is null to mean that the dog is doing nothing.</p>
<p>You should almost always avoid using null when you can + be very careful when you do use it. The problem is that it's very easy in Java to set things to <code>null</code> by mistake. For example, if you try looking up a nonexistent key in a Map, you'll get back <code>null</code> instead of getting an exception.</p>
<p>This means that it can get very tricky to distinguish between &quot;I meant to put null there&quot; vs &quot;somebody accidentally put 'null' there by mistake&quot;.</p>
<p>(For more discussion on this topic, try googling &quot;null, a billion dollar mistake&quot;.)</p>
<p>By adding <code>NOTHING</code> to your enum, you can represent every single possible action and simplify your logic.</p></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>31</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>32</span></div>
<pre><code class="language-java"> public void seeWhatDogIsDoing(){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>33</span></div>
<pre><code class="language-java"> if(actions != null){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>34</span></div>
<pre><code class="language-java"> switch (actions){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>35</span></div>
<pre><code class="language-java"> case SITTING:</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>36</span></div>
<pre><code class="language-java"> System.out.printf("%s is now sitting.\n", determineGender1);</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>37</span></div>
<pre><code class="language-java"> break;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>38</span></div>
<pre><code class="language-java"> case WALKING:</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>39</span></div>
<pre><code class="language-java"> System.out.printf("%s is now walking. Watch %s.\n", determineGender1, determineGender3);</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>40</span></div>
<pre><code class="language-java"> break;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>41</span></div>
<pre><code class="language-java"> case RUNNING:</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>42</span></div>
<pre><code class="language-java"> System.out.printf("%s is now running. Catch %s.\n", determineGender1, determineGender3);</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>43</span></div>
<pre><code class="language-java"> break;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>44</span></div>
<pre><code class="language-java"> case SLEEPING:</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>45</span></div>
<pre><code class="language-java"> System.out.printf("%s is now sleeping. Shhh..\n", determineGender1);</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>46</span></div>
<pre><code class="language-java"> break;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>47</span></div>
<pre><code class="language-java"> case PLAYING:</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>48</span></div>
<pre><code class="language-java"> System.out.printf("%s is now playing. Play with %s.\n", determineGender1, determineGender3);</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>49</span></div>
<pre><code class="language-java"> break;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>50</span></div>
<pre><code class="language-java"> default:</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>51</span></div>
<pre><code class="language-java"> System.out.printf("%s is doing nothing.\n", determineGender1);</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>52</span></div>
<pre><code class="language-java"> break;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line annotated">
<div class="codeline">
<div class="lineno"><span>53</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"><p>Consider returning the string, instead of printing it. That way, the user of your class gets to decide what to do with this information.</p>
<p>More broadly speaking, it might be a good idea to move all of this logic into a different class entirely, and just stick with returning <code>action</code>. That way, you can keep your logic a little cleaner: the <code>Dog</code> class just contains pure information, and the caller is responsible for doing something with that information.</p>
<p>That said, if your goal was to just practice using objects, then keeping this method inside <code>Dog</code> is reasonable.</p></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>54</span></div>
<pre><code class="language-java"> }else{</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>55</span></div>
<pre><code class="language-java"> System.out.printf("%s is doing nothing.\n", determineGender1);</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>56</span></div>
<pre><code class="language-java"> } </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>57</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>58</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>59</span></div>
<pre><code class="language-java"> public void seeWhatDogIsThinking(){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>60</span></div>
<pre><code class="language-java"> String thought = thoughtDetermination(genderDetermination());</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>61</span></div>
<pre><code class="language-java"> System.out.printf("%s is thinking of %s.\n", determineGender1, thought);</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line annotated">
<div class="codeline">
<div class="lineno"><span>62</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"><p>Same thing: consider returning the string, not printing it.</p>
<p>The gender should be determined within the constructor. You should also be comparing that enum or class directly, instead of using a number.</p></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>63</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>64</span></div>
<pre><code class="language-java"> //This method is written just so I don't need to have long lines everytime in other methods to determine gender </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>65</span></div>
<pre><code class="language-java"> int genderDetermination(){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>66</span></div>
<pre><code class="language-java"> if(gender.toLowerCase().equals("male")){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>67</span></div>
<pre><code class="language-java"> return 1;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>68</span></div>
<pre><code class="language-java"> }else if(gender.toLowerCase().equals("female")){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>69</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>70</span></div>
<pre><code class="language-java"> return 2;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>71</span></div>
<pre><code class="language-java"> }else{</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>72</span></div>
<pre><code class="language-java"> System.out.println("Error: Gender assignment");</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>73</span></div>
<pre><code class="language-java"> return 0;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>74</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>75</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>76</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>77</span></div>
<pre><code class="language-java"> void genderDeterminator(int num){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>78</span></div>
<pre><code class="language-java"> if(num == 1){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>79</span></div>
<pre><code class="language-java"> determineGender1 = "He";</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>80</span></div>
<pre><code class="language-java"> determineGender2 = "His";</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>81</span></div>
<pre><code class="language-java"> determineGender3 = "him";</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>82</span></div>
<pre><code class="language-java"> }else if(num == 2){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>83</span></div>
<pre><code class="language-java"> determineGender1 = "She";</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>84</span></div>
<pre><code class="language-java"> determineGender2 = "Her";</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>85</span></div>
<pre><code class="language-java"> determineGender3 = "her";</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>86</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>87</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>88</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>89</span></div>
<pre><code class="language-java"> String thoughtDetermination(int num){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>90</span></div>
<pre><code class="language-java"> int randNum;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>91</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>92</span></div>
<pre><code class="language-java"> if(num == 1){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line annotated">
<div class="codeline">
<div class="lineno"><span>93</span></div>
<pre><code class="language-java"> randNum = (int)(Math.random() * maleThoughts.length);</code></pre>
</div>
<div class="commentary"><p>Refactor the logic to get a random number into a helper method.</p></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>94</span></div>
<pre><code class="language-java"> return maleThoughts[randNum];</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>95</span></div>
<pre><code class="language-java"> }else if(num == 2){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>96</span></div>
<pre><code class="language-java"> randNum = (int)(Math.random() * femaleThoughts.length);</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>97</span></div>
<pre><code class="language-java"> return femaleThoughts[randNum];</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>98</span></div>
<pre><code class="language-java"> }else{</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>99</span></div>
<pre><code class="language-java"> return "Error: Either you haven't assigned a gender or you used other wording to assign gender.";</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>100</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>101</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>102</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>103</span></div>
<pre><code class="language-java"> public void setName(String name){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>104</span></div>
<pre><code class="language-java"> dogName = name;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>105</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>106</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>107</span></div>
<pre><code class="language-java"> public String getName(){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>108</span></div>
<pre><code class="language-java"> return dogName;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>109</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>110</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>111</span></div>
<pre><code class="language-java"> public void setAge(int age){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>112</span></div>
<pre><code class="language-java"> dogAge = age;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>113</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>114</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>115</span></div>
<pre><code class="language-java"> public int getAge(){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>116</span></div>
<pre><code class="language-java"> return dogAge;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>117</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>118</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>119</span></div>
<pre><code class="language-java"> public void setGender(String gender){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>120</span></div>
<pre><code class="language-java"> this.gender = gender;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>121</span></div>
<pre><code class="language-java"> genderDeterminator(genderDetermination());</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>122</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>123</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>124</span></div>
<pre><code class="language-java"> public String getGender(){</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>125</span></div>
<pre><code class="language-java"> return gender;</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line annotated">
<div class="codeline">
<div class="lineno"><span>126</span></div>
<pre><code class="language-java"> }</code></pre>
</div>
<div class="commentary"><p>Strongly consider adding validation to your setters. For example, in your <code>age</code> setter, throw an exception if the input age is negative.</p>
<p>In the <code>name</code> and <code>gender</code> setters, throw an exception if the input is null, or if the user provides an invalid gender.</p>
<p>Basically, you want to structure your class so that your fields (your internal state) always makes sense, and aggresively perform checks on the &quot;boundary&quot;: the places where the user provides input. Fail as soon as possible.</p>
<p>This also makes it much easier for the user of your class to debug. It's much easier to identify what the problem is if their call to <code>setGender</code> is throwing an <code>IllegalArgumentException</code> vs if they call some completely unrelated method, which fails in mysterious ways.</p>
<p>Also, you're missing getters and setters for <code>action</code> -- not sure if that's deliberate or not.</p></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>127</span></div>
<pre><code class="language-java">}</code></pre>
</div>
<div class="commentary"></div>
</div>
<div class="line not-annotated">
<div class="codeline">
<div class="lineno"><span>128</span></div>
<pre><code class="language-java"> </code></pre>
</div>
<div class="commentary"></div>
</div></div>
</div>
<script src="https://cdnjs.cloudflare.com/ajax/libs/velocity/1.2.2/velocity.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/prism/0.0.1/prism.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/2.1.4/jquery.min.js"></script>
<script>
(function() {
function createButtonElement() {
var div = document.createElement('div');
var span = document.createElement('span');
span.textContent = 'Hide';
div.appendChild(span);
div.setAttribute('class', 'button');
return div;
}
function createButton(element) {
var button = createButtonElement();
button.addEventListener('click', function() {
$(element).toggle(250);
if (button.textContent == "Hide") {
button.innerHTML = "<span>Show</span>";
//element.classList.add("hidden");
} else {
button.innerHTML = "<span>Hide</span>";
//element.classList.remove("hidden");
}
});
return button;
}
function insertToggle(element) {
element.parentNode.insertBefore(createButton(element), element);
}
var annotations = document.querySelectorAll(".annotated .commentary");
for (var i = 0; i < annotations.length; i++) {
insertToggle(annotations[i]);
}
})();
</script>
</body>
</html>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment