Nxt source code analysis (QA)

[Jean-Luc]

I hope you already remove "double copy"

Code:

peers = ((HashMap<String, Peer>)Nxt.peers.clone()).values();

from thread, which is start every second

Yes. There is no invocation of clone() in 0.4.9e.

[ImmortAlex]

I didn't know that, are you sure? From the java source of ConcurrentHashMap, which Nxt.peers already is in 0.4.9e:

You are right, values backed by the map.
Hmm, I remember the times when values() return copy of map. Or my memory is very bad...

[Jean-Luc]

There are also a lot of places where the base Exception is being caught and suppressed. It's generally best to only catch exceptions that you can handle, otherwise, you're just continuing without knowing what went wrong and your code could be in a bad state.

Absolutely agree, haven't had the time to fix in 0.4.9e.

I'm unsure what you're seeding transactions.nxt with when it's not found? Are those the genesis block transactions? Shouldn't they be getting downloaded from somewhere?

Those are the genesis block transactions, hardcoded in the code. My account number is somewhere there too...

It's kind of awkward to have a private class (e.g. Blocks) populate a member of the containing type directly (e.g. Blocks.loadBlocks assigns Nxt.blocks directly). It looks like you're using classes primarily for namespacing (e.g. grouping related functions) instead of creating standalone classes. For example, the Blocks class cannot be used anywhere else because it has hard dependencies on Nxt (which is also doing server stuff). Might not be a big deal in your case, but it's usually better to minimize class dependencies by creating small, reusable classes with a single purpose (in other words, following the SOLID principle).

Agree. The code doesn't exactly follow any textbook design patterns either. Significant refactoring is needed, but no time for it now, need to fix the critical bugs first.

If read object throws here, neither stream will be closed. You should consider moving the closes to a finally block.

Rest assured, all streams are safely closed in finally blocks (or actually using java 7 try-with-resources) in 0.4.9e.

[NxtChg]

Hire this guy. If only for his nick

[mcjavar]

Hire this guy. If only for his nick

yes, do it asap!

[ImmortAlex]

Found

Code:

static byte[] convert(String string)

and

Code:

static String convert(byte[] bytes)

ineffictive.
Need I suggest some solutions (like Apache Commons )?

Edit: nevermind. I'm too strict this evening...

[Jean-Luc]

Found

Code:

static byte[] convert(String string)

and

Code:

static String convert(byte[] bytes)

ineffictive.
Need I suggest some solutions (like Apache Commons )?

Those convert methods also bothered me at first look, but I decided to leave it for later if they need optimizing. My girlfriend promised to buy me a license for YourKit (hey, they have a sale now, personal license is only $99 instead of $500) and I will spend some time profiling. It is only worth optimizing things that really are measured to be a problem.

[Jean-Luc]

Hire this guy. If only for his nick

yes, do it asap!

Well, he just mentioned a few posts above that he is too busy to be a full time developer. But if ImmortAlex wants to contribute part time, we seem to think similarly so this may work.

The problem right now is it will be difficult to split the work between multiple developers, as you can see everything is in one huge file which needs a lot of refactoring. Distributing who can do what without stepping on each others work, and then merging the changes, will take almost the same time as one person doing it all.

[mcjavar]

Hire this guy. If only for his nick

yes, do it asap!

Well, he just mentioned a few posts above that he is too busy to be a full time developer. But if ImmortAlex wants to contribute part time, we seem to think similarly so this may work.

The problem right now is it will be difficult to split the work between multiple developers, as you can see everything is in one huge file which needs a lot of refactoring. Distributing who can do what without stepping on each others work, and then merging the changes, will take almost the same time as one person doing it all.

So you plan to split it in different files? I am not an expert, but did some java before. When I saw that there is only one file I was shocked, to be honest...

[NxtChg]

The problem right now is it will be difficult to split the work between multiple developers, as you can see everything is in one huge file which needs a lot of refactoring. Distributing who can do what without stepping on each others work, and then merging the changes, will take almost the same time as one person doing it all.

Hey, you see he's good at proofreading

That could be a code review, which is never a bad thing. And Alex would have occasional pocket money on the side

[bitcool]

watching,  statically 

[FrictionlessCoin]

While lurking for injected bugs, I found that some parts of Nxt code makes my eyes bleed. With hope that I have enough programming skills (probably better, than my runglish ), I'm starting this thread. Not for bounty, but for better quality (but donations welcome, as usual ).

First of all, thanks to IntelliJ IDEA code analysis, I see a lot of syncronized with potentially wrong usage.
Static variables like peers, blocks, accounts, users are not final. They are initialized at start, so it is not a big flaw right now, but who knows about future.
When I look more deeply in syncronizations, I see a lot of places, where author get content of such global collection and then sync on it, but not sync on collection itself. Example:

Code:

Block.analyse() method:

Account generatorAccount = accounts.get(Account.getId(generatorPublicKey));
synchronized (generatorAccount) {
    generatorAccount.setBalance(generatorAccount.balance + totalFee * 100L);
    generatorAccount.setUnconfirmedBalance(generatorAccount.unconfirmedBalance + totalFee * 100L);
}

While in some places such usage may be safe, I think it's very bad practice anyway.

More to come...

The author did not even bother to run the code through static code analysis.

Also,  where the hell are the JUnit tests?


How can anyone expect this kind of code to be used to handle their currency?

[FrictionlessCoin]

Found

Code:

static byte[] convert(String string)

and

Code:

static String convert(byte[] bytes)

ineffictive.
Need I suggest some solutions (like Apache Commons )?

Edit: nevermind. I'm too strict this evening...

I don't think this developer has even heard of Apache commons.

I mean, it is obvious that he doesn't even know how to use a java package.

[Revelations86]

For us common folks, what does this mean in plan english? 

[GröBkAz]

While lurking for injected bugs, I found that some parts of Nxt code makes my eyes bleed. With hope that I have enough programming skills (probably better, than my runglish ), I'm starting this thread. Not for bounty, but for better quality (but donations welcome, as usual ).

First of all, thanks to IntelliJ IDEA code analysis, I see a lot of syncronized with potentially wrong usage.
Static variables like peers, blocks, accounts, users are not final. They are initialized at start, so it is not a big flaw right now, but who knows about future.
When I look more deeply in syncronizations, I see a lot of places, where author get content of such global collection and then sync on it, but not sync on collection itself. Example:

Code:

Block.analyse() method:

Account generatorAccount = accounts.get(Account.getId(generatorPublicKey));
synchronized (generatorAccount) {
    generatorAccount.setBalance(generatorAccount.balance + totalFee * 100L);
    generatorAccount.setUnconfirmedBalance(generatorAccount.unconfirmedBalance + totalFee * 100L);
}

While in some places such usage may be safe, I think it's very bad practice anyway.

More to come...

The author did not even bother to run the code through static code analysis.

Also,  where the hell are the JUnit tests?


How can anyone expect this kind of code to be used to handle their currency?

I do not think that this should be taken here as an opportunity to insult the developer. Please, show more humility for the work of others, even if you dont like the project.

[FrictionlessCoin]

While lurking for injected bugs, I found that some parts of Nxt code makes my eyes bleed. With hope that I have enough programming skills (probably better, than my runglish ), I'm starting this thread. Not for bounty, but for better quality (but donations welcome, as usual ).

First of all, thanks to IntelliJ IDEA code analysis, I see a lot of syncronized with potentially wrong usage.
Static variables like peers, blocks, accounts, users are not final. They are initialized at start, so it is not a big flaw right now, but who knows about future.
When I look more deeply in syncronizations, I see a lot of places, where author get content of such global collection and then sync on it, but not sync on collection itself. Example:

Code:

Block.analyse() method:

Account generatorAccount = accounts.get(Account.getId(generatorPublicKey));
synchronized (generatorAccount) {
    generatorAccount.setBalance(generatorAccount.balance + totalFee * 100L);
    generatorAccount.setUnconfirmedBalance(generatorAccount.unconfirmedBalance + totalFee * 100L);
}

While in some places such usage may be safe, I think it's very bad practice anyway.

More to come...

The author did not even bother to run the code through static code analysis.

Also,  where the hell are the JUnit tests?


How can anyone expect this kind of code to be used to handle their currency?

I do not think that this should be taken here as an opportunity to insult the developer. Please, show more humility for the work of others, even if you dont like the project.

This code is supposed to secure the currency that people own,   so I expect top notch software development best practices, not work that looks like it came out of a high school coding project.

I am teling you like it is,  if you can't handle the truth then so be it.

[FrictionlessCoin]

For us common folks, what does this mean in plan english? 

in plain english... the code has not been tested exhaustively.

So its like flying an airplane that hasn't been tested to fly.

[FrictionlessCoin]

I hope you already remove "double copy"

Code:

peers = ((HashMap<String, Peer>)Nxt.peers.clone()).values();

from thread, which is start every second

Which is dumb as hell because concurrent collections handles this kind of stuff.

So why is this poorly designed code out in the wild securing a currency?

It is only a matter of time before some guy drains out all the money out of all accounts.

This is a ticking time bomb!

Don't tell me I did not warn you folks.

[theKnownUnknown]

Now we know why this source code was not published before. This is a ticking time bomb. If you try to refactor this, it will explode. Do not take it personally  . Just my opininion and experience.

[NXT]

While lurking for injected bugs, I found that some parts of Nxt code makes my eyes bleed. With hope that I have enough programming skills (probably better, than my runglish ), I'm starting this thread. Not for bounty, but for better quality (but donations welcome, as usual ).

First of all, thanks to IntelliJ IDEA code analysis, I see a lot of syncronized with potentially wrong usage.
Static variables like peers, blocks, accounts, users are not final. They are initialized at start, so it is not a big flaw right now, but who knows about future.
When I look more deeply in syncronizations, I see a lot of places, where author get content of such global collection and then sync on it, but not sync on collection itself. Example:

Code:

Block.analyse() method:

Account generatorAccount = accounts.get(Account.getId(generatorPublicKey));
synchronized (generatorAccount) {
    generatorAccount.setBalance(generatorAccount.balance + totalFee * 100L);
    generatorAccount.setUnconfirmedBalance(generatorAccount.unconfirmedBalance + totalFee * 100L);
}

While in some places such usage may be safe, I think it's very bad practice anyway.

More to come...

The author did not even bother to run the code through static code analysis.

Also,  where the hell are the JUnit tests?


How can anyone expect this kind of code to be used to handle their currency?

I do not think that this should be taken here as an opportunity to insult the developer. Please, show more humility for the work of others, even if you dont like the project.

This code is supposed to secure the currency that people own,   so I expect top notch software development best practices, not work that looks like it came out of a high school coding project.

I am teling you like it is,  if you can't handle the truth then so be it.

hey mister big mouth, if its not safe break it right now or stfu