Java Code Review

// ❌ NPE riskString name = user.getName().toUpperCase();
// ✅ SafeString name = Optional.ofNullable(user.getName())    .map(String::toUpperCase)    .orElse("");
// ✅ Also safe (early return)if (user.getName() == null) {    return "";}return user.getName().toUpperCase();

// ❌ Swallowing exceptionstry {    process();} catch (Exception e) {    // silently ignored}
// ❌ Catching too broadcatch (Exception e) { }catch (Throwable t) { }
// ❌ Losing stack tracecatch (IOException e) {    throw new RuntimeException(e.getMessage());}
// ✅ Proper handlingcatch (IOException e) {    log.error("Failed to process file: {}", filename, e);    throw new ProcessingException("File processing failed", e);}

// ❌ Modifying while iteratingfor (Item item : items) {    if (item.isExpired()) {        items.remove(item);  // ConcurrentModificationException    }}
// ✅ Use removeIfitems.removeIf(Item::isExpired);
// ❌ Stream for simple operationslist.stream().forEach(System.out::println);
// ✅ Simple loop is cleanerfor (Item item : list) {    System.out.println(item);}
// ❌ Collecting to modifyList<String> names = users.stream()    .map(User::getName)    .collect(Collectors.toList());names.add("extra");  // Might be immutable!
// ✅ Explicit mutable listList<String> names = users.stream()    .map(User::getName)    .collect(Collectors.toCollection(ArrayList::new));

// ❌ Not thread-safeprivate Map<String, User> cache = new HashMap<>();
// ✅ Thread-safeprivate Map<String, User> cache = new ConcurrentHashMap<>();
// ❌ Check-then-act race conditionif (!map.containsKey(key)) {    map.put(key, computeValue());}
// ✅ Atomic operationmap.computeIfAbsent(key, k -> computeValue());
// ❌ Double-checked locking (broken without volatile)if (instance == null) {    synchronized(this) {        if (instance == null) {            instance = new Instance();        }    }}

// ❌ Only equals without hashCode@Overridepublic boolean equals(Object o) { ... }// Missing hashCode!
// ❌ Mutable fields in hashCode@Overridepublic int hashCode() {    return Objects.hash(id, mutableField);  // Breaks HashMap}
// ✅ Use immutable fields, implement both@Overridepublic boolean equals(Object o) {    if (this == o) return true;    if (!(o instanceof User user)) return false;    return Objects.equals(id, user.id);}
@Overridepublic int hashCode() {    return Objects.hash(id);}

// ❌ Missing - hard to debug// No toString()
// ❌ Including sensitive datareturn "User{password='" + password + "'}";
// ✅ Useful for debugging@Overridepublic String toString() {    return "User{id=" + id + ", name='" + name + "'}";}

// ✅ For classes with many optional parametersUser user = User.builder()    .name("John")    .email("[email protected]")    .build();

// ❌ Resource leakFileInputStream fis = new FileInputStream(file);// ... might throw before close
// ✅ Try-with-resourcestry (FileInputStream fis = new FileInputStream(file)) {    // ...}
// ❌ Multiple resources, wrong ordertry (BufferedWriter writer = new BufferedWriter(new FileWriter(file))) {    // FileWriter might not be closed if BufferedWriter fails}
// ✅ Separate declarationstry (FileWriter fw = new FileWriter(file);     BufferedWriter writer = new BufferedWriter(fw)) {    // Both properly closed}

// ❌ Boolean parametersprocess(data, true, false);  // What do these mean?
// ✅ Use enums or builderprocess(data, ProcessMode.ASYNC, ErrorHandling.STRICT);
// ❌ Returning null for "not found"public User findById(Long id) {    return users.get(id);  // null if not found}
// ✅ Return Optionalpublic Optional<User> findById(Long id) {    return Optional.ofNullable(users.get(id));}
// ❌ Accepting null collectionspublic void process(List<Item> items) {    if (items == null) items = Collections.emptyList();}
// ✅ Require non-null, accept emptypublic void process(List<Item> items) {    Objects.requireNonNull(items, "items must not be null");}

// ❌ String concatenation in loopString result = "";for (String s : strings) {    result += s;  // Creates new String each iteration}
// ✅ StringBuilderStringBuilder sb = new StringBuilder();for (String s : strings) {    sb.append(s);}
// ❌ Regex compilation in loopfor (String line : lines) {    if (line.matches("pattern.*")) { }  // Compiles regex each time}
// ✅ Pre-compiled patternprivate static final Pattern PATTERN = Pattern.compile("pattern.*");for (String line : lines) {    if (PATTERN.matcher(line).matches()) { }}
// ❌ N+1 in loopsfor (User user : users) {    List<Order> orders = orderRepo.findByUserId(user.getId());}
// ✅ Batch fetchMap<Long, List<Order>> ordersByUser = orderRepo.findByUserIds(userIds);