Some code style changes.

Some code style changes.

I'd like to establish a consistent and fairly conventional code style,
if it's OK.  Else please let me know which code style is prefered so I
can set up my IDE to enforce it.

Here's a summary of changes to the best of my recollection:

- No need to have multi-line comments where a single line suffices.

- Opening brace goes on the same line as the declaration start.  This
  is Java style, as opposed to C or C++ where it's customary to have
  the brace on the next line.  This also saves some screen
  real-estate, which is good.

- timeout variables should encode the units they're expressed in,
  since reading: timeout = 3000 doesn't say much about what that 3000
  is, while timeoutMs = 3000 does a lot with just an extra two
  letters.

- 'else' clause can be compacted with the surrounding braces.

- space after if, for, while etc, so 'if (cond) { ...' rather than
  if(cond).

- don't use short-form ifs, even if the consequent/alternative are one
  line long.  Doing so may introduce subtle bugs in nested conditional
  code.

- Removed unused imports.

- No lines beyond 100 columns.  That's about the place where it stops
  being comfortable for reading on a modern hires monitor.

- No ascii-art in parameter formatting.  When you need to break lines
  in parameter lists, just use normal indent.  Doing otherwise will
  cause formatting to break if any of the params or methods get
  refactored and renamed.

- The byte[] vs byte... comment is bogus.  byte... is equivalent to
  byte[].  There is ambiguity for uncast null *constant* because
  compiler doesn't know if null is the null array or null as the first
  array parameter.  But since production code never passes in pure
  null constant, this is solved by inserting appropriate conversion in
  the test code.  Varargs are quite OK, but arrays of primitves as
  parameters aren't stellar: Iterable should be used instead, but let
  that come later.

[grakic, 2011-07-17 11:14:08]

On 2011/07/16 18:47:09, fmil wrote:

Pushed. https://gitorious.org/freesteel/jfreesteel/commits/master

[grakic, 2011-07-17 12:33:17]

On 2011/07/16 18:47:09, fmil wrote:

I pasted your code styles notes to the wiki:
https://gitorious.org/freesteel/pages/CodeStyle

Please take a look at one of the next commits:
https://gitorious.org/freesteel/jfreesteel/commit/1dc09dfba299534d9474ebdbc7d...

Moving tag reading from EidInfo to EidCard allow us to support new tags with
future cards. I added few explicit casts as map.get returns null when key tag is
not present. I do not think we need more POJOs to represent individual TLV
structures, as they are now internal to EidCard.

This should make testing EidInfo easier.

[fmil, 2011-07-17 20:24:08]

On Sun, Jul 17, 2011 at 5:33 AM, <grakic@gmail.com> wrote:

> On 2011/07/16 18:47:09, fmil wrote:
>
> I pasted your code styles notes to the wiki:
>
https://gitorious.org/**freesteel/pages/CodeStyle<https://gitorious.org/frees...


Thanks.


>
>
> Please take a look at one of the next commits:
> https://gitorious.org/**freesteel/jfreesteel/commit/**
>
1dc09dfba299534d9474ebdbc7d2da**03ca6195cb<https://gitorious.org/freesteel/jfreesteel/commit/1dc09dfba299534d9474ebdbc7d2da03ca6195cb>
>
> Moving tag reading from EidInfo to EidCard allow us to support new tags
> with future cards.


+1


> I added few explicit casts as map.get returns null
> when key tag is not present.


Even been better if you factored the idiom:

<EidInfo>.setIssuingDate(Utils.bytes2UTF8String((byte[])
<some_map>.get(<some_id>)))

out into a separate method.  This would make the decoding way easier.

It would in fact be way easier still, if the EidInfo class weren't
unnecessarily stongly typed.  I think that you could, in principle, just
have a Map<String, String> internally, which would preclude the need to have
all the code that copies stuff over.

Another improvement may be splitting this class into three parts -- since it
already tries to do three separate things.  But it's not essential and can
be done later.

I am also fairly convinced that byte[] are evil and should be replaced with
something else overall, but that, too, can come later.

I do not think we need more POJOs to
> represent individual TLV structures, as they are now internal to
> EidCard.
>
> This should make testing EidInfo easier.


Yes, this is already a step forward.

f

[grakic, 2011-07-19 12:28:34]

On 2011/07/17 20:24:08, fmil wrote:
> Another improvement may be splitting this class into three parts -- since it
> already tries to do three separate things.  But it's not essential and can
> be done later.

I do not see use case where you would want document info and not personal info.

The fact there are 3 different EF blobs stored on the card internally can be
easily changed in the future with new series of cards. All 3 files are read much
faster than you can remove and insert new card.

There is a missing feature in EidCard that should be able to validate EidInfo
against the signature stored somewhere on a card. Official Windows application
is not doing this at the moment. I do not know if there is a single signature
for all data (with or without photo), or each EF is signed individually.

[fmil, 2011-07-19 17:14:45]

On Tue, Jul 19, 2011 at 5:28 AM, <grakic@gmail.com> wrote:

> On 2011/07/17 20:24:08, fmil wrote:
>
>> Another improvement may be splitting this class into three parts --
>>
> since it
>
>> already tries to do three separate things.  But it's not essential and
>>
> can
>
>> be done later.
>>
>
> I do not see use case where you would want document info and not
> personal info.
>
> The fact there are 3 different EF blobs stored on the card internally
> can be easily changed in the future with new series of cards. All 3
> files are read much faster than you can remove and insert new card.
>

Ok, we'll worry about that when the time comes.