कोड रिव्यू का मुख्य उद्देश्य ऐसे कोड को ढूंढना है जिसे maintain करना कठिन हो

 (mathstodon.xyz)
5 पॉइंट द्वारा GN⁺ 5 시간 전 | 1 टिप्पणियां | WhatsApp पर शेयर करें
  • कोड रिव्यू bug पकड़ने या integrity सुनिश्चित करने की प्रक्रिया से अधिक, बाद में maintain करना कठिन कोड को पहले से उजागर करने की प्रक्रिया के करीब है
  • अगर reviewer कोड पढ़कर भी उसे समझना कठिन पाए, तो भविष्य के maintainer को भी वही समस्या होने की संभावना अधिक है
  • सुधार अभी कर लेना बेहतर है, जब मूल लेखक को संदर्भ याद है; और सिर्फ कोड inspection से bug को भरोसेमंद तरीके से ढूंढ लेने की उम्मीद यथार्थवादी नहीं है
  • reviewer के लिए “bug ढूंढो” से ज्यादा “समझने की कोशिश करो, और जो समझ न आए उसे चिह्नित करो” अधिक व्यावहारिक काम है
  • अच्छा review हर चीज़ को पूरी तरह सिद्ध करने का काम नहीं, बल्कि जहाँ समझ न आए वहाँ नोट छोड़ने और सुधार की मांग करने से शुरू होता है

कोड रिव्यू का फोकस बदलना

  • कोड रिव्यू का मूल उद्देश्य reviewer का bug ढूंढना नहीं है, और न ही यह गारंटी देना कि कोड में bug नहीं हैं
  • सिर्फ कोड को सरसरी तौर पर देखकर सामान्य रूप से bug ढूंढ लेने की उम्मीद व्यावहारिक रूप से कमजोर है
  • इसलिए review का केंद्र “क्या यह सही कोड है” से अधिक “क्या कोई दूसरा व्यक्ति इसे बाद में पढ़ और सुधार सकता है” पर होता है

समझना कठिन कोड maintainability के जोखिम का संकेत

  • reviewer कोड को यह समझने के लिए पढ़ता है कि वह क्या कर रहा है और कैसे काम करता है
  • जो हिस्से समझ में नहीं आते, वे इस बात के risk signal बनते हैं कि भविष्य का maintainer वहाँ अटक सकता है
  • ऐसे कोड को तब ही ठीक कर लेना बेहतर है जब मूल लेखक को अभी भी संदर्भ याद हो

bug ढूंढने से अधिक व्यावहारिक review कार्य

  • “इस कोड में bug ढूंढो” जैसा अनुरोध ऐसा काम है जिसमें सफलता का आकलन करना कठिन है
  • कुछ bug मिल जाने पर भी और bug छूटे हो सकते हैं, इसलिए reviewer के लिए अक्सर सिर्फ असफलता ही स्पष्ट रह जाती है
  • इसके उलट, “इस कोड को समझने की कोशिश करो, और अगर समझ न आए तो बताओ” जैसा अनुरोध अधिक स्पष्ट मानदंड देता है
    • हर चीज़ को पूरी तरह समझना ज़रूरी नहीं है
    • जो भाग समझ में न आए, उन्हें दर्ज कर देना पर्याप्त है
    • पूरे को समझने की कोशिश करना और जहाँ रुकावट आए वहाँ नोट छोड़ देना ही review की भूमिका निभाना है

व्यावहारिक कामकाज में review के मानदंड

  • reviewer जिस कोड को समझ नहीं पाता, वह अपने आप में सुधार का विषय बन सकता है
  • review comment केवल bug report के लिए नहीं, बल्कि अपर्याप्त व्याख्या, संरचनात्मक समस्या, और पढ़ने में कठिन flow को उजागर करने के लिए भी होते हैं
  • इस मानदंड में कोड की वैधता सिद्ध करने से अधिक महत्वपूर्ण यह है कि बाद में टीम का कोई सदस्य उसे पढ़ और संभाल सके

bug मिलना एक सह-प्रभाव है

  • इसका अर्थ यह नहीं कि कोड रिव्यू bug बिल्कुल नहीं ढूंढ सकता
  • review के दौरान bug मिल सकते हैं, लेकिन इसे सभी bug या अधिकांश bug ढूंढने की विधि मानना कठिन है
  • अधिक यथार्थवादी सफलता की शर्त यह है कि समझने की क्षमता की जांच की जाए, और maintain करना कठिन हिस्सों को मूल लेखक के साथ मिलकर तुरंत सुधारा जाए

संबंधित पढ़ाई

1 टिप्पणियां

 
GN⁺ 5 시간 전
Hacker News की राय

  • कोड रिव्यू का कोई एकमात्र उद्देश्य नहीं होता। मेंटेन करना मुश्किल कोड ढूँढना महत्वपूर्ण है, लेकिन यह अकेला उद्देश्य नहीं है, और शायद सबसे महत्वपूर्ण भी न हो
    डेवलपर या AI अजीब दिशा में चला जाए तो यह malicious code को merge करना मुश्किल बनाने वाला safety guard बनता है, और समस्या से बहुत ज़्यादा करीब न होने वाला व्यक्ति बेहतर तरीका या छूटी हुई समस्या देख सकता है
    सिस्टम के दूसरे हिस्सों को बेहतर जानने वाला व्यक्ति interaction issues पकड़ सकता है, कम से कम एक और व्यक्ति उस कोड से परिचित हो जाता है, और लेखक व reviewer दोनों के लिए learning opportunity बनती है
    खासकर जब अनुभव में अंतर हो तो यह अहम है: नए कर्मचारियों को mentor करते समय मैं उन्हें अपने सभी PR में reviewer के रूप में जोड़ता हूँ ताकि वे मेरा काम करने का तरीका देखें, और उनके PR भी review करके दिशा देता हूँ। कभी-कभी मैं भी उनसे सीखता हूँ
    Bugs पकड़ना भी सही है, लेकिन यह मुख्य mechanism नहीं होना चाहिए; automated tests से पकड़ना मुश्किल security और performance bugs के लिए यह खास तौर पर महत्वपूर्ण है

    • एक पुराना तर्क और जोड़ें तो, लोग अपना code अलग तरह से लिखते हैं जब उन्हें पता होता है कि उसका review होगा और उसी code से submitter की क्षमता और संगठनात्मक fit के बारे में दीर्घकालिक धारणा बनेगी
    • तीसरे point जैसा ही, उस sub-domain से अधिक परिचित व्यक्ति मौजूदा code से मेल न खाने वाली या समस्या बन सकने वाली चीज़ें पकड़ सकता है
    • मैंने “single purpose” को इस अर्थ में पढ़ा कि code को समझो और जो हिस्से समझ न आएँ उन पर सवाल उठाओ। अगर मतलब यह है कि समझने के बाद आप गलत, मूर्खतापूर्ण या unsafe हिस्सों को चिह्नित कर सकते हैं, तो यह बात समझ आती है
      खासकर modularization और decomposition में ऐसा है; किसी बड़े PR को पूरा समझ लेने के बाद दिमाग में एक model बन जाता है, जिससे दिखने लगता है कि वह maintainable रहेगा, कभी पूरी nightmare बनेगा, या बीच की कोई स्थिति होगी

  • मेरे हिसाब से code review का शायद सबसे महत्वपूर्ण हिस्सा knowledge transfer है
    हमारी छोटी team में, urgent case न हो तो merge से पहले पूरी team PR पर approval देती है, और इससे सभी team members को current codebase की state का roughly पता रहता है। इससे पहले अधिक siloed company में झेले गए “जिस पूरे system पर मैं निर्भर था, वह गायब हो गया” जैसे surprises से बचा जा सकता है
    यह behavior के बारे में सवाल पूछने और समझ बढ़ाने की जगह भी बनता है। अच्छी तरह चलने वाली team में हर developer को पूरे system की कुछ समझ होनी चाहिए, उन parts की भी जिन्हें वे सीधे touch नहीं करते
    एक और महत्वपूर्ण function organizational knowledge check है। हाल ही में मैंने एक table में थोड़ा बदलाव किया, तो एक colleague ने बताया कि जिस microservice के बारे में मैंने सोचा भी नहीं था, वह उस table में write करती है और टूट जाएगी। मुझे उस microservice के existence या उसके उस table तक access का पता नहीं था, लेकिन उस check की वजह से बड़ा issue और data cleanup situation टल गई

    • “छोटी team पूरी की पूरी merge से पहले PR पर approval दे” धीमे चलने वाले छोटे codebase में अच्छा लगता है, लेकिन बड़ी team या तेज़ी से बदलते codebase पर इसे force करने से यह आसानी से code को सरसरी तौर पर देखकर approve button दबाने वाला formal ritual बन जाता है
      अंत में सब अपने काम में व्यस्त होते हैं और important PR को block करने वाला व्यक्ति नहीं बनना चाहते, इसलिए बस approve दबा देते हैं, जबकि वास्तव में कोई भी code review नहीं कर रहा होता
    • जानना चाहूँगा कि team का size कितना है। शायद पाँच engineers से ऊपर यह तरीका scale नहीं करेगा
      “जिस पूरे system पर मैं निर्भर था, वह गायब हो गया” जैसी समस्या के लिए मेरे हिसाब से automated tests बहुत ज़रूरी हैं, जो उस system पर निर्भर व्यक्ति के मौजूद न होने पर भी issue पकड़ सकें
      मैं हर चीज़ की shared ownership का भी मज़बूत समर्थक हूँ। लोगों का codebase के कुछ हिस्सों, खासकर अपने बनाए components, का practically owner बन जाना स्वाभाविक है, लेकिन इससे silos और low bus factor बनते हैं। ऐसा नहीं होना चाहिए कि एक व्यक्ति एक system own करे, और वह system फिर किसी दूसरे single component पर depend करे
    • अगर वह colleague review में नहीं होता तो उस breaking change को production में जाने से क्या रोकता, यह जानना चाहूँगा। अगर code review महत्वपूर्ण है तो tests कहाँ आते हैं
    • कभी-कभी ऐसे code के लिए भी PR बनाकर दूसरे developers को tag करता हूँ जिसे वैसे भी तुरंत merge करना है। मकसद यह है कि क्या और क्यों merge हुआ, इसे देखने का आसान रास्ता मिले ताकि लोग codebase के अंदर की चीज़ें miss न करें
    • non-trivial changes या cleanup work के लिए हमेशा दूसरी नज़र अच्छी होती है। लेकिन “हर कोई सब कुछ पढ़े” वाला तरीका बड़े N पर scale नहीं कर सकता
      जब पढ़ने के लिए बहुत अधिक हो जाता है तो कोई follow नहीं कर पाता, इसलिए delegate किया जाता है, docs बनाए जाते हैं और overview sessions रखे जाते हैं

  • मैंने हमेशा सोचा है कि code review को उस gateway के रूप में देखना सबसे अच्छा है जहाँ लेखक के ownership वाला code team या project की ownership में जाता है
    जिस code का मैं review कर रहा हूँ, वह आपका code नहीं, बल्कि जल्द ही हमारा code बनने वाला code है। स्वाभाविक है कि maintainability उसमें बड़ा factor है

    • सच में ईर्ष्या करने लायक और luxurious environment है
      हमारी team ने AI इस्तेमाल करना शुरू कर दिया है, इसलिए मैंने अपना तरीका simple कर दिया है। मैं comments नहीं छोड़ता, बस binary approval judgment करता हूँ: “क्या यह पागलपन की हद तक खराब है, या pass किया जा सकता है”
      मैं अपना समय और mental health बचा रहा हूँ

  • ऐसा करने से reviewer और author बस और आलसी हो जाएंगे
    code review का मकसद कई पहलुओं वाला होता है। क्या code maintain करना मुश्किल है, क्या उसमें bugs हो सकते हैं, क्या उसे और सरल और साफ़ बनाया जा सकता है, क्या वह project के code style से मेल खाता है, क्या वह दूसरों को code समझने में मदद करता है, क्या वह junior team members को onboard करता है, क्या वह design decisions का sanity check करता है—ये सब इसमें आते हैं
    इस तरह के हल्के लेख आम तौर पर आलसी code reviewers के अपने बचाव को सही ठहराने के ज्यादा करीब होते हैं

    • review में देखने के लिए अलग से एक checklist होती है
      देखना चाहिए कि issue या PR description के मुताबिक functionally लक्ष्य पूरा हो रहा है या नहीं, कोई बचा हुआ debug output या private API key जैसा अनावश्यक code तो नहीं है, memory leak, unhandled edge cases, security flaws, पुराने API calls जैसी साफ़ कमियां तो नहीं हैं
      यह भी देखना चाहिए कि क्या इसे और समझने लायक बनाया जा सकता है, abstraction जोड़ना या हटाना चाहिए या नहीं, variable/method names बेहतर हैं या नहीं, functional style का ज्यादा या कम इस्तेमाल करना बेहतर होगा या नहीं
      यह भी जांचना चाहिए कि codebase या style guide के साथ consistency है या नहीं, list की जगह hash set इस्तेमाल करने या lazy evaluation लागू करने जैसे साफ़ performance improvements हैं या नहीं, और tests पर्याप्त हैं या नहीं
      मैं इस दावे से भी पूरी तरह सहमत नहीं हूं कि अगर code समझ में नहीं आता तो उसे pass नहीं करना चाहिए। कुछ code सचमुच समझने में मुश्किल होता है, और लक्ष्य यह है कि वह functionally सही हो और जितना संभव हो उतना समझने लायक बनाया जाए
    • industry ने “author को दोष देने” से “process और team को दोष देने” की तरफ जाने की बहुत कोशिश की है
      लेकिन यह लेख बस clickbait जैसा है, और यह कहने जैसा है कि “लोग सोचते हैं dinner का मतलब खाना खाना है, पर असल में यह खाना नहीं बल्कि family और friends से जुड़ना है।” यह HN पर अच्छी तरह चलने वाली एक खास तरह की कमजोर reductionist logic है
    • AI की वजह से code लिखने और deploy करने की रफ्तार तेज हो गई है, इसलिए अब emphasis review पर शिफ्ट होना चाहिए। हमें देखना होगा कि code वाकई सही तरह से चलता है या नहीं, हमारी सभी assumptions सही हैं या नहीं, और कोई unintended side effects तो नहीं हैं
      मुझे लगा है कि review और debugging, code लिखने/produce करने से कहीं ज्यादा समय लेते हैं, और बस “काम कर जाए, इसकी प्रार्थना करना” कभी अच्छे नतीजे नहीं देता

  • “code को देखकर आम तौर पर bugs नहीं मिल सकते” यह बात बिल्कुल सही नहीं है। abstraction के हर level पर काफी कुछ मिल सकता है, और इन्हें code smells कहा जाता है
    बंद न किए गए file descriptors, await न किए गए coroutines, error log किए बिना किसी value पर वापस लौट जाने वाले विशाल try/catch blocks, गलत type casting—ये सब उसी में आते हैं
    सामान्य सिद्धांत के तौर पर, type checker, compiler और runtime को सिर्फ पार करने वाले stages के रूप में नहीं देखना चाहिए। इन stages के साथ काम करना चाहिए, यह मानना चाहिए कि वे मूल्यवान tools हैं, और इनके खिलाफ काम नहीं करना चाहिए

    • समझ नहीं आ रहा कि क्या कहा जा रहा है। मैंने code review में code चलाए बिना bugs पकड़े हैं, और उल्टा मेरे code में भी किसी और ने ऐसा किया है, और दूसरों के reviews में भी ऐसे उदाहरण देखे हैं
      “आम तौर पर” को किसी तरह technically सच बनाने के लिए define किया जा सकता है, लेकिन फिर उसका ज्यादा मतलब नहीं बचेगा

  • code review के बारे में broad दावे करने हों तो यह define करना जरूरी है कि किस तरह के code review की बात हो रही है
    आज GitHub-style asynchronous PR reviews और inline comments standard हैं, लेकिन review सिर्फ यही नहीं होते। पहले face-to-face reviews भी होते थे, जिनकी process thesis defense या conference presentation के ज्यादा करीब होती थी
    code review को उपयोगी quality practice बताने वाला literature—दरअसल उन कुछ quality practices में से एक जो उपयोगी हैं—ज्यादातर आज की तुलना में कहीं ज्यादा structured review processes से आया है
    निजी तौर पर, LLM से पहले के GitHub-style PR reviews मुझे process को ठीक महसूस कराने या governance checkbox भरने के लिए लगते थे, और LLM era में cost-benefit ratio बहुत खराब हो जाने से शायद वे गायब हो जाएंगे

    • synchronous reviews आज भी संभव हैं। मेरे शुरुआती managers में से एक ने सिखाया था कि अगर “standard” code review में एक से ज्यादा round आ-जा जाएं, तो लगभग हमेशा बेहतर है कि सीधे मिलकर, या अगर एक भी व्यक्ति remote हो तो Zoom पर, बात साफ़ कर ली जाए और फिर सहमति को comment में छोड़ दिया जाए
      अगर जबरन technical analogy दें, तो asynchronous text communication, सफलतापूर्वक encode की जा सकने वाली information के मामले में speech से ज्यादा lossy है और उसका throughput भी कम है। इसलिए जब बहुत सारी information exchange करनी हो, तो synchronization cost चुकाना worthwhile होता है
    • मेरी शुरुआती नौकरियों में से एक में change package print करके review करना और sign करना पड़ता था। final version को file cabinet में रखने के लिए एक जिम्मेदार व्यक्ति भी था
      यह traditional engineering के ज्यादा करीब था, और सभी को software को ज्यादा permanent चीज़ मानना पड़ता था

  • maintainability सुनिश्चित करना code review के फायदों में से एक है, यह सही है, लेकिन यही उसका एकमात्र उद्देश्य है कहना काफी bold claim है
    code review एक ऐसा tool भी है जिससे team code changes को समझती है और पूरे codebase के लिए shared responsibility बांटती है

  • code review कोई एक ही चीज़ नहीं है। knowledge sharing, responsibility washing, code quality, regulatory compliance जैसी कई वजहें होती हैं, और हमेशा की तरह उसका उद्देश्य क्या है यह usage context पर निर्भर करता है

    • यह कहना कि ज्यादातर लोग peer review की वजह नहीं समझते, काफी अनजान-सा लगता है। यह मानना कि इसका सिर्फ एक उद्देश्य है, अपने आप में ऐसा संकेत लगता है कि दूसरे teams या लोगों के साथ काम करने का अनुभव कम है

  • अगर author mathematician है, तो “code को देखकर आम तौर पर bugs नहीं मिल सकते” का मतलब यह नहीं होगा कि bugs ढूंढना पूरी तरह असंभव है
    इसका मतलब शायद यह है कि सभी bugs ढूंढना या कोई specific bug ढूंढना संभव नहीं है

    • university math lectures के अनुभव के आधार पर कहूं तो mathematicians अक्सर दूसरे इंसानों से communicate करने में बहुत खराब होते हैं। इससे समझ आता है कि वे क्यों सोचते हैं कि उन्होंने जो कहा और लगभग सभी लोग उसे जैसे पढ़ते हैं, वे अलग चीज़ें हैं
    • अगर मतलब यही है तो सही है
      maintainability वाले point से जोड़कर कहें तो code को जितना संभव हो उतना सरल बनाना ताकि review के जरिए debugging संभव होने की संभावना बढ़े, यह भी review का लक्ष्य है। यह absolute sense में bugs को नहीं रोकता, लेकिन probability बढ़ा देता है
    • लगता है mathematician author अपने natural-language quantifier का अर्थ नहीं समझ पाए। “code को देखकर आम तौर पर bugs नहीं मिल सकते” का मतलब है “code को देखकर आम तौर पर कोई भी bug नहीं मिल सकता,” न कि “सभी bugs नहीं मिल सकते”
      पहली interpretation relevant है लेकिन गलत है, और दूसरी interpretation सच है लेकिन relevant नहीं है
      शायद author कहना चाहते थे कि “किसी दिए गए bug को code देखकर आम तौर पर नहीं पाया जा सकता,” यानी “हर bug B के लिए यह जरूरी नहीं कि B को पाया जा सके,” लेकिन यह भी सच होते हुए मुख्य मुद्दे से दूर है

  • मैंने ऐसे लोगों के साथ काम किया है जो PR सुझावों को अक्सर ठुकरा देते हैं, और ऐसे लोगों के साथ भी जो सुझाव स्वीकार करते हैं
    जो व्यक्ति सुझाव स्वीकार करता है, उसके बारे में सोचने पर लगता है कि वह कुछ हद तक ownership मेरे साथ साझा करना चाहता है। ऐसा महसूस होता है कि हम दोनों code को maintain और समझ रहे हैं, और एक ही दिशा में देख रहे हैं
    इसके उलट, जो व्यक्ति PR सुझावों को ठुकराता है, उसके PR में भाग लेने की इच्छा कम हो जाती है। अगर वैसे भी अस्वीकार ही होना है, तो समय लगाकर इतनी बारीकी से review क्यों किया जाए

    • हमारी टीम comments के आगे आम तौर पर thought, change, nit, fix, chat जैसे prefix लगाती है
      उदाहरण के लिए, thought का मतलब कुछ ऐसा होता है: “बाद में foo और आम हो सकता है, तब refactor कर लेंगे”; change: “यह leaky abstraction है, इसलिए इसे bar की तरह model करना चाहूंगा”; nit: “नाम थोड़ा कम intuitive है, Baz या Boo पर विचार करें”; fix: “यह unit test गलत field को verify करता है”; chat: “यह इस category के समाधान का स्वरूप तय करने वाला बड़ा decision है, इसलिए पहले team से discuss करें”
      कुछ prefix का मतलब होता है कि बदलाव होने तक PR रुका रहेगा, और कुछ का मतलब होता है कि comment मानना या न मानना लेखक पर है। लेखक के लिए यह स्पष्ट हो जाता है कि क्या चीज़ “साझा समझ तक पहुंचने वाली बात” है और क्या “पसंदीदा अभिव्यक्ति” या “observation” है
      हालांकि, अगर आपने nit छोड़ा है और सामने वाला सहमत न होकर उसे ignore कर दे, तो बुरा नहीं मानना चाहिए। अगर आपको वह बात strongly महसूस हुई थी, तो वह nit नहीं होनी चाहिए थी
    • अगर आपको लगता है कि बात महत्वपूर्ण है, तो blocking suggestion छोड़कर बातचीत को मजबूर करना चाहिए।