3 ms·

Well, the client code certainly isn't great. Reading through it random, I see a lot of undocumented code, functions with 20 positional arguments (who wants to b

by t8sr 4y ago

Well, the client code certainly isn't great. Reading through it random, I see a lot of undocumented code, functions with 20 positional arguments (who wants to bet some call sites silently swap two of them?), a file called constants.go where random strings are defined far away from where they're used, etc. There are also random, mostly undocumented, interfaces lying around /everywhere/, in all kinds of places not connected with either the call site or the implementation. My favorite is a custom bools library called "strings". I haven't found any obvious bugs, but the coding standards are poor. Good on them for open sourcing it, but man, did nobody stop and think "hang on, is this code gonna make us look bad?"

rwc 4y ago
This is not a snarky response, but is it possible the answer is simply "who cares?" I see NordVPN ads all over youtube, podcasts, and TV. Those overwhelmingly non-technical customers certainly don't care about the code quality, documentation, or constants defined far from where they're used. It seems to me this is about marketing the product to be perceived as transparent and secure, which is certainly what those customers care about.
Kinrany 4y ago
It could work as a forcing function: "people are saying mean things about our code and it harms our sales, let's rewrite"
ashwagary 4y ago
Apart from being sloppy, any worrying bits of code throwing up security red flags to you?
- weird-eye-issue 4y ago
  No that is why they are nitpicking about recently open sourced code that is "undocumented"
  - ashwagary 4y ago
    I don't see it as a nitpick. The review can be valuable to people working on the code and users wondering how competent the Nord team is.
    - weird-eye-issue 4y ago
      Oh come on. Review? Give me a break. Yeah this review will really give potential Nord users something to really think about before they purchase!
    - noncoml 4y ago
      I hate this kind of “reviews” at work. 0 contribution to the actual functionality of the code, but strong opinions about superficial matters. The OP clearly just skimmed through the code without any interest in understanding how it actually works. Just a code bashing. Sad
      - t8sr 4y ago
        Yeah, I said I just read through it at random. I know that some programmers hate getting their code reviewed. All I can say is that code reviews are, in my experience, strongly correlated with code that's cheaper to maintain and operationally less surprising. Seeing this code certainly makes me think that the team writing it either doesn't have a culture of taking code reviews seriously, or that they don't have a lot of people who know the best practices in this language (Go). Either way, if they're not catching the easy stuff, I don't trust them to catch the subtle stuff either.
Dowwie 4y ago
That's how you ship Rust with resource constraints. You get on with your work and pay down debts as you go. The fear of what other people will think about your code can become seriously debilitating, fucking up project timelines and priorities. Good on the author(s) for sharing. Perfect is the enemy of good enough. I thought people have moved on from using slog for structured logging and instead rely on the same as exposed by the tracing library. In the end, slog was good enough for the task at hand and that's what matters most.
e12e 4y ago
> a file called constants.go where random strings are defined far away from where they're use Wouldn't that be the purpose of such a file? One central location to adjust what some magic number is - then reference it (via eg lsp autocompletion) were used?
gliiics 4y ago
I honestly think answers like this one are the reason more software aren't open-sourced
- t8sr 4y ago
  NordVPN is a professional business making wild claims about the security properties of their product. We are not talking about bashing an amateur gamedev for their coding standards - we're talking about something that supposedly protects your privacy online. I think it's OK to expect a high standard of code review.

by t8sr 4y ago

rwc 4y ago

This is not a snarky response, but is it possible the answer is simply "who cares?" I see NordVPN ads all over youtube, podcasts, and TV. Those overwhelmingly non-technical customers certainly don't care about the code quality, documentation, or constants defined far from where they're used. It seems to me this is about marketing the product to be perceived as transparent and secure, which is certainly what those customers care about.

Kinrany 4y ago

It could work as a forcing function: "people are saying mean things about our code and it harms our sales, let's rewrite"

ashwagary 4y ago

Apart from being sloppy, any worrying bits of code throwing up security red flags to you?

weird-eye-issue 4y ago
No that is why they are nitpicking about recently open sourced code that is "undocumented"
- ashwagary 4y ago
  I don't see it as a nitpick. The review can be valuable to people working on the code and users wondering how competent the Nord team is.
  - weird-eye-issue 4y ago
    Oh come on. Review? Give me a break. Yeah this review will really give potential Nord users something to really think about before they purchase!
  - noncoml 4y ago
    I hate this kind of “reviews” at work. 0 contribution to the actual functionality of the code, but strong opinions about superficial matters. The OP clearly just skimmed through the code without any interest in understanding how it actually works. Just a code bashing. Sad
    - t8sr 4y ago
      Yeah, I said I just read through it at random. I know that some programmers hate getting their code reviewed. All I can say is that code reviews are, in my experience, strongly correlated with code that's cheaper to maintain and operationally less surprising. Seeing this code certainly makes me think that the team writing it either doesn't have a culture of taking code reviews seriously, or that they don't have a lot of people who know the best practices in this language (Go). Either way, if they're not catching the easy stuff, I don't trust them to catch the subtle stuff either.

Dowwie 4y ago

That's how you ship Rust with resource constraints. You get on with your work and pay down debts as you go. The fear of what other people will think about your code can become seriously debilitating, fucking up project timelines and priorities. Good on the author(s) for sharing. Perfect is the enemy of good enough. I thought people have moved on from using slog for structured logging and instead rely on the same as exposed by the tracing library. In the end, slog was good enough for the task at hand and that's what matters most.

e12e 4y ago

> a file called constants.go where random strings are defined far away from where they're use Wouldn't that be the purpose of such a file? One central location to adjust what some magic number is - then reference it (via eg lsp autocompletion) were used?

gliiics 4y ago

I honestly think answers like this one are the reason more software aren't open-sourced

t8sr 4y ago
NordVPN is a professional business making wild claims about the security properties of their product. We are not talking about bashing an amateur gamedev for their coding standards - we're talking about something that supposedly protects your privacy online. I think it's OK to expect a high standard of code review.