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

Swift implementation #13

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

Swift implementation #13

wants to merge 1 commit into from

Conversation

mephistophele-s
Copy link

No description provided.

Copy link

@GYFK GYFK left a comment

Choose a reason for hiding this comment

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

@anastasiaSTR just a brief review with few changes. I suggest revisioning the following work by yourself and making proper changes with having what was commented in HowProgrammingWorks/LinkedList#16 in mind as there are lots of improvements that have to be done to this implementation.

import Foundation


class Node<T>: CustomStringConvertible where T: CustomStringConvertible, T: Comparable {
Copy link

@GYFK GYFK Jun 7, 2017

Choose a reason for hiding this comment

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

That is not the preferred code style and is wrong in the means of your task. It is mainly used when you have several Generic and try to constrain them like:

class Nodes<A, B, C> where A: Comparable, B: Equatable, C: CustomStringConvertible {
	
}

It may be better to use &here.

class Node<T>: CustomStringConvertible where T: CustomStringConvertible & Comparable {

because you use only one Generic and only a few protocols. When you have more protocols It is better to use a typealias

typealias NodeRelatedProtocol = CustomStringConvertible & Comparable & Equatable

class Node<T>: CustomStringConvertible where T: NodeRelatedProtocol {

Yet for this task you should use extensions combined with the element-based behavior.

public class Node<T> {

	public typealias Element = T
	public let value: Element
	
	public init(with value: Element) {
		self.value = value
	}
}

public extension Node where Node.Element: Equatable {
	
	func nodeValueEqualTo(valueOf aNode: Node) -> Bool {
		return self.value == aNode.value
	}
}

}

func delete() {
parentNode!.subNodes = parentNode!.subNodes.filter({ $0 !== self })
Copy link

@GYFK GYFK Jun 7, 2017

Choose a reason for hiding this comment

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

That's a very bad idea combined with using the value of parentNode as a Node? with the write access from outside the class.

If the user tries for some reason doing the following

let tree = Tree<Int>()
tree.root.value = 99
tree.root.parentNode = nil
tree.root.delete()

The result is predictable.

Copy link

@GreatAndPowerfulKing GreatAndPowerfulKing left a comment

Choose a reason for hiding this comment

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

I completely agree with @GYFK. Also I would advise you not to require conformance to CustomStringConvertible from T.

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