
Ask HN: Is my code any good? - nikant
 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:&#x2F;&#x2F;github.com&#x2F;NikantVohra&#x2F;HackerNewsClient-iOS.If you want to take a look at the app it can be downloaded from https:&#x2F;&#x2F;itunes.apple.com&#x2F;us&#x2F;app&#x2F;hn-app&#x2F;id983203003?ls=1&amp;mt=8.
======
bigbento
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...](https://github.com/NikantVohra/HackerNewsClient-
iOS/blob/master/HackerNews/StoryCell.swift#L45-L49)

...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!

~~~
nikant
Thanks for the feedback :)

------
fmax30
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.

------
vittoriom
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! :)

------
tigroferoce
I'm not a ios developer, but you can find some other feedback on
[https://codereview.stackexchange.com/](https://codereview.stackexchange.com/)

------
alc90
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.

~~~
nikant
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 :)

------
eric_h
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.

~~~
rimantas
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.

~~~
eric_h
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.

------
tylerc230
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.

~~~
purans
We use [https://github.com/kif-framework/KIF](https://github.com/kif-
framework/KIF) for automation! But you know the deal on how comprehensively
you can do automation test!

------
zoskia
How long have you been coding total?

Was Design+Code your sole source for learning how to program?

~~~
nikant
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.

~~~
avinassh
The design (icons, graphics, screenshots) all look really nice. if you did all
of that by yourself, then kudos!

------
gfosco
Looks pretty good to me. Congrats on launching an app!

~~~
nikant
Thanks :)

------
WorldWideWayne
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!

~~~
nikant
Thanks for looking into the repo. I appreciate your feedback :)

------
mafribe
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.

~~~
nikant
I wanted to get some feedback on my code design and structure. I think that is
difficult to get via automated code review.

~~~
mafribe
That's not what your question asks for.

Anyway, in my experience, the ability to test with ease is one dimenision of
design quality.

~~~
RussianCow
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.)

------
GaiusCoffee
After several years in a test-driven development team, seeing your test
directory made me cringe :/

~~~
nikant
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?

~~~
ninjakeyboard
It's not simply best practice - it's necessary to maintain your code.

~~~
rimantas
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.

------
cFire
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.

~~~
cFire
To illustrate, here's something I wrote not too long ago:
[https://gitlab.insomnia247.nl/coolfire/pfsense-ident-
proxy/b...](https://gitlab.insomnia247.nl/coolfire/pfsense-ident-
proxy/blob/master/identproxy.rb)

It's a little quick-and dirty but it's the way it's commented that makes the
point I'm going for here.

~~~
kyberias
Here's an example. You write:

    
    
      # 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
    

Don't do this (comment a block of code).

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)

~~~
cFire
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.)

~~~
kyberias
One good resource for learning and inspiration is Robert C. Martin's book
Clean Code. Opinionated, yes, but a good read none the less.

