Hello, inspired by the "Tiny RPN calculator" example seen on the dlang homepage, I wanted to create my own version.

I'd appreciate your opinions regarding style, mistakes/code smell/bad practice. Thank you.

    import std.array;
    import std.conv;
    import std.stdio;

    void main(string[] args)
    {
        if (args.length == 1)
        {
            writeln("usage examples:\n\t", args[0], " 5 3 + 2 *\n\t", args[0],
                    " 12.1 11 /");
            return;
        }

        immutable string valid_ops = "+-*/";
        string spaced_args = args[1 .. $].join(" ");

        // this is done to correctly handle "messy" inputs such as "5 3 2*+"
        foreach (o; valid_ops)
        {
            spaced_args = spaced_args.replace(o, " " ~ o ~ " ");
        }

        real[] stack;

        foreach (op; spaced_args.split)
        {
    LabeledSwitch:
            switch (op) // operator or operand?
            {
                static foreach (o; valid_ops)
                {
                case [o]:
                    stack = stack[0 .. $ - 2] ~
                            mixin("stack[$ - 2]" ~ o ~ "stack[$ - 1]");
                    break LabeledSwitch;
                }
            default:
                // assume it's an operand, a real number
                stack ~= to!real(op);
                break;
            }
        }

        writeln(stack);
    }

On Friday, 17 July 2020 at 21:37:46 UTC, AB wrote:
> Hello, inspired by the "Tiny RPN calculator" example seen on the dlang homepage, I wanted to create my own version.
>
> I'd appreciate your opinions regarding style, mistakes/code smell/bad practice. Thank you.

I generally don't know what I'm talking about, but nothing stands out as outright wrong. Style is unique from person to person.

valid_ops is a compile-time constant and can be made an enum.

I'm not happy about the looping and allocating replacements of spaced_args, but it's an easy solution where alternatives quickly become tricky or involve regular expressions (something like `spaced_args.matchAll("[0-9]+|[+*/-]".regex)`). Regex is neat but heavy.

You can use std.algorithm.iteration.splitter instead of std.array.split to lazily foreach spaced_args.

I don't know enough to say if `case [o]` will allocate at runtime. If so, it could be replaced with `case to!string(o)`, but maybe the compiler is smart enough to not consider [o] a runtime literal. For a program this short it doesn't really matter, but for a longer one it might be worth looking up.

You're not checking for array size before slicing stack, and as such the program range errors out on operator-operand count mismatch.

The rest is micro-optimisation and nit-picking. If valid_ops is an enum, you could foreach over it as an AliasSeq with a normal foreach (`foreach (o; aliasSeqOf!valid_ops)`; see std.meta). You could use std.array.Appender instead of appending to a real[]. etc.

On Saturday, 18 July 2020 at 10:33:23 UTC, Anonymouse wrote:
> I'm not happy about the looping and allocating replacements of spaced_args,

Actually, I got rid of that code entirely in order to support negative values:

    -5 -6 *
        == 30

On Friday, 17 July 2020 at 21:37:46 UTC, AB wrote:
> I'd appreciate your opinions regarding style, mistakes/code smell/bad practice. Thank you.

In a project this small, implementability (meaning, ease of writing) is really the main guideline, readability is a non-issue. When your codebase hits a few hundred lines in size you should start to gradually pay more attention to readability/scalablity.

In fact, things that are recommended in 2000-line projects may even be antipatterns in 100-line projects. For example, unit testing greatly reduces bugs, so it's highly recommended in general. But if your project is so small that normal manual testing covers most everything anyway, unit testing is just needless bloat.

A github gist might be better when doing style reviews, because the forum snippets tend to have wandering indentation and line wrapping.