Code Smells - All This for Handling a Left Mouse Button Press

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.
• 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.

⚠️ 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_R is opaque — its purpose is unclear.
• sel.type == 0 — unclear what type 0 means.
• 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_selected matters so much in certain branches.
• The purpose of the shape_point_selected logic 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?