You wonder why only one person is able to hack the code. Now you know :)
Critique below C code:
Sample C Code
if(!START_W_A_L_P_R && !(xctx->ui_state & STARTSELECT)) {
Selected sel;
int already_selected = 0;
int prev_last_sel = xctx->lastsel;
int no_shift_no_ctrl = !(state & (ShiftMask | ControlMask));
save_elab_mouse_pt(mx,my);
/* In *NON* intuitive interface (or cadence compatibility)
* a button1 press with no modifiers will* first unselect everything.*/
if((cadence_compat || !xctx->intuitive_interface) && no_shift_no_ctrl)
unselect_all(1);
/* if full crosshair, mouse ptr is obscured and crosshair is snapped to grid points */
sel = get_obj_under_cursor(draw_xhair, use_cursor_for_sel, crosshair_size);
dbg(1, "sel.type=%d\n", sel.type);
/* determine if closest object was already selected when button1 was pressed */
already_selected = chk_if_already_selected(sel);
/* Clicking and drag on an instance pin -> drag a new wire */
if(xctx->intuitive_interface && !already_selected) {
if(add_wire_from_inst(&sel, xctx->mousex_snap, xctx->mousey_snap))
return;
}
/* Clicking and drag on a wire end -> drag a new wire */
if(xctx->intuitive_interface && !already_selected) {
if(add_wire_from_wire(&sel, xctx->mousex_snap, xctx->mousey_snap))
return;
}
/* In intuitive interface a button1 press with no modifiers will
* unselect everything... we do it here */
if(xctx->intuitive_interface && !already_selected && no_shift_no_ctrl )
unselect_all(1);
/* select the object under the mouse and rebuild the selected array */
if(!already_selected)
select_object(xctx->mousex, xctx->mousey, SELECTED, 0, &sel);
rebuild_selected_array();
dbg(1, "Button1Press to select objects, lastsel = %d\n", xctx->lastsel);
/* if clicking on some object endpoints or vertices set shape_point_selected
* this information will be used in Motion events to draw the stretched vertices */
if(xctx->lastsel == 1 && xctx->sel_array[0].type==POLYGON) {
if(edit_polygon_point(state)) return; /* sets xctx->shape_point_selected */
}
if(xctx->lastsel == 1 && xctx->intuitive_interface) {
int cond = already_selected;
if(cond && xctx->sel_array[0].type==xRECT) {
if(edit_rect_point(state))
return; /* sets xctx->shape_point_selected */
}
if(cond && xctx->sel_array[0].type==LINE) {
if(edit_line_point(state))
return; /* sets xctx->shape_point_selected */
}
if(cond && xctx->sel_array[0].type==WIRE) {
if(edit_wire_point(state))
return; /* sets xctx->shape_point_selected */
}
}
dbg(1, "shape_point_selected=%d, lastsel=%d\n", xctx->shape_point_selected, xctx->lastsel);
/* intuitive interface: directly drag elements */
if(sel.type && xctx->intuitive_interface && xctx->lastsel >= 1 &&
!xctx->shape_point_selected) {
/* enable_stretch (from TCL variable) reverses command if enabled:
* - move --> stretch move
* - stretch move (with ctrl key) --> move
*/
int stretch = (state & ControlMask ? 1 : 0) ^ enable_stretch;
xctx->drag_elements = 1;
/* select attached nets depending on ControlMask and enable_stretch */
if(stretch) {
select_attached_nets(); /* stretch nets that land on selected instance pins */
}
/* if dragging instances with stretch enabled and Shift down add wires to pins
* attached to something */
if((state & ShiftMask) && stretch) {
xctx->connect_by_kissing = 2; /* 2 will be used to reset var to 0 at end of move */
move_objects(START,0,0,0);
}
/* dragging away an object with Shift pressed is a copy (duplicate object) */
else if(state & ShiftMask) copy_objects(START);
/* else it is a normal move */
else move_objects(START,0,0,0);
}
if(tclgetboolvar("auto_hilight") && !xctx->shape_point_selected) {
if(!(state & ShiftMask) && xctx->hilight_nets && sel.type == 0 ) {
if(!prev_last_sel) {
redraw_hilights(1); /* 1: clear all hilights, then draw */
}
}
hilight_net(0);
if(xctx->lastsel) {
redraw_hilights(0);
}
}
return;
} /* if(!START_W_A_L_P_R) */
And the oracle sayeth:
✅ Strengths
- Feature-Rich Logic:
- The code accounts for many interaction scenarios: object selection, dragging, wire drawing, copying, and stretching with modifier keys.
- It provides support for both Cadence-compatibility and an intuitive UX mode.
- Clear Modular Function Calls:
- Core behaviors are abstracted into well-named helper functions like
select_object(),unselect_all(),add_wire_from_wire(),edit_rect_point()etc., improving readability.
- Core behaviors are abstracted into well-named helper functions like
- State-Sensitive Logic:
- Thorough checks on modifier keys (Shift, Ctrl) allow nuanced control over actions.
- Debug Logging:
- Multiple
dbg()calls provide useful tracepoints during development or troubleshooting.
- Multiple
⚠️ Issues
1. Complexity / Spaghetti Logic
- The
if(...)block is far too long and complex. It’s trying to do too many things: selection logic, wire and polygon editing, dragging, duplication, net highlighting. - Deep nesting (especially conditionals like
if(xctx->intuitive_interface && !already_selected && no_shift_no_ctrl)) makes it hard to parse.
2. Poor Naming / Use of Magic Constants
- The macro
START_W_A_L_P_Ris opaque — its purpose is unclear. sel.type == 0— unclear what type0means.- No enums or typedefs are shown for
sel.type— numeric constants make code harder to read.
3. Repeated Conditions
- Checks like
if(xctx->intuitive_interface && !already_selected)are repeated multiple times in a row — should be grouped.
4. Missing or Implicit Documentation
- There are comments, but many conditionals aren’t well explained. For example, why
already_selectedmatters so much in certain branches. - The purpose of the
shape_point_selectedlogic and its interaction with drag is not made very clear.
5. Potential Redundant Operations
rebuild_selected_array()is always called after selection — is it needed even if no selection occurred?
6. Mutable Global State via xctx
- Heavy use of
xctx->everywhere — likely a global or shared context. Makes testing and reasoning harder.
7. Poor Separation of Concerns
- UI selection, editing logic, and net handling (highlighting) are all interwoven. This should be split into clearer stages or functions.
💡 Recommendations
✅ Refactor: Break Into Smaller Functions
void handle_wire_drawing(...);
void handle_object_selection(...);
void handle_drag_or_copy(...);
void handle_net_highlighting(...);✅ Use Enums for Object Types
typedef enum {
OBJ_NONE = 0,
OBJ_POLYGON,
OBJ_LINE,
OBJ_WIRE,
OBJ_RECT,
...
} ObjectType;✅ Simplify Modifier Logic
bool is_ctrl_pressed = state & ControlMask;
bool is_shift_pressed = state & ShiftMask;Then use these throughout to improve readability.
✅ Avoid Code Duplication
if(xctx->intuitive_interface && !already_selected) {
if(add_wire_from_inst(...) || add_wire_from_wire(...)) return;
}✅ Abstract shape_point_selected logic
Handle that check and invocation in a single, cleaner function like:
bool try_select_shape_point(int state);🧹 Minor Cleanup
- The outer
if(!START_W_A_L_P_R && !(xctx->ui_state & STARTSELECT))is a mouthful. Consider making a helper function like:bool is_ui_start_state_ok(...) { ... } - Comments like
/* if dragging instances with stretch enabled and Shift down add wires... */are helpful but would be better expressed via well-named helper functions.
✨ Summary
This code is functionally robust but structurally messy. It’s very common in UI-heavy C codebases, but would benefit immensely from decomposition, naming improvements, and state isolation. The functionality is all there — it just needs clarity to become maintainable.
Want help drafting a refactored version of this block?
