Pages:
Author

Topic: Nxt source code analysis (QA) - page 9. (Read 14115 times)

hero member
Activity: 854
Merit: 500
January 03, 2014, 03:25:11 PM
#35
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 Smiley ), I'm starting this thread. Not for bounty, but for better quality (but donations welcome, as usual Smiley ).

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.
sr. member
Activity: 428
Merit: 252
January 03, 2014, 03:18:25 PM
#34
For us common folks, what does this mean in plan english?  Grin
legendary
Activity: 868
Merit: 1000
Cryptotalk.org - Get paid for every post!
January 03, 2014, 03:02:18 PM
#33
Found
Code:
static byte[] convert(String string)
and
Code:
static String convert(byte[] bytes)
ineffictive.
Need I suggest some solutions (like Apache Commons Smiley )?

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.
legendary
Activity: 868
Merit: 1000
Cryptotalk.org - Get paid for every post!
January 03, 2014, 02:59:25 PM
#32
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 Smiley ), I'm starting this thread. Not for bounty, but for better quality (but donations welcome, as usual Smiley ).

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?
legendary
Activity: 1441
Merit: 1000
Live and enjoy experiments
January 03, 2014, 02:55:28 PM
#31
watching,  statically  Grin
hero member
Activity: 840
Merit: 1002
Simcoin Developer
January 03, 2014, 10:19:18 AM
#30
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 Smiley

That could be a code review, which is never a bad thing. And Alex would have occasional pocket money on the side Smiley
hero member
Activity: 784
Merit: 500
January 03, 2014, 10:18:31 AM
#29
Hire this guy. If only for his nick Smiley

yes, do it asap! Smiley
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...
sr. member
Activity: 392
Merit: 250
January 03, 2014, 10:09:27 AM
#28
Hire this guy. If only for his nick Smiley

yes, do it asap! Smiley
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.
sr. member
Activity: 392
Merit: 250
January 03, 2014, 09:42:14 AM
#27
Found
Code:
static byte[] convert(String string)
and
Code:
static String convert(byte[] bytes)
ineffictive.
Need I suggest some solutions (like Apache Commons Smiley )?
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.
hero member
Activity: 784
Merit: 501
January 03, 2014, 09:29:53 AM
#26
Found
Code:
static byte[] convert(String string)
and
Code:
static String convert(byte[] bytes)
ineffictive.
Need I suggest some solutions (like Apache Commons Smiley )?

Edit: nevermind. I'm too strict this evening...
hero member
Activity: 784
Merit: 500
January 03, 2014, 09:26:49 AM
#25
Hire this guy. If only for his nick Smiley

yes, do it asap! Smiley
hero member
Activity: 840
Merit: 1002
Simcoin Developer
January 03, 2014, 09:12:10 AM
#24
Hire this guy. If only for his nick Smiley
sr. member
Activity: 392
Merit: 250
January 03, 2014, 09:02:13 AM
#23
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.
Quote
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... Wink
Quote
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. Smiley Significant refactoring is needed, but no time for it now, need to fix the critical bugs first.
Quote
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.
hero member
Activity: 784
Merit: 501
January 03, 2014, 08:58:52 AM
#22
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...
sr. member
Activity: 392
Merit: 250
January 03, 2014, 08:53:27 AM
#21
I hope you already remove "double copy"
Code:
peers = ((HashMap)Nxt.peers.clone()).values();
from thread, which is start every second Smiley
Yes. There is no invocation of clone() in 0.4.9e.
hero member
Activity: 784
Merit: 501
January 03, 2014, 08:53:15 AM
#20
Just make sure you don't give him/them the whole 0.4.9e code in this topic!   Wink
I have no time to be full-time Nxt core developer Sad
sr. member
Activity: 392
Merit: 250
January 03, 2014, 08:52:02 AM
#19
AFAIR, HashMap.values() make a copy, while HashMap.entrySet() definitely not.
So my variant is even better. You sync in peers anyway, so it is safe.
I didn't know that, are you sure? From the java source of ConcurrentHashMap, which Nxt.peers already is in 0.4.9e:
Code:
    /**
     * Returns a {@link Collection} view of the values contained in this map.
     * The collection is backed by the map, so changes to the map are
     * reflected in the collection, and vice-versa.  The collection
     * supports element removal, which removes the corresponding
     * mapping from this map, via the Iterator.remove,
     * Collection.remove, removeAll,
     * retainAll, and clear operations.  It does not
     * support the add or addAll operations.
     *
     *

The view's iterator is a "weakly consistent" iterator
     * that will never throw {@link ConcurrentModificationException},
     * and guarantees to traverse elements as they existed upon
     * construction of the iterator, and may (but is not guaranteed to)
     * reflect any modifications subsequent to construction.
     */
    public Collection values() {
        Collection vs = values;
        return (vs != null) ? vs : (values = new Values());
    }

sr. member
Activity: 299
Merit: 250
January 03, 2014, 08:50:37 AM
#18
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.

Two examples:
(1) Around all the parseInts where you're loading config, you should be catching the more specific NumberFormatException (which you can handle) than Exception (which you probably can't).
(2) It does seem odd that if saveTransactions fails by throwing an exception (which it can) the code just ignores it and keeps going. I would expect it to do something in that case.

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?

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

The file handles on error conditions:

Code:
		static void loadBlocks(String fileName) throws Exception {

FileInputStream fileInputStream = new FileInputStream(fileName);
ObjectInputStream objectInputStream = new ObjectInputStream(fileInputStream);
blockCounter = objectInputStream.readInt();
blocks = (HashMap)objectInputStream.readObject();
lastBlock = objectInputStream.readLong();
objectInputStream.close();
fileInputStream.close();

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

That's all I have for now, but I'll probably look the code more later Smiley.
         
      }
hero member
Activity: 784
Merit: 501
January 03, 2014, 08:49:14 AM
#17
I hope you already remove "double copy"
Code:
peers = ((HashMap)Nxt.peers.clone()).values();
from thread, which is start every second Smiley
hero member
Activity: 784
Merit: 501
January 03, 2014, 08:47:19 AM
#16
My replacement of the same lines in getAnyPeer, already in 0.4.9e:
Code:
            List selectedPeers = new ArrayList();

            for (Peer peer : Nxt.peers.values()) {

                if (peer.blacklistingTime <= 0 && peer.state == state && peer.announcedAddress.length() > 0
                        && (!applyPullThreshold || !enableHallmarkProtection || peer.getWeight() >= pullThreshold)) {

                    selectedPeers.add(peer);

                }

            }

AFAIR, HashMap.values() make a copy, while HashMap.entrySet() definitely not.
So my variant is even better. You sync in peers anyway, so it is safe.
Pages:
Jump to: