Opened 9 years ago

Closed 9 years ago

#466 closed defect (fixed)

duplicate code in xfrout

Reported by: jinmei Owned by: zzchen_pku
Priority: low Milestone: y2 12 month milestone
Component: xfrout 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: 0 Internal?: no

Description

XfroutSession?._zone_is_empty() and XfroutSession?._zone_exist() are essentially duplicate. They do the same thing, and just return a negated value.

We need some clarification or refactoring here.

Subtickets

Change History (13)

comment:1 Changed 9 years ago by zzchen_pku

  • Owner set to zzchen_pku
  • Status changed from new to assigned

comment:2 follow-up: Changed 9 years ago by zzchen_pku

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

Updated in origin/trac466, since this ticket is created by you, please take a look at it, thanks.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 9 years ago by jinmei

Replying to zzchen_pku:

Updated in origin/trac466, since this ticket is created by you, please take a look at it, thanks.

First off, I made a minor style fix to the branch.

xfrout.py.in

  • I think _zone_is_empty() is not intuitive in the context of how it is used and what it actually does. What it actually does is to test if a specified zone has an SOA RR. That is, it could return True even if a zone isn't "empty" (in that it has some RRs). I'd rename it to zone_has_soa(), and use it with "not".
  • I'd like to see more description for _zone_is_empty() and _zone_exist(). Specifically, I'd like to see an explanation how we use these two sets of functions.

sqlite3_ds.py

  • zone_exist doesn't have a test. This probably also means it wasn't developed in test driven, and if really not, it's not good (TDD is especially important when we introduce a new thing, because it would affect how we design it). If my guess is correct, I'd suggest delete zone_exist once, and restart it from the scratch in TDD (of course it's a waste in some sense, but we need some redundant practice to change our mindset)
  • I'd like zone_exist (or whatever new result) to have more complete pydoc style documentation. It would include description of param/return, and provide more detailed explanation (e.g. what "zone exists" means), explain in which case an exception can be raised and which exception.

comment:4 Changed 9 years ago by jinmei

  • Owner changed from jinmei to zzchen_pku

comment:5 in reply to: ↑ 3 ; follow-up: Changed 9 years ago by zzchen_pku

Replying to jinmei:

First off, I made a minor style fix to the branch.

Thanks.

xfrout.py.in

  • I think _zone_is_empty() is not intuitive in the context of how it is used and what it actually does. What it actually does is to test if a specified zone has an SOA RR. That is, it could return True even if a zone isn't "empty" (in that it has some RRs). I'd rename it to zone_has_soa(), and use it with "not".
  • I'd like to see more description for _zone_is_empty() and _zone_exist(). Specifically, I'd like to see an explanation how we use these two sets of functions.

Done.

sqlite3_ds.py

  • zone_exist doesn't have a test. This probably also means it wasn't developed in test driven, and if really not, it's not good (TDD is especially important when we introduce a new thing, because it would affect how we design it). If my guess is correct, I'd suggest delete zone_exist once, and restart it from the scratch in TDD (of course it's a waste in some sense, but we need some redundant practice to change our mindset)
  • I'd like zone_exist (or whatever new result) to have more complete pydoc style documentation. It would include description of param/return, and provide more detailed explanation (e.g. what "zone exists" means), explain in which case an exception can be raised and which exception.

I noticed that sqlite3_ds doesn't have any unittests before, I think it is because:

Test-driven development is difficult to use in situations where full functional tests are 
required to determine success or failure. Examples of these are user interfaces, programs 
that work with databases, and some that depend on specific network configurations.

It's difficult to test database behaviors. I have updated zone_exist according to TDD, and found that we need to use too much fakes and mocks, so I wonder whether TDD applies to this specific situation?

comment:6 Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to jinmei

comment:7 in reply to: ↑ 5 ; follow-up: Changed 9 years ago by jinmei

Replying to zzchen_pku:

xfrout.py.in

  • I think _zone_is_empty() is not intuitive in the context of how it is used and what it actually does. What it actually does is to test if a specified zone has an SOA RR. That is, it could return True even if a zone isn't "empty" (in that it has some RRs). I'd rename it to zone_has_soa(), and use it with "not".
  • I'd like to see more description for _zone_is_empty() and _zone_exist(). Specifically, I'd like to see an explanation how we use these two sets of functions.

Done.

I've made some additional suggested updates. Those include:

  • replaced "NS" with "name server" because "NS" looks like an RR type and can be confusing.
  • s/soa/SOA

sqlite3_ds.py

  • I'd like zone_exist (or whatever new result) to have more complete pydoc style documentation. It would include description of param/return, and provide more detailed explanation (e.g. what "zone exists" means), explain in which case an exception can be raised and which exception.

I noticed that sqlite3_ds doesn't have any unittests before, I think it is because:

Test-driven development is difficult to use in situations where full functional tests are 
required to determine success or failure. Examples of these are user interfaces, programs 
that work with databases, and some that depend on specific network configurations.

It's difficult to test database behaviors. I have updated zone_exist according to TDD, and found that we need to use too much fakes and mocks, so I wonder whether TDD applies to this specific situation?

I see some valid points here. In particular, it's true that writing unittests for DB related stuff is difficult (in fact, in a commoly adoped definition it wouldn't be considered "unittest"). However:

  • it still cannot be a reason for skipping tests, even if it's not test "driven".
  • in case of sqlite3 it should still be relatively easier to provide tests because we only need library calls (and pre-populated test DB file), i.e, we don't need a separate process or network-based transactions.
  • in fact, the C++ version of sqlite3 data source has detailed "unit" tests.
  • so I suspect the main reason why the python version of sqlite3 data source didn't have tests was because we made compromise due to the hard deadline for the year1 deliverable (or we were simply lazy:-)
  • whether this could/should be test "driven" may be debatable, but for the above reasons in this specific case I believe it can, and if it can I believe it shoud.
  • at the bottom line I don't see a valid excuse to skip tests for the sqlite3 data source python module.
  • and, in fact, it looks like you found and fixed a bug thanks to writing the test:-)

As for documentation, I'd like to use the pydoc style comments. Eventually that would apply to other functions of this file. I wouldn't request that within the scope of this ticket, but let's at least use it for newer code.

sqlite3_ds_test.py

  • I suspect the copyright year should be 2011:-)
  • in this case I suspect we cannot use mocks because this module is our interface to sqlite3. For tests of a user app of this module such as xfrout we can use a mock data source, but tests for sqlite3 specific module should use sqlite3.
  • exception case(s) should also be tested.

comment:8 Changed 9 years ago by jinmei

  • Owner changed from jinmei to zzchen_pku

comment:9 in reply to: ↑ 7 ; follow-up: Changed 9 years ago by zzchen_pku

Replying to jinmei:

I've made some additional suggested updates. Those include:

  • replaced "NS" with "name server" because "NS" looks like an RR type and can be confusing.
  • s/soa/SOA

Okay, thanks.

sqlite3_ds.py
I see some valid points here. In particular, it's true that writing unittests for DB related stuff is difficult (in fact, in a commoly adoped definition it wouldn't be considered "unittest"). However:

  • it still cannot be a reason for skipping tests, even if it's not test "driven".
  • in case of sqlite3 it should still be relatively easier to provide tests because we only need library calls (and pre-populated test DB file), i.e, we don't need a separate process or network-based transactions.
  • in fact, the C++ version of sqlite3 data source has detailed "unit" tests.
  • so I suspect the main reason why the python version of sqlite3 data source didn't have tests was because we made compromise due to the hard deadline for the year1 deliverable (or we were simply lazy:-)
  • whether this could/should be test "driven" may be debatable, but for the above reasons in this specific case I believe it can, and if it can I believe it shoud.
  • at the bottom line I don't see a valid excuse to skip tests for the sqlite3 data source python module.
  • and, in fact, it looks like you found and fixed a bug thanks to writing the test:-)

Okay.

As for documentation, I'd like to use the pydoc style comments. Eventually that would apply to other functions of this file. I wouldn't request that within the scope of this ticket, but let's at least use it for newer code.

I updated the sqlite3_ds comments by using pydoc style.

sqlite3_ds_test.py

  • I suspect the copyright year should be 2011:-)

Yeah, I just copied it from another file, thanks for checking:-)

  • in this case I suspect we cannot use mocks because this module is our interface to sqlite3. For tests of a user app of this module such as xfrout we can use a mock data source, but tests for sqlite3 specific module should use sqlite3.
  • exception case(s) should also be tested.

Done.

comment:10 Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to jinmei

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

Replying to zzchen_pku:

As for documentation, I'd like to use the pydoc style comments. Eventually that would apply to other functions of this file. I wouldn't request that within the scope of this ticket, but let's at least use it for newer code.

I updated the sqlite3_ds comments by using pydoc style.

Hmm, you seemed to make an overhaul update. So I've added one additional code level refacotring as well as some minor editorial suggested changes.

Other points look okay. So if my suggested change is okay please feel free to merge it.

comment:12 Changed 9 years ago by jinmei

  • Owner changed from jinmei to zzchen_pku

comment:13 Changed 9 years ago by zzchen_pku

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

Merged,closing it.
Thanks for review.

Note: See TracTickets for help on using tickets.