Add files via upload #2

GabijaBern · 2018-05-21T09:16:10Z

Hello, I have done the tasks, I know that there are minor changes that could be done in order to improve the solutions. Currently I am preparing for my exams, so if I have time until the deadline, I will edit my solutions. I am not catching exceptions, should we do that? Tell me if I am submitting the tasks wrong as well, I am more used to GitLab and shell :)

flerdacodeu · 2018-05-25T06:47:16Z

GabijaBern/assignment1/Anagrams.java

+			Map<Character, Integer> firstMap = createCharMap(firstWord);
+			Map<Character, Integer> secondMap = createCharMap(secondWord);
+
+			if(firstMap.equals(secondMap))


Style: if you have an if-statement that returns true and false, you probably can get rid of the if-statement and just return the condition.

flerdacodeu · 2018-05-25T06:48:44Z

GabijaBern/assignment1/Anagrams.java

+	public static boolean stringSensitive(String firstWord, String secondWord)
+	{
+		//we could first check if those words are the same length
+		if(firstWord.length()==secondWord.length()) 


Style: if you check a pre-condition or a simple case, it is often easier to return early rather than nest your code deeper.

In this case:

if (firstWord.length() != secondWord.length()) { // Anagrams must have the same number of characters. return false; } Map<Character, Integer> firstMap = createCharMap(firstWord); Map<Character, Integer> secondMap = createCharMap(secondWord); ...

Avoiding deep indentation (by this means or by creating a helper function, for example) helps make the code more linear and easier to read.

flerdacodeu · 2018-05-25T06:49:48Z

GabijaBern/assignment1/Anagrams.java

+	}
+
+	//-------------------second part of the question-----------------------
+	public static boolean stringInensitive(String firstWord, String secondWord)


I think you mean stringInsensitive().

flerdacodeu · 2018-05-25T06:50:29Z

GabijaBern/assignment1/Anagrams.java

+public class Anagrams {
+
+
+	//-----------------first part of the question------------------------------


Instead of comments referring to the assignment, as a future software engineer, think about people using your code. In this case, a javadoc explaining what the function does would be helpful.

/** * Determines if two words are anagrams of each other. * * <p>This function is case sensitive, i.e., words that differ in capitalisation (e.g., 'Race' and 'Care') * will not be considered anagrams. * * @param firstWord the first word to be considered * @param secondWord the second word to be considered * @returns true if the words are anagrams, false otherwise. */

Yes, I will try to do the documentation for the second assignment, I know how to do it, but just wrote it more for "myself". So good point, I will try to improve. :)

Optional but it would be great if you could apply this suggestion to this assignment as well :)
The point of code reviews is to improve the code before it is merged into the main repository.
On a typical academic assignment, what matter is what you submit; on a engineering task, you want to iterate until it is as good as possible (within reason) before what is merged will stay around (other people will use it and modify it).

flerdacodeu · 2018-05-25T08:48:11Z

GabijaBern/assignment1/SingleLinkList.java

+class Node<Type>
+{
+	public Node<Type> next;
+	private Type value;


This could be final, since you do not actually modify it after initialization.

flerdacodeu · 2018-05-25T08:52:25Z

GabijaBern/assignment1/SingleLinkList.java

+}
+
+//class for Node objects
+class Node<Type>


Style: It is common convention to use a single capital letter (e.g., T) to represent a parameter type. This is so that when you read the code, you know where it is coming from.

flerdacodeu · 2018-05-25T08:58:32Z

GabijaBern/assignment1/SingleLinkList.java

+	public void printNode()
+	{
+		System.out.println("Node value is: " + value);
+


Style: this blank line seems spurious.
Not a big difference, but consistent style helps people read your code.

flerdacodeu · 2018-05-25T08:59:44Z

GabijaBern/assignment1/SingleLinkList.java

+	public Node<Type> next;
+	private Type value;
+
+	//constructor method


This comments does not add much information, so it is better left out.
However, you could add a javadoc comment telling people about the data parameter.
In this case is rather obvious, but for more complex code it can be very helpful to do so.

flerdacodeu · 2018-05-25T12:49:27Z

Nicely done.

Thanks for submitting this. I know exams are important!

In terms of exceptions, generally you want to handle checked exceptions (that are declared explicitly by the functions you call). But, in your main() function that's not probably required for this assignment. Any specific questions you had in terms of handling them?

Your submission looks right. You can actually usually whichever tool works best for you: we just did not want to present more than one way that people had to choose from nor impost specific tools. I will take a note to make that explicit (if you already know how to push code to GitHub by command line, please feel free to do so) in next year's GitHub training.

GabijaBern · 2018-05-28T13:23:06Z

Hello, thank you for the reviews. I have edited my tasks according to your feedback. I noticed that once you learn more sophisticated data structures and other, you stop recognizing simple solutions. Is that a problem, that I have approached the tasks with recursion and collections? I will try to improve in these fields: 1. javadoc 2. exceptions. Overall, I will try to write more reusable code. :)

flerdacodeu

Thanks for all the changes! A few more suggestions to improve things. Almost there!

flerdacodeu · 2018-05-30T21:59:43Z

GabijaBern/assignment1/Anagrams.java

+	public static boolean stringSensitive(String firstWord, String secondWord)
+	{
+		//we could first check if those words are the same length
+		if(firstWord.length()!=secondWord.length()) 


Style: it is often better to always include the { and } braces, even around a single statement.
This is so that if you add a second statement, you do not accidentally have it indented as if it were part of the then-branch but actually is not.

flerdacodeu · 2018-05-30T22:00:45Z

GabijaBern/assignment1/Anagrams.java

+	public static boolean stringSensitive(String firstWord, String secondWord)
+	{
+		//we could first check if those words are the same length
+		if(firstWord.length()!=secondWord.length()) 


Style: it often makes the code more readable if you put spaces around operators like in:

if (firstWord.length() != secondWord.length()) { return false; }

flerdacodeu · 2018-05-30T22:01:30Z

GabijaBern/assignment1/Anagrams.java

+		Map<Character, Integer> firstMap = createCharMap(firstWord);
+		Map<Character, Integer> secondMap = createCharMap(secondWord);
+
+		return(firstMap.equals(secondMap));


Style: Return statement do not usually include () around the value (return is a statement, but this makes it look like a function).

flerdacodeu · 2018-05-30T22:04:56Z

GabijaBern/assignment1/Anagrams.java

+public class Anagrams {
+
+
+	//-----------------first part of the question------------------------------


Optional but it would be great if you could apply this suggestion to this assignment as well :)
The point of code reviews is to improve the code before it is merged into the main repository.
On a typical academic assignment, what matter is what you submit; on a engineering task, you want to iterate until it is as good as possible (within reason) before what is merged will stay around (other people will use it and modify it).

flerdacodeu · 2018-05-30T22:06:17Z

GabijaBern/assignment1/Anagrams.java

+	{
+		//we don't want to distinguish lower and upper case letter, so we
+		//just turn all of them to lower
+		firstWord.toLowerCase();


Have you checked this code works? :)
The function toLowerCare() does not modify the string (strings are immutable in Java) but it returns a new string.

flerdacodeu · 2018-05-30T22:21:33Z

GabijaBern/assignment1/Anagrams.java

+				secondListOfMaps.add(createCharMap(secondSentenceWords[index]));
+			}
+
+			return(firstListOfMaps.containsAll(secondListOfMaps));


I think this might not be enough to guarantee that the sentences are anagrams.

Consider:

import java.util.Arrays; import java.util.List; public final class Test { public static void main(String[] args) { List<Integer> l1 = Arrays.asList(new Integer[] {1, 2, 2, 3}); List<Integer> l2 = Arrays.asList(new Integer[] {1, 2, 3, 3}); System.out.println(l1.containsAll(l2)); System.out.println(l2.containsAll(l1)); } }

This code prints true and true. The number of occurrences is not taken into account by containsAll().

What else can we do to solve this issue?

flerdacodeu · 2018-05-30T22:25:59Z

GabijaBern/assignment1/SingleLinkList.java

@@ -0,0 +1,76 @@
+//Q2 Implement an algorithm to find the kth to last element of a singly linked list
+public class SingleLinkList<Type> 


As below: use <T> instead of <Type>.

flerdacodeu · 2018-05-30T22:26:41Z

GabijaBern/assignment1/SingleLinkList.java

+//Q2 Implement an algorithm to find the kth to last element of a singly linked list
+public class SingleLinkList<Type> 
+{
+	private Node<Type> head = null;


Style: Initialising to null and 0 is the default, so it is usually omitted.

Yes, but my teacher always says not to count on default values and make the code clear :) should I still change it?

I think this can be a matter of personal preference, and depend on your team's or company's style guide. The important thing is to always be consistent. If you sometimes use it, and sometimes don't then people don't know what you intended. Personally I agree with your teacher, but I know some teams at Google do the opposite and never add it.

flerdacodeu · 2018-05-30T22:29:27Z

GabijaBern/assignment1/SingleLinkList.java

+}
+
+//class for Node objects
+class Node<T>


A class like this should either be in its own Node.java file or be a nested class of SingleLinkedList. Given that this is used only within the other class, this is a good candidate for a nested class.

Putting a second class within the same source file is misleading (it is hard to find the source, you might actually have two classes with the same name within different classes that would conflict, the class is available to other classes in the package even if it is defined within the file of another class).

flerdacodeu · 2018-05-30T22:30:29Z

GabijaBern/assignment1/SingleLinkList.java

+//class for Node objects
+class Node<T>
+{
+	public final Node<T> next;


The next field cannot be final, because setNext() actually sets it. Probably this triggers a compilation error!

I have done the task in various ways, I tried to improve and added javadocs :)

GabijaBern · 2018-06-05T17:51:09Z

Hello, thank you for the review, I have the last exam tomorrow, I will revise and if I have time I will fix everything before the exam :)

Good morning, sorry for the late updates, this week was very intense exams period... For the anagrams instead of checking if containsAll(), I just delete all the elements and see if all word maps were found. :)

flerdacodeu self-requested a review May 24, 2018 22:48

flerdacodeu requested changes May 25, 2018

View reviewed changes

GabijaBern added 2 commits May 28, 2018 14:13

Updated anagrams task according to the reviews

7514d84

Updated linked list task according to the reviews

11825c5

flerdacodeu requested changes May 30, 2018

View reviewed changes

GabijaBern added 2 commits June 5, 2018 18:39

Second task with Binary trees

12c5a94

I have done the task in various ways, I tried to improve and added javadocs :)

Delete BinaryTreeAncestors.java

b8dd08b

Final changes

e9b351c

Good morning, sorry for the late updates, this week was very intense exams period... For the anagrams instead of checking if containsAll(), I just delete all the elements and see if all word maps were found. :)

		public class Anagrams {


		//-----------------first part of the question------------------------------

		@@ -0,0 +1,76 @@
		//Q2 Implement an algorithm to find the kth to last element of a singly linked list
		public class SingleLinkList<Type>

Add files via upload #2

Are you sure you want to change the base?

Add files via upload #2

Uh oh!

Conversation

GabijaBern commented May 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flerdacodeu commented May 25, 2018

Uh oh!

GabijaBern commented May 28, 2018

Uh oh!

flerdacodeu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flerdacodeu May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GabijaBern commented Jun 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

flerdacodeu May 30, 2018 •

edited

Loading