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

Code metrics improvements / code optimizations / production (readability/coupling/complexity) #66

Closed
wants to merge 14 commits into from

Conversation

default-writer
Copy link

LinkedList optimizations:
-added counter variable
-added overflow/underflow exceptions
-moved counter to read-only property
-moved code to function nodeAtIndex function
-changed variables to optionals for readability
-removed overcompilcated code blocks
-removed Enumerable constraint for T
-removed print

LLNode optimizations:
-added default init and parametrized init to LLNode

Tests modifications:
-added try! code block to bypass exception handling

…ity/coupling/complexity)

LinkedList<T> optimizations:
-added counter variable
-added overflow/underflow exceptions
-moved counter to read-only property
-moved code to function nodeAtIndex function
-changed variables to optionals for readability
-removed overcompilcated code blocks
-removed Enumerable constraint for T
-removed print

LLNode<T> optimizations:
-added default init and parametrized init to LLNode<T>

Tests modifications:
-added try! code block to bypass exception handling
-added formatting / removed extra lines /
-added SequenceType protocol implementation
-adjusted test/code to match the changes
-added default initializer and parameterized initializer (Array<T>!)
-added self-described functions for readability
-added ArrayLiteralConvertible protocol requirements
-moved some optionals to values in safe function calls
-moved code to contained class/miscellaneous code improvements (LLNode<T>)

Stack<T>:
-removed complexity
-added count > 0 checks before pop()

LLNode<T>:
-added complexity to LLNode<T> from LinkedList<T> to lower coupling
-removed default initializer

Tests:
-removed one extra test (LLNode<T>)
-moved from optional to value in filter test (LinkedList<T>)
-addLinkAtIndex to empty list is not allowed by now
-added removeNode parameter
-added testLinkedListArrayLiteralConvertible test
-added testLinkedListStructure test
-removed helper function (buildLinkedList)
@default-writer
Copy link
Author

Currently working at
default-writer@50f8ede

My opinion that it is essential to replace spaces with tabs, because different users like different tabs on XCode [Preferences - > Text Editing -> Indentation].

I prefer 2, GitHub prefers 8, so for commits using tabs it will not overcommit the lines and extra spaces

-removed uncovered function (removeNode())
-removed extra brackets (removeLinkAtIndex)
@morganchen12
Copy link
Contributor

My concern with tabs vs spaces is Xcode's default is to use spaces to indent. Either way, the precedent for tabs vs spaces shouldn't be evaluated on a case-by-case basis per PR; someone should add a style guide or contributing guide to this repo. Until then, you may want to stick with spaces since the rest of the project already uses spaces--but, if you don't, it's not such a big deal.

@morganchen12
Copy link
Contributor

There's a few places (1, 2) where force-unwraps aren't necessary.

-removed force unwraps (@morganchen12)
@morganchen12
Copy link
Contributor

For the map and filter functions, consumers probably shouldn't have to know about the LLNode type, since that would be revealing the implementation details of the linked list in its public interface.

func map<U>(transform: T -> U) -> LinkedList<U>

instead of

func map(formula: LLNode<T> -> T) -> LinkedList<T>

@morganchen12
Copy link
Contributor

There are a few defer closures that are followed immediately by return statements, which seems unnecessary.

@morganchen12
Copy link
Contributor

the addLink() and addNode() methods could maybe be renamed to indicate how the values are added (i.e. append, prepend), but naming is hard and verbosity can feel awkward.

-removed LLNode<T> dependency for map function (@morganchen12)
@default-writer
Copy link
Author

@morganchen12 Hi! Could you advice easy to read names for the duplicate function naming (if i understood remark correctly) for addNode, addLink functions? (i.e. append<...>, prepend<...>)? I'm not a native speaker and do not understand the mush difference and verbosity then

@morganchen12
Copy link
Contributor

addLinkAtIndex(key: index:) could be renamed to addValue(_: atIndex:), addLink(key:) could be renamed to prepend(key:). Other than that it looks fine. Naming is also partially opinion, so you're free to propose alternatives.

-removed filter dependency on LLNode<T>
-removed extra defer in loop (waynewbishop#66 (comment))
-removed ! requirements for several params/return values (waynewbishop#66 (comment))
-fixed error in test case (testLinkedListArrayLiteralConvertible())
@waynewbishop
Copy link
Owner

Thanks for submitting this pull request. Since this submission the code has been changed considerably. Feel free to check the latest version on develop if there are additional changes you'd like to see.

Regards;

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.

None yet

4 participants