Code Review results documented
parent
4d6f9092eb
commit
2550724333
@ -0,0 +1,27 @@
|
||||
## Code explanation
|
||||
This is a program manages the processing of orders in the following way: when an order is registered with the `RegisterOrder` method, the order is first deleted using the `DeleteOrder` method. Then, the order is converted to a list of `ReviewExportLine` objects using the `ConvertOrderToExportLines` method and added to a queue of lists of `ReviewExportLine` objects. There is a separate thread running in the background which was mentioend in the assignment itself. This service continuously checks the queue and processes the review export lines when the queue is not empty and the review folder is empty. When the review folder is not empty, the thread waits for 2 seconds and checks the queue again. The processing of review export lines involves opening a file in the review folder with a random 7 character string as the file name, writing the review export lines to the file, and then closing the file.
|
||||
|
||||
## Problems with the code
|
||||
1. The `ProcessReviewExportLines` method runs in an infinite loop, which means that it will run indefinitely until the program is terminated. This may not be desirable behavior, especially if the program is intended to run for a fixed amount of time or in response to specific user input. Also, this seems hacky: a better practice would be to use some sort of a listener or a state observer (with observer pattern).
|
||||
|
||||
2. Possible race-conditions: the `ProcessReviewExportLines` method checks whether the review folder is empty before processing the review export lines. However, it is possible that other processes or threads may be writing to the review folder at the same time, which could cause the program to skip processing the review export lines even if the folder is not actually empty.
|
||||
|
||||
3. The `ProcessReviewExportLines` method writes to the review folder using a randomly generated file name. This may cause problems if the program needs to read the contents of the review folder at a later time, as it will be difficult to determine which file corresponds to which order.
|
||||
|
||||
4. The `FilterString` method replaces semicolons (;) with colons (:). This may cause problems if the input strings contain colons that are not intended to be replaced. If it is such a simple operation, then it should be done locally.
|
||||
|
||||
5. The `DeleteOrder` method enqueues a single ReviewExportLine object with a Type field value of "9" to the queue. It is not clear what the purpose of this line is, or how it is related to the process of deleting an order.
|
||||
|
||||
6. `GenerateRandomString` method does not check the validity of the returned path: Since it generates these characters randomly, there could be a chaater that is considered illegal to be used as in a system-path. Additionally, 7 characters (which are very bounded) might not prove to be enough to prevent collisions.
|
||||
|
||||
## How to solve them
|
||||
1. Add a condition to the loop to terminate it after a certain number of iterations or after a certain amount of time has passed Alternatively, a cancellation token to the method and pass it a token that can be used to cancel the loop. Or used better design-patterns
|
||||
|
||||
2. Use a locking-mechanism to syncronize access to a shared resource, in this case, this folder.
|
||||
|
||||
3. Use an incrementative / more predictable file name. Such as using timestamps.
|
||||
|
||||
4. Move the operation upwards, or use the method better, to actually filter-out invalid characters.
|
||||
|
||||
5. Add proper useful logic. Or add comments explaining it.
|
||||
|
||||
@ -1,144 +1,144 @@
|
||||
// using SollicitantReview.Models;
|
||||
// using SollicitantReview.Services;
|
||||
// using Microsoft.Extensions.Options;
|
||||
// using System;
|
||||
// using System.Collections.Generic;
|
||||
// using System.IO;
|
||||
// using System.Linq;
|
||||
// using System.Text;
|
||||
// using System.Threading;
|
||||
// using System.Threading.Tasks;
|
||||
|
||||
// namespace SollicitantReview
|
||||
// {
|
||||
// public class SollicitantReview
|
||||
// {
|
||||
// private SollicitantReviewSettings options;
|
||||
// private IWriter writer;
|
||||
// private Queue<List<ReviewExportLine>> exportLineQueue;
|
||||
|
||||
// public SollicitantReview(IOptions<SollicitantReviewSettings> options, IWriter writer)
|
||||
// {
|
||||
// this.options = options.Value;
|
||||
// this.writer = writer;
|
||||
// this.exportLineQueue = new Queue<List<ReviewExportLine>>();
|
||||
|
||||
// var processExportLinesThread = new Thread(new ThreadStart(ProcessReviewExportLines));
|
||||
// processExportLinesThread.Start();
|
||||
// }
|
||||
|
||||
// public void RegisterOrder(Order order)
|
||||
// {
|
||||
// DeleteOrder(order);
|
||||
// exportLineQueue.Enqueue(ConvertOrderToExportLines(order));
|
||||
// }
|
||||
|
||||
// private string FilterString(string value)
|
||||
// {
|
||||
// return value.Replace(";", ":");
|
||||
// }
|
||||
|
||||
// private void DeleteOrder(Order order)
|
||||
// {
|
||||
// var exportLines = new List<ReviewExportLine>()
|
||||
// {
|
||||
// new ReviewExportLine()
|
||||
// {
|
||||
// Type = "9",
|
||||
// OrderNumber = FilterString(order.OrderNumber),
|
||||
// Amount = "1",
|
||||
// UserId = FilterString(order.User),
|
||||
// JournalPostIndication = "Y"
|
||||
// }
|
||||
// };
|
||||
|
||||
// exportLineQueue.Enqueue(exportLines);
|
||||
// }
|
||||
|
||||
// private List<ReviewExportLine> ConvertOrderToExportLines(Order order, string userId = null)
|
||||
// {
|
||||
// var exportLines = new List<ReviewExportLine>();
|
||||
|
||||
// foreach (var orderLine in order.OrderLines)
|
||||
// {
|
||||
// exportLines.Add(new ReviewExportLine()
|
||||
// {
|
||||
// OrderNumber = FilterString(order.OrderNumber),
|
||||
// Name = FilterString(orderLine.Name),
|
||||
// Amount = FilterString(orderLine.Amount),
|
||||
// ProductNumber = FilterString(orderLine.Number),
|
||||
// ProductDescription = FilterString(orderLine.Description),
|
||||
// UserId = userId == null ? FilterString(order.User) : userId,
|
||||
// Location = FilterString(orderLine.Location),
|
||||
// JournalPostIndication = "Y"
|
||||
// });
|
||||
// }
|
||||
|
||||
// return exportLines;
|
||||
// }
|
||||
|
||||
// private void ProcessReviewExportLines()
|
||||
// {
|
||||
// while(true)
|
||||
// {
|
||||
// if (exportLineQueue.Any() && IsFolderEmpty(options.ReviewFolderPath))
|
||||
// {
|
||||
// var exportLines = exportLineQueue.Dequeue();
|
||||
// var randomStringLength = 7;
|
||||
|
||||
// writer.OpenFile($"{options.ReviewFolderPath}\\{GenerateRandomString(randomStringLength)}.txt");
|
||||
|
||||
// foreach (var exportLine in exportLines)
|
||||
// {
|
||||
// writer.WriteLine($"{exportLine.Type};" +
|
||||
// $"{exportLine.OrderNumber};" +
|
||||
// $"{exportLine.Name};" +
|
||||
// $"{exportLine.Amount};" +
|
||||
// $"{exportLine.ProductNumber};" +
|
||||
// $"{exportLine.ProductDescription};" +
|
||||
// $"{exportLine.UserId};" +
|
||||
// $"{exportLine.Location};" +
|
||||
// $"{exportLine.JournalPostIndication};" +
|
||||
// $"{exportLine.Info2}");
|
||||
// }
|
||||
|
||||
// writer.CloseFile();
|
||||
// }
|
||||
|
||||
// Thread.Sleep(2000);
|
||||
// }
|
||||
// }
|
||||
|
||||
// private bool IsFolderEmpty(string path)
|
||||
// {
|
||||
// DirectoryInfo dirInfo = new DirectoryInfo(path);
|
||||
|
||||
// if (!dirInfo.Exists)
|
||||
// {
|
||||
// throw new CouldNotFindReviewDirectory();
|
||||
// }
|
||||
|
||||
// return !dirInfo.GetFiles("*.txt").Any();
|
||||
// }
|
||||
|
||||
// private string GenerateRandomString(int length)
|
||||
// {
|
||||
// StringBuilder stringBuilder = new StringBuilder();
|
||||
// Random random = new Random();
|
||||
// var ASCIIOffset = 65;
|
||||
// var ASCIIRange = 25;
|
||||
|
||||
// char letter;
|
||||
|
||||
// for (int i = 0; i < length; i++)
|
||||
// {
|
||||
// double randomDouble = random.NextDouble();
|
||||
// int shift = Convert.ToInt32(Math.Floor(ASCIIRange * randomDouble));
|
||||
// letter = Convert.ToChar(shift + ASCIIOffset);
|
||||
// stringBuilder.Append(letter);
|
||||
// }
|
||||
|
||||
// return stringBuilder.ToString();
|
||||
// }
|
||||
// }
|
||||
// }
|
||||
using SollicitantReview.Models;
|
||||
using SollicitantReview.Services;
|
||||
using Microsoft.Extensions.Options;
|
||||
using System;
|
||||
using System.Collections.Generic;
|
||||
using System.IO;
|
||||
using System.Linq;
|
||||
using System.Text;
|
||||
using System.Threading;
|
||||
using System.Threading.Tasks;
|
||||
|
||||
namespace SollicitantReview
|
||||
{
|
||||
public class SollicitantReview
|
||||
{
|
||||
private SollicitantReviewSettings options;
|
||||
private IWriter writer;
|
||||
private Queue<List<ReviewExportLine>> exportLineQueue;
|
||||
|
||||
public SollicitantReview(IOptions<SollicitantReviewSettings> options, IWriter writer)
|
||||
{
|
||||
this.options = options.Value;
|
||||
this.writer = writer;
|
||||
this.exportLineQueue = new Queue<List<ReviewExportLine>>();
|
||||
|
||||
var processExportLinesThread = new Thread(new ThreadStart(ProcessReviewExportLines));
|
||||
processExportLinesThread.Start();
|
||||
}
|
||||
|
||||
public void RegisterOrder(Order order)
|
||||
{
|
||||
DeleteOrder(order);
|
||||
exportLineQueue.Enqueue(ConvertOrderToExportLines(order));
|
||||
}
|
||||
|
||||
private string FilterString(string value)
|
||||
{
|
||||
return value.Replace(";", ":");
|
||||
}
|
||||
|
||||
private void DeleteOrder(Order order)
|
||||
{
|
||||
var exportLines = new List<ReviewExportLine>()
|
||||
{
|
||||
new ReviewExportLine()
|
||||
{
|
||||
Type = "9",
|
||||
OrderNumber = FilterString(order.OrderNumber),
|
||||
Amount = "1",
|
||||
UserId = FilterString(order.User),
|
||||
JournalPostIndication = "Y"
|
||||
}
|
||||
};
|
||||
|
||||
exportLineQueue.Enqueue(exportLines);
|
||||
}
|
||||
|
||||
private List<ReviewExportLine> ConvertOrderToExportLines(Order order, string userId = null)
|
||||
{
|
||||
var exportLines = new List<ReviewExportLine>();
|
||||
|
||||
foreach (var orderLine in order.OrderLines)
|
||||
{
|
||||
exportLines.Add(new ReviewExportLine()
|
||||
{
|
||||
OrderNumber = FilterString(order.OrderNumber),
|
||||
Name = FilterString(orderLine.Name),
|
||||
Amount = FilterString(orderLine.Amount),
|
||||
ProductNumber = FilterString(orderLine.Number),
|
||||
ProductDescription = FilterString(orderLine.Description),
|
||||
UserId = userId == null ? FilterString(order.User) : userId,
|
||||
Location = FilterString(orderLine.Location),
|
||||
JournalPostIndication = "Y"
|
||||
});
|
||||
}
|
||||
|
||||
return exportLines;
|
||||
}
|
||||
|
||||
private void ProcessReviewExportLines()
|
||||
{
|
||||
while(true)
|
||||
{
|
||||
if (exportLineQueue.Any() && IsFolderEmpty(options.ReviewFolderPath))
|
||||
{
|
||||
var exportLines = exportLineQueue.Dequeue();
|
||||
var randomStringLength = 7;
|
||||
|
||||
writer.OpenFile($"{options.ReviewFolderPath}\\{GenerateRandomString(randomStringLength)}.txt");
|
||||
|
||||
foreach (var exportLine in exportLines)
|
||||
{
|
||||
writer.WriteLine($"{exportLine.Type};" +
|
||||
$"{exportLine.OrderNumber};" +
|
||||
$"{exportLine.Name};" +
|
||||
$"{exportLine.Amount};" +
|
||||
$"{exportLine.ProductNumber};" +
|
||||
$"{exportLine.ProductDescription};" +
|
||||
$"{exportLine.UserId};" +
|
||||
$"{exportLine.Location};" +
|
||||
$"{exportLine.JournalPostIndication};" +
|
||||
$"{exportLine.Info2}");
|
||||
}
|
||||
|
||||
writer.CloseFile();
|
||||
}
|
||||
|
||||
Thread.Sleep(2000);
|
||||
}
|
||||
}
|
||||
|
||||
private bool IsFolderEmpty(string path)
|
||||
{
|
||||
DirectoryInfo dirInfo = new DirectoryInfo(path);
|
||||
|
||||
if (!dirInfo.Exists)
|
||||
{
|
||||
throw new CouldNotFindReviewDirectory();
|
||||
}
|
||||
|
||||
return !dirInfo.GetFiles("*.txt").Any();
|
||||
}
|
||||
|
||||
private string GenerateRandomString(int length)
|
||||
{
|
||||
StringBuilder stringBuilder = new StringBuilder();
|
||||
Random random = new Random();
|
||||
var ASCIIOffset = 65;
|
||||
var ASCIIRange = 25;
|
||||
|
||||
char letter;
|
||||
|
||||
for (int i = 0; i < length; i++)
|
||||
{
|
||||
double randomDouble = random.NextDouble();
|
||||
int shift = Convert.ToInt32(Math.Floor(ASCIIRange * randomDouble));
|
||||
letter = Convert.ToChar(shift + ASCIIOffset);
|
||||
stringBuilder.Append(letter);
|
||||
}
|
||||
|
||||
return stringBuilder.ToString();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue