Opened 9 years ago

Closed 9 years ago

#301 closed enhancement (fixed)

Review result for auth

Reported by: zhanglikun Owned by: jinmei
Priority: medium Milestone:
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 3.0 Internal?: no

Description

The following comments was inputted by stephen for ticket #289, so seperated it to one single ticket, maybe jinmei is the best person for accepting this ticket.

src/bin/auth/auth_srv.cc
With very similar names (AuthSrv? and AuthSrvImpl?), it might be clearer to put the declaration and implementation of AuthSrvImpl? into separate files.

The AuthSrv? setXxxx and getXxxx functions are sufficiently trivial that consideration could be given to defining them inline in the header file.

Check with Jerry Chen about recent (9 August 2010) changes to this file (in processAxfrQuery) concerning problems with AXFRs failing because of connection problems.

src/bin/auth/auth_srv.h
A header describing the purpose of the class would be useful.

Remark: all methods (not just ones changed by this update) should have a doxygen header.

setXfrinSession: looks as if the last line of the header comment is missing.

src/bin/auth/asio_link.cc
The asio_link.h file contains the class definitions for the wrapper around ASIO. However, the file asio_link.cc contains additional classes (such as TCPClient) that know about the application. I suggest that asio_link.cc contain only those classes and functions relating to the wrapper interface and that the other classes be moved to a separate file. This will make it easier for asio_link to be used elsewhere.

Header documentation for the helper classes (e.g. what is their purpose and the purpose of their methods) should be provided.

src/bin/auth/asio_link.h
OK. Documentation is good.

Subtickets

Change History (7)

comment:1 in reply to: ↑ description Changed 9 years ago by jinmei

  • Owner changed from jinmei to stephen
  • Status changed from new to assigned

Replying to zhanglikun:

A quick response on one specific point:

src/bin/auth/asio_link.cc

[snip]

Header documentation for the helper classes (e.g. what is their purpose and the purpose of their methods) should be provided.

What are the helper classes?

comment:2 Changed 9 years ago by stephen

  • Owner changed from stephen to jinmei

The terminology is a bit loose (sorry), but I mean classes defined in asio_link.cc such as TCPClient, TCPServer, UDPServer, IOServiceImpl etc.

The classes declared in asio_link.h are extremely well-documented; it would be good to have the classes declared in asio_link.cc documented to that standard as well.

comment:3 in reply to: ↑ description Changed 9 years ago by jinmei

Replying to zhanglikun:

The following comments was inputted by stephen for ticket #289, so seperated it to one single ticket, maybe jinmei is the best person for accepting this ticket.

Actually, for auth_srv I suspect Evan is the best person as the original author of the class, but I took this ticket as a whole.

I've created a branch "trac301", and I believe I addressed most of the comments. The changeset is r2969. I'm giving the ticket back to Stephen.

src/bin/auth/auth_srv.cc
With very similar names (AuthSrv? and AuthSrvImpl?), it might be clearer to put the declaration and implementation of AuthSrvImpl? into separate files.

I'm not sure if I understand this, but since AuthSrvImpl? is the implementation class for AuthSrv? using the pimpl idiom, we cannot separate AuthSrvImpl? into a different .cc file. I noted it's for pimpl in auth_src.h.

The AuthSrv? setXxxx and getXxxx functions are sufficiently trivial that consideration could be given to defining them inline in the header file.

It could, but for impl we cannot. In any case these are not expected to be performance sensitive operations, so I think it's okay.

Check with Jerry Chen about recent (9 August 2010) changes to this file (in processAxfrQuery) concerning problems with AXFRs failing because of connection problems.

I don't understand what this comment means. Please clarify.

src/bin/auth/auth_srv.h
A header describing the purpose of the class would be useful.

Done.

Remark: all methods (not just ones changed by this update) should have a doxygen header.

Done.

setXfrinSession: looks as if the last line of the header comment is missing.

Fixed.

src/bin/auth/asio_link.cc
The asio_link.h file contains the class definitions for the wrapper around ASIO. However, the file asio_link.cc contains additional classes (such as TCPClient) that know about the application. I suggest that asio_link.cc contain only those classes and functions relating to the wrapper interface and that the other classes be moved to a separate file. This will make it easier for asio_link to be used elsewhere.

Yes, I've noted that. We'll eventually refactor the code so that the wrapper implementation is independent from specific applications. Evan seems to do something along with this line in #327. In any case I think it's beyond the scope of this ticket.

Header documentation for the helper classes (e.g. what is their purpose and the purpose of their methods) should be provided.

Done.

I've not touched TCPServer and UDPServer this time if you intended to include these because there seems to be an attempt of heavily refactoring these classes (#327). That refactoring ticket would be a better place to improve the description.

One more point: I've renamed AuthSrv::configSession() to AuthSrv::getConfigSession() based on our coding guideline.

comment:4 Changed 9 years ago by jinmei

  • Add Hours to Ticket changed from 0.0 to 3.0
  • Total Hours changed from 0.0 to 3.0

comment:5 Changed 9 years ago by jinmei

  • Owner changed from jinmei to stephen
  • Status changed from assigned to reviewing

comment:6 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to jinmei

src/bin/auth/auth_srv.cc
With very similar names (AuthSrv and AuthSrvImpl), it might be clearer to put the
declaration and implementation of AuthSrvImpl into separate files.

I'm not sure if I understand this, but since AuthSrvImpl is the implementation class
for AuthSrv! using the pimpl idiom, we cannot separate AuthSrvImpl into a different
.cc file. I noted it's for pimpl in auth_src.h.

I was only suggesting that the contents of the auth_srv.cc file be split into multiple files - e.g. put the definition of AuthSrvImpl in auth_srv_impl.h and the methods in auth_srv_impl.cc and do appropriate #includes. The pimpl idiom - which in this case is that auth_srv.h is impervious to changes in the functionality of AuthSrv(Impl) - is preserved.

However, it's only a style suggestion and there is no need to make any changes.

The AuthSrv setXxxx and getXxxx functions are sufficiently trivial that consideration
could be given to defining them inline in the header file.

It could, but for impl we cannot. In any case these are not expected to be performance
sensitive operations, so I think it's okay.

Fair point.

Check with Jerry Chen about recent (9 August 2010) changes to this file (in
processAxfrQuery) concerning problems with AXFRs failing because of connection
problems.

I don't understand what this comment means. Please clarify.

I saw something authored by Jerry Chen that mentioned these problems, but I can't now find it in either my email records or the Jabber logs. My fault, I should have copied the information here. Ignore it - if it is still a problem, it will doubtless appear during testing.

All other changes are OK.

Please go ahead and merge.

comment:7 in reply to: ↑ 6 Changed 9 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed

Replying to stephen:

src/bin/auth/auth_srv.cc
With very similar names (AuthSrv and AuthSrvImpl), it might be clearer to put the
declaration and implementation of AuthSrvImpl into separate files.

I'm not sure if I understand this, but since AuthSrvImpl is the implementation class
for AuthSrv! using the pimpl idiom, we cannot separate AuthSrvImpl into a different
.cc file. I noted it's for pimpl in auth_src.h.

I was only suggesting that the contents of the auth_srv.cc file be split into multiple files - e.g. put the definition of AuthSrvImpl in auth_srv_impl.h and the methods in auth_srv_impl.cc and do appropriate #includes. The pimpl idiom - which in this case is that auth_srv.h is impervious to changes in the functionality of AuthSrv(Impl) - is preserved.

Ah, okay. Hmm, we may need this separation if a module (with full implementation) gets very large (although in that case we may have to reconsider the class design), but in this case I think it's a matter of preference. And,

However, it's only a style suggestion and there is no need to make any changes.

you seem to agree with this, so for now I'll keep the unified style.

[snip other points]

All other changes are OK.

Please go ahead and merge.

Okay, done. I won't bother to consume a ChangeLog? entry for this ticket.

Thanks for the review.

Note: See TracTickets for help on using tickets.