[sip-comm-dev] Re: svn commit: r5130 - trunk/src/net/java/sip/communicator: impl/gui/main/chat/conference impl/protocol/icq service/protocol


#1

Fellow developers,

More as a plea and much less as a lesson, I'd like to remind the following:

- Non-static inner classes have an implicit reference to the instance
of the outer class. If the inner class doesn't need access to the
instance of the outer class, the implicit reference is a waste.
Consequently, prefer static inner classes to non-static one.

- Directly calling Integer, Long, etc. constructors is inefficient. In
stead of new Integer(int), use Integer.valueOf(int) or just use
autoboxing where possible. new Integer(String) is supposed to be
written Integer.parseInt(String). new Integer(int).toString() is to be
replaced with Integer.toString(int).

I guess "premature optimization is the root of all evil" has started
chiming in at this point. While it may as well be true (though these
"optimizations" don't really affect the design), the fact is that
these cause warnings in a list of more than a thousand and thus shadow
real problems such as the following where the bit masks 0xc0 and 0x02
are incompatible and cause the method to always return false:

    public boolean is6to4()
    {
        return address instanceof Inet6Address
            && (address.getAddress()[0] & 0xff) == 0x20
            && (address.getAddress()[0] & 0xc0) == 0x02;
    }

The default warnings in Eclipse are currently ~4600 and I have ~1000
in various static code analysis tools so it would help me a lot to add
as few as possible new warnings.

Thank you,
Lubo

···

On Fri, Mar 13, 2009 at 10:38 PM, <lubomir_m@dev.java.net> wrote:

Author: lubomir_m
Date: 2009-03-13 20:38:23+0000
New Revision: 5130

Modified:
trunk/src/net/java/sip/communicator/impl/gui/main/chat/conference/ConferenceChatManager.java
trunk/src/net/java/sip/communicator/impl/protocol/icq/ProtocolProviderServiceIcqImpl.java
trunk/src/net/java/sip/communicator/service/protocol/ChatRoomMemberRole.java

Log:
Avoids Integer allocations, removes unnecessary fields by making inner classes static.

Modified: trunk/src/net/java/sip/communicator/impl/gui/main/chat/conference/ConferenceChatManager.java
Url: https://sip-communicator.dev.java.net/source/browse/sip-communicator/trunk/src/net/java/sip/communicator/impl/gui/main/chat/conference/ConferenceChatManager.java?view=diff&rev=5130&p1=trunk/src/net/java/sip/communicator/impl/gui/main/chat/conference/ConferenceChatManager.java&p2=trunk/src/net/java/sip/communicator/impl/gui/main/chat/conference/ConferenceChatManager.java&r1=5129&r2=5130

--- trunk/src/net/java/sip/communicator/impl/gui/main/chat/conference/ConferenceChatManager.java (original)
+++ trunk/src/net/java/sip/communicator/impl/gui/main/chat/conference/ConferenceChatManager.java 2009-03-13 20:38:23+0000
@@ -913,7 +913,8 @@
/**
* Joins a chat room in an asynchronous way.
*/
- private class JoinChatRoomTask extends SwingWorker<String, Object>
+ private static class JoinChatRoomTask
+ extends SwingWorker<String, Object>
{
private static final String SUCCESS = "Success";

@@ -932,11 +933,11 @@
private static final String UNKNOWN_ERROR
= "UnknownError";

- private ChatRoomWrapper chatRoomWrapper;
+ private final ChatRoomWrapper chatRoomWrapper;

- private String nickName;
+ private final String nickName;

- private byte[] password;
+ private final byte[] password;

    JoinChatRoomTask\(   ChatRoomWrapper chatRoomWrapper,
                        String nickName,

@@ -971,29 +972,19 @@
logger.trace("Failed to join chat room: "
+ chatRoom.getName(), e);

- if(e.getErrorCode()
- == OperationFailedException.AUTHENTICATION_FAILED)
+ switch (e.getErrorCode())
{
+ case OperationFailedException.AUTHENTICATION_FAILED:
return AUTHENTICATION_FAILED;
- }
- else if(e.getErrorCode()
- == OperationFailedException.REGISTRATION_REQUIRED)
- {
+ case OperationFailedException.REGISTRATION_REQUIRED:
return REGISTRATION_REQUIRED;
- }
- else if(e.getErrorCode()
- == OperationFailedException.PROVIDER_NOT_REGISTERED)
- {
+ case OperationFailedException.PROVIDER_NOT_REGISTERED:
return PROVIDER_NOT_REGISTERED;
- }
- else if(e.getErrorCode()
- == OperationFailedException
- .SUBSCRIPTION_ALREADY_EXISTS)
- {
+ case OperationFailedException.SUBSCRIPTION_ALREADY_EXISTS:
return SUBSCRIPTION_ALREADY_EXISTS;
- }
- else
+ default:
return UNKNOWN_ERROR;
+ }
}
}

@@ -1070,11 +1061,12 @@
/**
* Finds a chat room in asynchronous way.
*/
- private class FindRoomTask extends SwingWorker<ChatRoom, Object>
+ private static class FindRoomTask
+ extends SwingWorker<ChatRoom, Object>
{
- private String chatRoomName;
+ private final String chatRoomName;

- private ChatRoomProviderWrapper chatRoomProvider;
+ private final ChatRoomProviderWrapper chatRoomProvider;

    FindRoomTask\(   String chatRoomName,
                    ChatRoomProviderWrapper chatRoomProvider\)

@@ -1109,9 +1101,10 @@
}
}

- private class FindAllRoomsTask extends SwingWorker<List<String>, Object>
+ private static class FindAllRoomsTask
+ extends SwingWorker<List<String>, Object>
{
- private ChatRoomProviderWrapper chatRoomProvider;
+ private final ChatRoomProviderWrapper chatRoomProvider;

    FindAllRoomsTask\(ChatRoomProviderWrapper provider\)
    \{

Modified: trunk/src/net/java/sip/communicator/impl/protocol/icq/ProtocolProviderServiceIcqImpl.java
Url: https://sip-communicator.dev.java.net/source/browse/sip-communicator/trunk/src/net/java/sip/communicator/impl/protocol/icq/ProtocolProviderServiceIcqImpl.java?view=diff&rev=5130&p1=trunk/src/net/java/sip/communicator/impl/protocol/icq/ProtocolProviderServiceIcqImpl.java&p2=trunk/src/net/java/sip/communicator/impl/protocol/icq/ProtocolProviderServiceIcqImpl.java&r1=5129&r2=5130

--- trunk/src/net/java/sip/communicator/impl/protocol/icq/ProtocolProviderServiceIcqImpl.java (original)
+++ trunk/src/net/java/sip/communicator/impl/protocol/icq/ProtocolProviderServiceIcqImpl.java 2009-03-13 20:38:23+0000
@@ -814,7 +814,7 @@
return aimConnection;
}

- public class AimIcbmListener implements IcbmListener
+ public static class AimIcbmListener implements IcbmListener
{

    public void newConversation\(IcbmService service, Conversation conv\)

@@ -834,7 +834,6 @@
net.kano.joustsim.oscar.oscar.service.icbm.Message message,
Set triedConversations)
{
-
}
}

Modified: trunk/src/net/java/sip/communicator/service/protocol/ChatRoomMemberRole.java
Url: https://sip-communicator.dev.java.net/source/browse/sip-communicator/trunk/src/net/java/sip/communicator/service/protocol/ChatRoomMemberRole.java?view=diff&rev=5130&p1=trunk/src/net/java/sip/communicator/service/protocol/ChatRoomMemberRole.java&p2=trunk/src/net/java/sip/communicator/service/protocol/ChatRoomMemberRole.java&r1=5129&r2=5130

--- trunk/src/net/java/sip/communicator/service/protocol/ChatRoomMemberRole.java (original)
+++ trunk/src/net/java/sip/communicator/service/protocol/ChatRoomMemberRole.java 2009-03-13 20:38:23+0000
@@ -61,7 +61,7 @@
/**
* the name of this role.
*/
- private String roleName = null;
+ private final String roleName;

/\*\*
 \* The index of a role is used to allow ordering of roles by other modules

@@ -69,7 +69,7 @@
* Higher values of the role index indicate roles with more permissions and
* lower values pertain to more restrictive roles.
*/
- private int roleIndex;
+ private final int roleIndex;

/\*\*
 \* Creates a role with the specified &lt;tt&gt;roleName&lt;/tt&gt;\. The constructor

@@ -136,17 +136,22 @@
*/
public boolean equals(Object obj)
{
- if( obj == null
- || !(obj instanceof ChatRoomMemberRole)
- || !((ChatRoomMemberRole)obj).getRoleName().equals(roleName)
- || ((ChatRoomMemberRole)obj).getRoleIndex() != getRoleIndex())
- {
- return false;
- }
- else
- {
+ if (obj == this)
return true;
- }
+
+ /*
+ * XXX Implementing Object#equals(Object) with instanceof is error
+ * prone. The safe and recommended approach is to return true only if
+ * the runtime types of the two Objects being tested are one and the
+ * same i.e. getClass().equals(obj.getClass()).
+ */
+ if (!(obj instanceof ChatRoomMemberRole))
+ return false;
+
+ ChatRoomMemberRole role = (ChatRoomMemberRole) obj;
+
+ return role.getRoleName().equals(getRoleName())
+ && (role.getRoleIndex() == getRoleIndex());
}

/\*\*

@@ -176,8 +181,7 @@
public int compareTo(Object obj)
throws ClassCastException
{
- return new Integer(getRoleIndex())
- .compareTo(new Integer(((ChatRoomMemberRole)obj).getRoleIndex()));
+ return getRoleIndex() - ((ChatRoomMemberRole) obj).getRoleIndex();
}

}

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: commits-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net