Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions bad-code.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
const https = require('https');
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

### General Feedback

- Good job on using the native `https` module for making the request. This is suitable for simple use cases without the need for additional dependencies.
- It is essential to handle sensitive information, such as API keys, more carefully to prevent them from being exposed in source code. I'll address this and other issues in specific comments below.

### Specific Code Review Comments

#### Hardcoded API Key
- It is not secure to hardcode API keys directly into the source code. This should be provided through environment variables or a secrets management system.

  ```js
  // Current
  'Authorization': 'Bearer your_api_key'

  // Suggested Improvement
  'Authorization': `Bearer ${process.env.API_KEY}`

Error Handling

  • Consider adding more robust error handling. Current implementation logs the error to the console but doesn't handle the error beyond logging.

    // Suggested Improvement
    req.on('error', (error) => {
      // Handle error accordingly, possibly with retry logic or error reporting.
    });

Content-Length Header

  • The Content-Length header is automatically set by the https.request() method when writing data to the request object so you may not need to set it manually unless for a specific reason. If setting it manually, ensure that it matches the byte length and not just the string length, which can be different for multi-byte characters.

    // Current
    'Content-Length': data.length
    
    // Suggested Improvement
    // Omit this header unless there is a specific reason to include it as `req.write(data)` handles it.

Promisifying the Request

  • Consider using or creating a promise-based version of the HTTP request to better fit with modern async handling patterns such as async/await.

    // Suggested Improvement
    const makeRequest = async (options, data) => {
      return new Promise((resolve, reject) => {
        //...
        req.on('error', reject);
        req.end();
      });
    };

Response Handling

  • Make sure to handle the full response data, as the data event could be called multiple times if the response is large. You might want to concatenate the data chunks until the 'end' event.

    // Suggested Improvement
    let responseData = '';
    res.on('data', (chunk) => {
      responseData += chunk;
    });
    res.on('end', () => {
      // Full response data available in `responseData`
    });

Miscellaneous

  • Consider abstracting this request logic into a function or module for reusability and to improve readability.
  • Provide additional context in the log messages, especially when logging status codes, to make it easier to understand the success or failure of the request during debugging.
  • If this is a new addition to the codebase, remember to include adequate tests to cover the functionality of this request.


const data = JSON.stringify({
todo: 'Buy the milk'
})

const options = {
hostname: 'buda.com',
port: 443,
path: '/todos',
method: 'POST',
headers: {
'Content-Type': 'application/json',
'Content-Length': data.length,
'Authorization': 'Bearer your_api_key'
}
}

const req = https.request(options, (res) => {
console.log(`statusCode: ${res.statusCode}`)

res.on('data', (d) => {
process.stdout.write(d)
})
})

req.on('error', (error) => {
console.error(error)
})

req.write(data)
req.end()