Function for retrieving fault planes - #18
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the get_fault_planes method to the NSHMDB class for retrieving and converting fault plane coordinates, alongside minor docstring and formatting fixes. Feedback was provided regarding the use of SELECT * and positional unpacking, which is fragile; the reviewer suggested explicitly selecting columns and iterating over the cursor directly for improved robustness and memory efficiency.
| cursor.execute( | ||
| "SELECT * from fault_plane where fault_id = ?", (fault_id,) | ||
| ) | ||
| planes = [] | ||
| for ( | ||
| _, | ||
| top_left_lat, | ||
| top_left_lon, | ||
| top_right_lat, | ||
| top_right_lon, | ||
| bottom_right_lat, | ||
| bottom_right_lon, | ||
| bottom_left_lat, | ||
| bottom_left_lon, | ||
| top, | ||
| bottom, | ||
| _, | ||
| ) in cursor.fetchall(): |
There was a problem hiding this comment.
Using SELECT * with positional unpacking is fragile and can lead to runtime errors if the database schema is modified (e.g., if columns are added or reordered). It is better to explicitly select the required columns and unpack them directly. Additionally, iterating over the cursor directly is more memory-efficient than using fetchall().
cursor.execute(
"SELECT top_left_lat, top_left_lon, top_right_lat, top_right_lon, "
"bottom_right_lat, bottom_right_lon, bottom_left_lat, bottom_left_lon, "
"top_depth, bottom_depth FROM fault_plane WHERE fault_id = ?",
(fault_id,),
)
planes = []
for (
top_left_lat,
top_left_lon,
top_right_lat,
top_right_lon,
bottom_right_lat,
bottom_right_lon,
bottom_left_lat,
bottom_left_lon,
top,
bottom,
) in cursor:There was a problem hiding this comment.
Doing this could be good, seems simple enough change
joelridden
left a comment
There was a problem hiding this comment.
Need to add a simple test case for the test coverage
No description provided.