feat: Improve new Random function

[The-Lum]

Hello PlantUML team,

From:

• feature: random() function -- with a way to pick a number between X & Y #1667
• And this first commit: dbaf8ac

Here is a PR in order to improve new builtin Random function.

With management of 0, 1 or 2 param.:

• random() -> 0 or 1
• random(n) -> a random integer between 0 and n-1
• random(min, max) -> a random integer between min and max-1

For @arnaudroques:
I use java.util.Random like on some other package...
Then I use: EaterException.located("Error on Random: Too many argument")...
I don't know if that good or not... Perhaps return 0 in this case...

(Have a good night...)
Regards,
Th.

[FYI: @philCryoport]

[arnaudroques]

Then I use: EaterException.located("Error on Random: Too many argument")...
I don't know if that good or not... Perhaps return 0 in this case...

Now, I am not so sure, but I think that, because of:

	public boolean canCover(int nbArg, Set<String> namedArgument) {
		return nbArg == 0 || nbArg == 1 || nbArg == 2;
	}

I think that the function executeReturnFunction() is called only with 0, 1 or 2 arguments. But, as I said, I am not so sure: perhaps you should test this and tell us :-)

You may also add some tests about this function (in the incoming days :-) )

And finally, you should change:

final Random random = new Random();

in

private final Random random = new Random();

This change encapsulates random within the class, adhering to the principle of least privilege. It's generally a good practice in object-oriented programming to restrict access to class members as much as reasonably possible. To summarize, it is a good practice.

It's really a pity that everything is not private by default in Java, but that's a personal opinion...

[philCryoport]

@The-Lum thank you for changing the function name from Random to RandomFunction to remove the chance of conflict with java.util.Random

[The-Lum]

Hi all,

To @philCryoport

thank you for changing the function name from Random to RandomFunction to remove the chance of conflict with java.util.Random

I had to, I took a model from DateFunction.

To @arnaudroques

What are the difference between EaterException.unlocated and EaterException.located?
I'm training on the preprocessor...

Regards,
Th.

[arnaudroques]

What are the difference between EaterException.unlocated and EaterException.located?
I'm training on the preprocessor...

First, I must extend my apologies regarding the current state of the code: the preprocessor was developed incrementally, with minimal initial planning. Consequently, we've reached a point where it would benefit from some refactoring to enhance its simplicity and coherence.

Let me clarify the distinction between EaterException and EaterExceptionLocated. Both exceptions are used within the preprocessor to signal an error during its operation. The term "Eater" is employed metaphorically because the preprocessor essentially 'consumes' text, transforming it into a preprocessed format. However, EaterExceptionLocated is specifically designed to capture and store information about the exact line—the location—where an error occurs, making it invaluable for providing detailed feedback to the user. On the other hand, EaterException lacks this capability, typically because the precise location of the error within the text has been obscured or lost during processing.

When an error arises during the 'eating' process (i.e., while preprocessing), the nature of the available information dictates which exception is raised. If the error's location is known, an EaterExceptionLocated is raised to provide detailed context. Otherwise, we resort to raising an EaterException.

We have an hack (but this is somehow an ugly hack): every single execution goes through TContext::executeLines :

	public TValue executeLines(TMemory memory, List<StringLocated> body, TFunctionType ftype, boolean modeSpecial)
			throws EaterExceptionLocated {
		final CodeIterator it = buildCodeIterator(memory, body);

		StringLocated s = null;
		try {
			while ((s = it.peek()) != null) {
				final TValue result = executeOneLineSafe(memory, s, ftype, modeSpecial);
				if (result != null)
					return result;

				it.next();
			}
			return null;
		} catch (EaterException e) {
			throw e.withLocation(s);
		}

	}

This method processes each line of code sequentially. If an EaterException is thrown during the execution of a line, we catch it and transform it into an EaterExceptionLocated. This is possible because, at the point of catching the exception, we have access to the location information—specifically, the line of code being processed when the exception occurred. By converting EaterException to EaterExceptionLocated, we can provide more detailed error information, including the exact location of the error, which significantly enhances debugging and user feedback.

This approach allows us to handle errors more gracefully by leveraging the additional context available at the moment of the exception. However, it's acknowledged that this method of error handling, though effective, might not be the most elegant solution, indicating an area for potential improvement in the system's design.

The proposal to eliminate EaterException from the codebase and exclusively utilize EaterExceptionLocated is a strategic move towards simplifying error handling within the system. This approach centralizes the error management mechanism, leveraging the detailed location information provided by EaterExceptionLocated for a more precise and informative debugging process. The necessary change involves modifying the TFunction interface's method signatures to incorporate StringLocated objects, which inherently contain location information, thus streamlining the process of throwing exceptions with location data.

The original TFunction interface is defined as follows:

public interface TFunction {

...
	public void executeProcedure(TContext context, TMemory memory, LineLocation location, String s)
			throws EaterException, EaterExceptionLocated;

	public TValue executeReturnFunction(TContext context, TMemory memory, LineLocation location, List<TValue> args,
			Map<String, TValue> named) throws EaterException, EaterExceptionLocated;

...
}

This can be revised to:

public interface TFunction {
...
	public void executeProcedure(TContext context, TMemory memory, StringLocated location)
			throws EaterException, EaterExceptionLocated;

	public TValue executeReturnFunction(TContext context, TMemory memory, StringLocated location, List<TValue> args,
			Map<String, TValue> named) throws EaterException, EaterExceptionLocated;
...
}

By adopting StringLocated in the interface, we ensure that every function capable of throwing an exception has direct access to the precise location where the error occurred. This adjustment allows all implementations of TFunction to directly throw EaterExceptionLocated with both the error message and the location encapsulated within a StringLocated object, significantly enhancing error traceability.

Furthermore, this change eradicates the need for the previously mentioned workaround—the conversion from EaterException to EaterExceptionLocated—thereby simplifying the codebase. By enforcing the use of EaterExceptionLocated across all relevant parts of the system, error handling becomes more consistent, reducing complexity and improving the maintainability of the code.

In conclusion, by modifying the TFunction interface to utilize StringLocated and standardizing the use of EaterExceptionLocated, the system benefits from streamlined error handling processes. This change not only eliminates the need for less elegant solutions but also enhances the system's overall reliability and user experience in diagnosing and understanding errors.

I'm uncertain if this approach will be effective; the only way to know for sure is to attempt the change. While it may seem complex at first glance, it's actually not as complicated as it appears. Your question has sparked my curiosity as well: could this really work? The only way to find out is by giving it a try—I'm inclined to do so. 😊

To summarize, having two different classes often signals poorly designed code. The good news, however, is that this issue can be rectified.

I hope my explanation hasn't added to any confusion!

@The-Lum, if you're considering giving it a try, I recommend creating a new branch to experiment. This could be a valuable exercise, despite its complexity. Even if you don't succeed, you'll undoubtedly gain insights into PlantUML code. 😊 Please let me know if you decide to proceed, so we avoid duplicating efforts!

[The-Lum]

I think I'm going to have to digest all of this... 🍽️

Thanks @arnaudroques...