Ask HN: Is my code any good?
I have written my first iOS app in Swift, a Hacker News Client for iOS. I am a beginner iOS developer. I need some feedback from the Hacker News community about my code quality so that I can improve myself in the future. Be brutal and honest about what you think of my code structure and style.The project can be found at the following URL https://github.com/NikantVohra/HackerNewsClient-iOS.If you want to take a look at the app it can be downloaded from https://itunes.apple.com/us/app/hn-app/id983203003?ls=1&mt=8.
43 comments
Have they fixed testing with Swift yet? I am normally a pretty test driven person, but honestly, Apple makes it pretty annoying to use tests with Swift, so I don't blame you for your lonely scaffold.
I'd say overall it looks pretty well written app code, especially if you say you're a beginner. One thing I'd like to point out is that in my (limited Swift) experience, stuff like this:
https://github.com/NikantVohra/HackerNewsClient-iOS/blob/mas...
...where you're force casting/unboxing/whateveritscalled a bunch of things is a smell. Sometimes it's unavoidable because of the weirdness between Cocoa and Swift, so typically your interface builder outlets will be riddled with that stuff, but for your own internal APIs, try and keep things as option-less as possible.
UI code is pretty challenging to keep organized, especially when platforms encourage using things like two-way bindings and keeping state all over the place. I'd recommend checking out Gary Bernhardt's talk "Boundaries" and Andy Matuschak's similar writings (sorry, can't think of anything off the top of my head), as they have some good ideas for demarcating the line between functional parts of your code that should be super clean and the anything goes world of external UI APIs.
Good luck and keep coding!
I'd say overall it looks pretty well written app code, especially if you say you're a beginner. One thing I'd like to point out is that in my (limited Swift) experience, stuff like this:
https://github.com/NikantVohra/HackerNewsClient-iOS/blob/mas...
...where you're force casting/unboxing/whateveritscalled a bunch of things is a smell. Sometimes it's unavoidable because of the weirdness between Cocoa and Swift, so typically your interface builder outlets will be riddled with that stuff, but for your own internal APIs, try and keep things as option-less as possible.
UI code is pretty challenging to keep organized, especially when platforms encourage using things like two-way bindings and keeping state all over the place. I'd recommend checking out Gary Bernhardt's talk "Boundaries" and Andy Matuschak's similar writings (sorry, can't think of anything off the top of my head), as they have some good ideas for demarcating the line between functional parts of your code that should be super clean and the anything goes world of external UI APIs.
Good luck and keep coding!
Thanks for the feedback :)
Looks pretty nice and clean.
I don't know how you are using firebase in the app. But it seems you aren't using authentication.
Which basically means that anyone can delete all the data in your firebase instance.
I had a look at your code and at a first glance it looks quite good to be your first Swift project. As the others already said, you're missing tests that makes refactoring a bit hard for someone who's new to your code.
Other 2 points I noticed is:
1) You're using CocoaPods but you included SwiftyJSON, Alamofire and a couple of other libraries as plain source code. Why not submodules at least? This makes it hard to update the source of the external dependencies to the latest version
2) You're doing way too many casts in your code. This means that something could be wrong on an architectural level. I would like to help you (I already cloned your repo) but I'm using Swift 1.2 (btw your code is still on Swift 1.1) and I didn't manage to migrate because of point 1).
Apart from that, good job on shipping your app! :)
Other 2 points I noticed is:
1) You're using CocoaPods but you included SwiftyJSON, Alamofire and a couple of other libraries as plain source code. Why not submodules at least? This makes it hard to update the source of the external dependencies to the latest version
2) You're doing way too many casts in your code. This means that something could be wrong on an architectural level. I would like to help you (I already cloned your repo) but I'm using Swift 1.2 (btw your code is still on Swift 1.1) and I didn't manage to migrate because of point 1).
Apart from that, good job on shipping your app! :)
I'm not a ios developer, but you can find some other feedback on https://codereview.stackexchange.com/
Don't know much Swift - but I just wanted to say congrats on shipping something and also for the app - it looks really nice.
Good job.
Regarding the code quality - from my experience you'll be able to also see for yourself if the code is well written when you'll want to change something in the app(e.g. in the next update) and see how easy (or not) it is to do so.
Also my advice will be to take a look at other open source projects and try to see how others have done the same things - what was their approach and how is this different from what you would do in a similar scenario.
Good job.
Regarding the code quality - from my experience you'll be able to also see for yourself if the code is well written when you'll want to change something in the app(e.g. in the next update) and see how easy (or not) it is to do so.
Also my advice will be to take a look at other open source projects and try to see how others have done the same things - what was their approach and how is this different from what you would do in a similar scenario.
Yeah I am already looking into the next update and faced some challenges. Thanks for the advice. Will take care of the same in the future :)
I've not written any swift apps yet, but I have written some objective c and the (effectively) empty test directory is definitely an indicator that this code will be hard to change.
Empty test directory says absolutely nothing how hard this code will be to change.
And seeing that this code is quite well organized, to change it won't be hard at all.
The code is well organized, and as there's relatively little of it, it will be relatively easy for the author to change the code. As he starts to add features and accept pull requests, he will have to manually check via static analysis and manual testing to ensure that the code will still work perfectly.
As the code base grows, doing it this way reliably will become more difficult, to the point that he will begin to fear change.
There is simply no debate that a well tested code base is easier to confidently add features to, confidently refactor and debug than code with no tests at all. This is particularly true if you bring on new contributors who have had no experience with the codebase.
Humans are not computers, they are never as good at repeatedly executing all possible execution paths of an application without growing tired or over confident that it doesn't need to check some path because it's sure it works.
I would take a well tested codebase over a well organized one with no tests any day of the week, because I know I could confidently reorganize the code without breaking anything. (well tested does imply that the tests themselves are well organized, though).
The OP was asking for criticism. He should add tests, lots of 'em.
As the code base grows, doing it this way reliably will become more difficult, to the point that he will begin to fear change.
There is simply no debate that a well tested code base is easier to confidently add features to, confidently refactor and debug than code with no tests at all. This is particularly true if you bring on new contributors who have had no experience with the codebase.
Humans are not computers, they are never as good at repeatedly executing all possible execution paths of an application without growing tired or over confident that it doesn't need to check some path because it's sure it works.
I would take a well tested codebase over a well organized one with no tests any day of the week, because I know I could confidently reorganize the code without breaking anything. (well tested does imply that the tests themselves are well organized, though).
The OP was asking for criticism. He should add tests, lots of 'em.
For those advocating automated tests. Are you actually iOS developers with experience testing ios code? If so, I'm curious what frameworks/tools you've successfully used for unit and integration testing.
We use https://github.com/kif-framework/KIF for automation! But you know the deal on how comprehensively you can do automation test!
How long have you been coding total?
Was Design+Code your sole source for learning how to program?
Was Design+Code your sole source for learning how to program?
No I have been coding now for close to an year now. But this is my first project with Swift. I have also watched the video lectures of the iOS development course offered by Stanford on iTunes.
The design (icons, graphics, screenshots) all look really nice. if you did all of that by yourself, then kudos!
Congratulations on shipping your code!
I'm not a Swift or iOS developer really, so I don't know how those projects are supposed to be organized, but I glanced through your repo and I can see that you seem to be pretty good at breaking things up into small functions and stuff seems readable and well formatted enough to me. Good luck!
I'm not a Swift or iOS developer really, so I don't know how those projects are supposed to be organized, but I glanced through your repo and I can see that you seem to be pretty good at breaking things up into small functions and stuff seems readable and well formatted enough to me. Good luck!
Thanks for looking into the repo. I appreciate your feedback :)
It's useful to get human feedback on one's code, but human attention
is scarce, and human judgement is influenced by many non-technical
factors (e.g. mood, politeness or hostility) that do not derive directly
from code quality.
For these reasons, I recommend complementing internet-based code review by measurement: Use (automated) testing in multiple forms, unit, integration, randomised. Count the number of bugs you encounter. Classify the bugs you find, track how the numbers and classes of bugs in your code evolve over time. Compare those statistics with other coders. This not only gives you ideas about your relative coding ability, but will also reveal areas where you could try and improve your abilites.
For these reasons, I recommend complementing internet-based code review by measurement: Use (automated) testing in multiple forms, unit, integration, randomised. Count the number of bugs you encounter. Classify the bugs you find, track how the numbers and classes of bugs in your code evolve over time. Compare those statistics with other coders. This not only gives you ideas about your relative coding ability, but will also reveal areas where you could try and improve your abilites.
I wanted to get some feedback on my code design and structure. I think that is difficult to get via automated code review.
That's not what your question asks for.
Anyway, in my experience, the ability to test with ease is one dimenision of design quality.
Anyway, in my experience, the ability to test with ease is one dimenision of design quality.
How is that not what was asked? The phrasing of the question makes it immediately obvious, at least to me, that the OP was looking for a design critique. (Unless of course the question was edited since you wrote your initial comment.)
>>That's not what your question asks for.
Maybe you didn't read it, but the question clearly and explicitly says: "Be brutal and honest about what you think of my code structure and style."
Maybe you didn't read it, but the question clearly and explicitly says: "Be brutal and honest about what you think of my code structure and style."
After several years in a test-driven development team, seeing your test directory made me cringe :/
Yeah I know I need follow the best practices. I have started writing tests for the next update. But do you recommend TDD for iOS development?
Devil's advocate: I feel like TDD for iOS is a bit of a wasteland, and don't usually bother with it personally, unless there's some especially dense mission-critical business logic that I need to be 100% confident in.
If you wanna get a lot better, try writing your own testing framework. I wrote a BDD style testing library for Androod when I first started learning Java, and that helped me understand how to write good code for Android platform.
It's not simply best practice - it's necessary to maintain your code.
It is not. There are heaps of code with test which is unmaintainable and heaps of elegant code without test in sight which is easy to maintain.
Cargo culting is rarely the best practice.
In my experience it's useful to add a little more documentation/comments to your code. It makes it a lot more readable for anyone else and more importantly, a lot easier for yourself when you're writing updates a while from now.
I like to have a little comment above every function that explains what it does at least. On top of that I usually put a single line above every logical block of code. Something like "Loop trough list of someobject." It seems obvious but it does make it a lot easier to read back code later.
I like to have a little comment above every function that explains what it does at least. On top of that I usually put a single line above every logical block of code. Something like "Loop trough list of someobject." It seems obvious but it does make it a lot easier to read back code later.
To illustrate, here's something I wrote not too long ago:
https://gitlab.insomnia247.nl/coolfire/pfsense-ident-proxy/b...
It's a little quick-and dirty but it's the way it's commented that makes the point I'm going for here.
It's a little quick-and dirty but it's the way it's commented that makes the point I'm going for here.
Here's an example. You write:
Rather make a function that does the same and name it for example "ValidateIdentResponse". Write a test for it. Call that function from this function. You get shorter, less-complex main function and you don't need that silly comment.
(EDIT: Formatting)
# Check if it's something that looks like an ident response
if( input =~ /^(\d+)(|\s),(|\s)(\d+)$/ )
p1 = $1.to_i
p2 = $4.to_i
else
sane = false
end
Rather make a function that does the same and name it for example "ValidateIdentResponse". Write a test for it. Call that function from this function. You get shorter, less-complex main function and you don't need that silly comment.
(EDIT: Formatting)
Worth keeping in mind that you don't always want to split something into a separate function. When you split out a function, you should be asking yourself if you're actually simplifying something, or if you're just sweeping the complexity under the rug. If the person reading the first function will typically need to see the definition of the helper, the helper's definition is non-obvious, and the helper wouldn't be used elsewhere, it's probably not worth splitting out.
Another important consideration is whether or not the code block you split out makes any assumptions about the current state of the program (or object or whatever), and if those assumptions not being met would result in non-obvious bugs. (If this is true, you should probably wait until there's actual duplication somewhere before splitting the function out, IMO.) This is obviously not an issue with pure functions, but for most programs, most functions aren't pure.
See http://number-none.com/blow/john_carmack_on_inlined_code.htm... for further perspective on this.
Another important consideration is whether or not the code block you split out makes any assumptions about the current state of the program (or object or whatever), and if those assumptions not being met would result in non-obvious bugs. (If this is true, you should probably wait until there's actual duplication somewhere before splitting the function out, IMO.) This is obviously not an issue with pure functions, but for most programs, most functions aren't pure.
See http://number-none.com/blow/john_carmack_on_inlined_code.htm... for further perspective on this.
Valid point. In this case a separate function would be better. It's not always an option though.
Edit: Rather I should say; It's not always an option that makes things better.
If you're writing a comment for a block of code is definitely a good time to think "Should this not be a separate function?" (I clearly need to do this more myself too.)
Edit: Rather I should say; It's not always an option that makes things better.
If you're writing a comment for a block of code is definitely a good time to think "Should this not be a separate function?" (I clearly need to do this more myself too.)
One good resource for learning and inspiration is Robert C. Martin's book Clean Code. Opinionated, yes, but a good read none the less.
Besides unit testing, with regexes I always put a quick comment like "matches foo but not bar or baz" which might not be comprehensive but allows the next person to get a quick understanding of what the code does at a glance.
I like what Jeff Atwood wrote: "Code Tells You How, Comments Tell You Why"
http://blog.codinghorror.com/code-tells-you-how-comments-tel...
http://blog.codinghorror.com/code-tells-you-how-comments-tel...
I would say you're documenting way too much: the obvious shouldn't need documentation.
It's more a matter of what you consider to be obvious in this case. In my experience, especially regex matching can become very not-so-obvious if you're coming back to code you wrote some time ago. Or if someone else is reading it.
# Do NAT table lookup
n = natlookup( p1, p2 )
> On top of that I usually put a single line above every logical block of code. Something like "Loop trough list of someobject." It seems obvious but it does make it a lot easier to read back code later.
I have to respectfully disagree. This is really, really, REALLY BAD advice.
One should strive to write clean code with short functions, well-named variables and parameters, so that it's easy to understand without redundant comments like "loop trough (sic.) list...". Programming languages have looping constructs (e.g. for-statement). Therefore one does not need to write a comment that describes what a for-statement does.
Also, do not write comments that explain what a function does. Rather, name the function so that it is obvious from the name what it does. If the function does multiple things and name would be too long, split the function into logical subroutines and name them all properly.
I have to respectfully disagree. This is really, really, REALLY BAD advice.
One should strive to write clean code with short functions, well-named variables and parameters, so that it's easy to understand without redundant comments like "loop trough (sic.) list...". Programming languages have looping constructs (e.g. for-statement). Therefore one does not need to write a comment that describes what a for-statement does.
Also, do not write comments that explain what a function does. Rather, name the function so that it is obvious from the name what it does. If the function does multiple things and name would be too long, split the function into logical subroutines and name them all properly.
Comments should clarify, not repeat what the code already says.