Custom sorting a list of objects efficiently I'm trying to make sorting of objects in my program as fast as possible. I know that sorting with a cmp function is deprecated in python 3.x and that using keys is faster, but I'm not sure how to get the same functionality without using a cmp function.

Here's the definition of the class:

class Topic:
    def __init__(self, x, y, val):
        self.id = val
        self.x = x
        self.y = y

I have a dictionary full of Topic to float key, value pairs and a list of Topics to be sorted. Each topic in the list of Topics to be sorted has an entry in this dictionary. I need to sort the list of topics by the value in the dictionary. If two topics have a difference in value <= .001, the topic with higher ID should come first.

Here's the current code I'm using:

topicToDistance = {}
# ...
# topicToDistance populated
# ...
topics = topicToDistance.keys()
def firstGreaterCmp(a, b):
  if abs(topicToDistance[a]-topicToDistance[b]) <= .001:
    if a.id < b.id:
      return 1
  if topicToDistance[a] > topicToDistance[b]:
      return 1
  return -1
# sorting by key may be faster than using a cmp function
topics.sort(cmp = firstGreaterCmp)

Any help making this a bit more efficient would be greatly appreciated.

Note that comparison doesn't have to return 1 or -1. For example returning 123 would have the same effect as returning 1. With this in mind, the firstGreaterCmp function could be somewhat simpler:

def firstGreaterCmp(a, b):
    difference = topicToDistance[a] - topicToDistance[b]
    if abs(difference) <= .001:
        if a.id < b.id:
            return 1
    return int(difference)

On the other hand, the comparison function must return an int, and since your value is a float, I had to cast it. I don't really know if that has a non-negligible cost or not.

UPDATE

Also, as @janne-karila pointed out in a comment,
int(difference) is 0 for a range of values (due to integer truncation),
while your original code never returns 0.
It depends on your use case whether this is acceptable or not.

In the Topic class, I find it somewhat strange that the id parameter comes last in the parameter list.
Normally I would expect an id field to be the first.

Your code doesn't follow PEP8. It would be better this way:

def firstGreaterCmp(a, b):
    if abs(topicToDistance[a] - topicToDistance[b]) <= .001:
        if a.id < b.id:
            return 1
    if topicToDistance[a] > topicToDistance[b]:
        return 1
    return -1

# sorting by key may be faster than using a cmp function
topics.sort(cmp=firstGreaterCmp)


Ability System in my game I'm writing a game in C# Monogame, and I've decided to make my project as modular as possible.

GitHub

At first, I've had a single project with a folder that's separated inside to other folders which represent different systems in the game (Items, Abilities, Stats).

I've started by making a .dll project for everything that I've considered as a 'system', i.e - Items, Abilities, Stats, and others that I will soon add, like the player itself (that's just a class right now).

I'm designing the Abilities system based on the answer given on this post.

I'm asking for a code review about the Ability system module, and if you care, about the rest of the game (the MainModule is just remainder of what I've had before I refactored into modularizing everything, so for now there's not much need for whatever's there, unless you see something the could help).

I'd appreciate referencing stuff (or solutions to problems I have) to design patterns, as I'd rather use design patterns to solve problems.

The ability system code:

public interface IBehavior
{
    void ApplyBehavior(IUnit unit);
}

// ---------------------------------------------------- //

public abstract class ActivatableBehavior : IBehavior
{
    public abstract bool BehaviorImplentation(IUnit destinationPlayer);

    public void ApplyBehavior(IUnit unit)
    {
        BehaviorImplentation(unit);
    }
}

// ---------------------------------------------------- //

public abstract class LimitedTimeBehavior : IBehavior
{
    public Stopwatch Timer { get; }
    public abstract void ApplyBehavior(IUnit unit);
    public long Duration { get; }

    protected LimitedTimeBehavior(long durationOfBehavior)
    {
        Timer = new Stopwatch();
        Duration = durationOfBehavior;
    }
}

// ---------------------------------------------------- //

public abstract class TickBasedBehavior : LimitedTimeBehavior
{
    public long TickTime { get; }
    protected abstract void ApplyTick(IUnit unit);

    protected TickBasedBehavior(long durationOfBehavior, long timeBetweenTicks)
        : base(durationOfBehavior)
    {
        TickTime = timeBetweenTicks;
    }

    public override void ApplyBehavior(IUnit unit)
    {
        Thread trd = new Thread(() => ThreadMethod(unit));
        trd.Start();
    }

    private void ThreadMethod(IUnit unit)
    {
        Timer.Start();

        while (Timer.ElapsedMilliseconds < Duration)
        {
            if (Timer.Elapsed.TotalMilliseconds % TickTime == 0)
                ApplyTick(unit);
        }

        Timer.Stop();
    }
}

// ---------------------------------------------------- //

public abstract class DurationBehavior : LimitedTimeBehavior
{
    protected abstract void Activate(IUnit unit);
    protected abstract void Deactivate(IUnit unit);

    private bool IsActivated { get; set; }

    protected DurationBehavior(long durationOfBehavior)
        : base(durationOfBehavior)
    {
        IsActivated = false;
    }

    public override void ApplyBehavior(IUnit unit)
    {
        if (IsActivated) return;
        Thread trd = new Thread(() => ThreadMethod(unit));
        trd.Start();
        IsActivated = true;
    }

    private void ThreadMethod(IUnit unit)
    {
        Timer.Start();
        Activate(unit);
        while (Timer.Elapsed.TotalMilliseconds < Duration) { }
        Timer.Reset();
        Deactivate(unit);
        IsActivated = false;
    }
}

That is really not how you should be using a Stopwatch...

E.g. this

while (Timer.Elapsed.TotalMilliseconds < Duration) { }

You're needlessly using CPU cycles while you're waiting. That's not good! You know exactly when you want to do something so you should scheduled something rather than do pretend work. A simple example could be:

private void ThreadMethod(IUnit unit)
{
    Activate(unit);
    Task.Delay(Duration).ContinueWith(_ => Deactivate(unit));
}

This isn't safe either:

while (Timer.ElapsedMilliseconds < Duration)
{
    if (Timer.Elapsed.TotalMilliseconds % TickTime == 0)
        ApplyTick(unit);
}

You're very unlikely to actually get true on that if statement meaning you're unlikely to call ApplyTick and almost certainly won't call it the number of times you want to! That's because a Tick is a really, really small measure of time... I want to say 1/10000 of a millisecond.

Edit - I misread your if statement - I thought you were using Ticks (due to the TickTime field. It's still not a great idea but less serious than the above implies.

You shouldn't need a stopwatch at all for this sort of thing.

You should use the highest level of abstraction you can. A double can be anything (e.g. hours, minutes, seconds...) - a timespan is a period of time, much more suitable for your LimitedTimeBehavior:

public abstract class LimitedTimeBehavior : IBehavior
{
    public abstract void ApplyBehavior(IUnit unit);

    public TimeSpan Duration { get; }

    protected LimitedTimeBehavior(TimeSpan duration)
    {
        Duration = duration;
    }
}

You said you're interested in design patterns so I though I'd draw your attention to one you're nearly using:

public abstract class ActivatableBehavior : IBehavior
{
    public abstract bool BehaviorImplentation(IUnit destinationPlayer);

    public void ApplyBehavior(IUnit unit)
    {
        BehaviorImplentation(unit);
    }
}

That's the Template Method pattern but it's more normal to call multiple methods within the template method.

public abstract class ActivatableBehavior : IBehavior
{
    public abstract void BehaviorImplementation(IUnit destinationPlayer);

    public abstract bool CanApplyBehvaiorTo(IUnit destinationPlayer);

    public void ApplyBehavior(IUnit unit)
    {
        if (CanApplyBehvaiorTo(unit))
        {
            BehaviorImplementation(unit);
        }
    }
}

Note that I've fixed your typo (Implentation -> Implementation) and have removed the bool return type that was never used.


Working with databases in PHP I am 100% self-taught, and so I try to read as much as I can about best practices, conventions, etc. However I freelance, and have been in a long-term contract position for the past little while with almost no peers to review my code. So I get nervous about developing bad habits.

Looking closely at this function I wrote (meant to give me a quick look over the db make sure nothing else is signed that ought to be unsigned):

$db = Database::instance();
$tables = array();
$result = $db->query('SHOW TABLES');
foreach($result as $table)
{
        $create = $db->query("SHOW CREATE TABLE `".current($table)."`");
        $tables[current($create->current())] = end($create->current());
}
echo var_dump($tables);

I see a couple things - $tables and $table are similar things, but $tables is not made up of every $table, so it could be confusing.

I tend to name all query results as $result unless there is a potential naming conflict.

Concatenating a function result within a query call seems more a quality of 'fast code' than 'good code'.

Am I being too nit-picky? Does this code look like it was written by an amateur/bad programmer?

Your code looks like its good quality code. And yes, you’re right to be concerned. Most likely, after you’re gone, somebody else will (eventually) maintain, or change the code that you’ve written to date. I suggest that you comment and write the code so that a maintainer understands its purpose at first read.

For example ...

/*
 * Look quickly over the database to make sure nothing else is signed that ought to be unsigned.
 * TODO: Explain why unsigned is better than signed…
 */

$db = Database::instance();
$all_tables = array();
$result = $db->query('SHOW TABLES');
foreach($result as $unique_table)
{
   $show_created_table = $db->query("SHOW CREATE TABLE `".current($unique_table)."`");
   $all_tables[current($show_created_table->current())] = end($show_created_table->current());
}
echo var_dump($all_tables);

It's worth doing for yourself, for memory sake, if you haven't touched the code for a longtime.
It will also help the maintainer understand the intented purpose of the function. Now, if your project is large and complex, I'd recommend that you use phpdoc -- it's a good standard.


Removing numbering gaps in a series of filenames I've recently started programming and have been working through some problems such as the one below:

Write a program that finds all files with a given prefix, such as spam001.txt, spam002.txt, and so on, in a single folder and locates any gaps in the numbering (such as if there is a spam001.txt and spam003.txt but no spam002.txt). Have the program rename all the later files to close this gap.

I was able to solve with below, but I can't imagine this being the most elegant solution - just seems very convoluted and overcomplicated. I would like to build good habits and train myself to write more efficient code - any suggestions for how can this be optimized/shortened?

#! python3
# fillGap.py - finds all files with given prefix in folder
# and locates gaps in numbering and fills the gap

import os, re, shutil

# Arguments: folder, prefix
def fillGap(folder, prefix):

    # Create regex with prefix + number + extension
    prefixRegex = re.compile(r'(%s)((\d)+)(\.[a-zA-Z0-9]+)' % prefix)

    # Make sure folder path is absolute
    folder = os.path.abspath(folder)

    # Create list with all files that match regex
    fileList = []
    for item in os.listdir(folder):
        if os.path.isfile(os.path.join(folder, item)) and prefixRegex.search(item):
            fileList.append(item)

    # Look for the starting number by incrementing, starting from 0
    Number = 0
    Completed = False
    while True:
        for filename in fileList:
            mo = prefixRegex.search(filename)
            if Number == int(mo.group(2)):
                length = len(str(mo.group(2)))
                fileList.remove(filename)
                Completed = True
                break
        Number += 1
        if Completed == True:
            break

    # Increment from starting number, finding next largest file and renaming
    nextNumber = Number
    while True:
        for filename in fileList:
            mo = prefixRegex.search(filename)
            if nextNumber == int(mo.group(2)):
                # New number including prefixed zero's
                subNumber = '0' * (length - len(str((Number)))) + str(Number)
                # New filename with updated number
                newFileName = prefixRegex.sub(r'%s%s\4' % (prefix, subNumber), filename)
                shutil.move(os.path.join(folder, filename), os.path.join(folder, newFileName))
                fileList.remove(filename)
                Number += 1
        if len(fileList) == 0:
            break
        nextNumber += 1

This is actually a very tricky function to write well, especially for a beginner.

Bugs

Be careful whenever you do string interpolation: escaping is often needed.  Here, you have plopped prefix directly into your regex, without considering whether there might be characters within the prefix that might have special significance in the regex language.  Here, you need to call re.escape().

if fileList is empty, you'll get stuck in an infinite loop, since Completed will never be set to True.

You probably want to require all of the filenames to start with the prefix.  Instead, you do re.search(), which performs an search that is not anchored at the beginning of the string.  You should use either re.match() or include a ^ at the beginning of your regex.

You zero-pad the numbers so that they are all the same width, based on the width of the number in the smallest-numbered file.  If the numbers are all the same width, then that's fine.  However, if they might not all be the same width, then it makes more sense to go with the width of the largest-numbered file.

Also, it's not obvious what the right behaviour is if spam1.txt and spam001.txt both exist.  Maybe that isn't a concern for you, but I thought I should mention it.

Algorithm

Your strategy works well enough for small numbers.  I wouldn't recommend it for a files named like photo_2016022235959.jpg — it could take a while to count that high!  Sorting might be a better strategy than counting up from 0.

The nested while True: for filename in fileList: loops are cumbersome.  You can often avoid loops in Python by using list comprehensions and generator expressions.  An additional benefit of using comprehensions and generator expressions is that you can "say what you mean" instead of specifying what to do step-by-step.

There is some repetition of the string-analysis code in the two loops.  I recommend performing the regex match once and storing the match results instead of the filenames.

Implementation details

I don't see any reason for converting folder to an absolute path.

os.path.isfile(…) involves a system call, and is much more expensive than a regex test.  Therefore, you should reorder the tests to do the quick test first:

if prefixRegex.search(item) and os.path.isfile(os.path.join(folder, item)):

However, since Python 3.5, you can use os.scandir() instead of os.listdir() if you also want information about the entries such whether it is a regular file (thus rendering the previous point moot).  os.scandir() produces os.DirEntry objects as results, such that you can retrieve the entry.name of the file and test entry.is_file() efficiently.

Instead of zero-padding numbers by concatenating strings yourself…

# New number including prefixed zero's
subNumber = '0' * (length - len(str((Number)))) + str(Number)
# New filename with updated number
newFileName = prefixRegex.sub(r'%s%s\4' % (prefix, subNumber), filename)

… use str.format():

'{}{:0{width}}.{}'.format(prefix, n, extension, width=width)

That means, basically, prefix + str(n) + '.' + extension.  A :05 format spec makes the number zero-padded to five digits; writing it as :0{width} makes it take the width from the parameter named width.

Counting loops are often better written using enumerate().

PEP 8 is the official Python style guide.  The naming conventions say that you should use lower_case_with_underscores for function and variable names.  Following some other capitalization convention is allowable if you need to interoperate with existing code, but is not recommended.

Suggested solution

import os, re, shutil

def fill_gaps(folder, prefix):
    filename_re = re.compile(r'{}(\d+)\.(.*)'.format(re.escape(prefix)))
    name = lambda match: match.group(0)
    number = lambda match: int(match.group(1))
    ext = lambda match: match.group(2)

    # A list of regular expression matches for all filenames that fit the
    # pattern, sorted numerically. filter(None, ...) eliminates false values,
    # namely the failed re.match(...) calls that returned None.
    matches = sorted(filter(None, (
        filename_re.match(entry.name)
        for entry in os.scandir(folder)
        if entry.is_file()
    )), key=number)

    if not matches: return

    first_num = number(matches[0])
    width = max(len(match.group(1)) for match in matches)

    for n, match in enumerate(matches, first_num):
        old_name = os.path.join(folder, name(match))
        new_name = os.path.join(
            folder,
            '{}{:0{width}}.{}'.format(prefix, n, ext(match), width=width)
        )
        if new_name != old_name:
            if os.path.exists(new_name):
                raise FileExistsError(new_name)
            # print('mv {} {}'.format(old_name, new_name))
            shutil.move(old_name, new_name)

I'd rather have the operation fail than risk overwriting an existing file, so I've added the test if os.path.exists(new_name): raise FileExistsError(new_name).


DRY input-checking in python Let's say I want to write a super simple Python program to ask a user for their personal information and list it back out in a "pretty" format.
def get_info():
    first = input("enter your first name: ")
    while not first.isalpha():
        first = input("enter your first name: ")

    last = input("enter your last name: ")
    while not last.isalpha():
        last = input("enter your last name: ")

    age = input("enter your age: ")
    while not age.isnumeric():
        age = input("enter your age: ")
    age = int(age)

    has_pet = input("do you have a pet?: ").lower()
    while has_pet not in ["yes", "no", "y", "n"]:
        has_pet = input("do you have a pet?: ").lower()

    if "y" in has_pet:
        has_pet = True
    else:
        has_pet = False

    return [first, last, age, has_pet]

def print_info(info):
    first, last, age, has_pet = info
    print(f"Name: {first} {last}")
    print(f"Age: {age}")
    print(f"Has pet: {has_pet}")

print_info(get_info())

Clearly, I have violated DRY multiple times.
(1) Should I just throw the input() functions away and use arguments (with sys.argv)?
(2) The reason I can't use recursion is because I would have to ask the user for all of their information again if they mess up on, like, answering the pet question for example. Is there another way to clean up this code without sacrificing speed / resources?

Yes, abandon input(), which is usually a less powerful and flexible choice.
Here's an approximation of your code using
argparse. Notice several
things: (1) very little repetition in our code; (2) much less algorithmic code,
which means fewer chances for error; (3) greater simplicity overall
and thus higher readability;
(4) good validation and user help messages out of the box; (5) many other
possibilities (see the library's documentation); (6) less hassle for
users to run the code repeatedly or in any kind of automated context;
and (7) less hassle during development of the code, for the same reason.
# Usage examples:

$ python demo.py George Washington 289 --pet
Namespace(age=289, first_name='George', last_name='Washington', pet=True)

# The code:

import argparse
import sys

def main(args):
    ap, opts = parse_args(args)
    print(opts)

def parse_args(args):
    ap = argparse.ArgumentParser()
    ap.add_argument('first_name', type = personal_name)
    ap.add_argument('last_name', type = personal_name)
    ap.add_argument('age', type = int)
    ap.add_argument('--pet', action = 'store_true')
    opts = ap.parse_args(args)
    return (ap, opts)

def personal_name(x):
    if x.isalpha():
        return x
    else:
        raise ValueError(f'Invalid name: {x}')

if __name__ == '__main__':
    main(sys.argv[1:])

But if you cannot or do not want to take that route, the roll-your-own approach
looks similar in spirit: we need a general-purpose function to get input;
it needs to take some type of text or label to tell the user what we are asking
for; and it needs a converter/validator to check the reply.
Here's a rough sketch of the new or different parts:
def main(args):
    d = dict(
        first = get_input(label = 'first name', convert = personal_name),
        last = get_input(label = 'last name', convert = personal_name),
        age = get_input(label = 'age', convert = int),
        pet = get_input(label = 'pet status [y/n]', convert = yesno),
    )
    print(d)

def yesno(x):
    if x in ('yes', 'y'):
        return True
    elif x in ('no', 'n'):
        return False
    else:
        raise ValueError(f'Invalid yes-no: {x}')

def get_input(label, convert):
    while True:
        reply = input(f'Enter {label}: ')
        try:
            return convert(reply)
        except (ValueError, TypeError):
            print('Invalid reply')


Playing sound in JavaFX when button is pressed, window pops up So I made a class for playing a sound in JavaFX when a button is pressed, a window pops up... I don't know about the quality of the code though. I don't know if I used the best way to play the sound. It's meant to be used to play small mp3 or wav files (not longer than 5 seconds) I made it so you can adjust the volume of it (i.e. an option menu where you can adjust your notification sound menu)

Here's the code:

public class Sound {

String file;
Media sound;
MediaPlayer player;

public Sound(String file) {
    this.file = file;
    this.sound = new Media(new File(file).toURI().toString());
    this.player = new MediaPlayer(sound);
}

public void play(){
    player.play();
}

public void stop(){
    player.stop();
}

public void setVolume(double value){
    player.setVolume(value);
}

public double getVolume(){
    return player.getVolume();
}
}

//A little GUI with a button that plays a sound and a volume slider for that sound

public class MusicPane extends Pane {

String path;
Sound sound;
public MusicPane(String path) {

    this.path = path;
    this.sound  = new Sound(path);

    //if this button is pressed, it plays the sound
    Button button = new Button("play");
    button.setOnAction(new EventHandler<ActionEvent>() {
        @Override
        public void handle(ActionEvent e) {
            //first stop a playing sound, then start the sound
            sound.stop();
            sound.play();
        }
    });

    //Slider to adjust the volume
    Slider slider = new Slider();
    slider.setMin(0);
    slider.setMax(1.0);
    slider.setValue(sound.getVolume());

    slider.valueProperty().addListener(new InvalidationListener() {
        @Override
        public void invalidated(Observable observable) {
            sound.setVolume(slider.getValue());
        }
    });
    this.getChildren().addAll(button, slider);
}
}

It's generally more standard to hide as many fields of your class as possible. I'd recommend making  file, sound and player in Sound as well as path and sound in MusicPane private.

Additionally you're not using file in Sound, so you can remove it. Furthermore your class' bodies are not indented by one level (contrary to usual conventions).

Aside from that there's not much to say about Sound the only thing that may be improved is using Paths.get instead of new File when you create your sound.

The MusicPane has a bit more to talk about. For one I don't quite understand why you're not using an fxml to specify the components. Additionally conventional wisdom suggests to use composition over inheritance. Instead of subclassing Pane into a MusicPane, you should use the Pane as a Member of the "GUI" you expose to your program.

This allows you to better share behaviour between components you create by subclassing Components instead of Pane.

The comments in the code aren't really useful... // if this button is pressed, it plays the sound can be completely removed, especially when you rename the button to play.
//first stop a playing sound, then start the sound  can be completely removed. It adds no information whatsoever to the code.
//Slider to adjust the volume becomes completely superfluous as soon as you rename slider to volumeSlider.


Format potentially invalid XML I'm working on an application that allows users edit/fix XML.
A part of this is to format the XML for better readability.
As the XML might be invalid, the existing methods I found for formatting (like XmlWriter or XDocument) don't work for me.
There might be all sorts of problems with the XML, although the most common is unescaped special characters.
public static string FormatXml(string xml)
{
    var tags = xml
        .Split('<')
        .Select(tag => tag.TrimEnd().EndsWith(">") ? tag.TrimEnd() : tag); //Trim whitespace between tags, but not at the end of values

    var previousTag = tags.First(); //Preserve content before the first tag, e.g. if the initial < is missing
    var formattedXml = new StringBuilder(previousTag);
    var indention = 0;

    foreach (var tag in tags.Skip(1))
    {
        if (previousTag.EndsWith(">"))
        {
            formattedXml.AppendLine();
            if (tag.StartsWith("/"))
            {
                indention = Math.Max(indention - 1, 0);
                formattedXml.Append(new string('\t', indention));
            }
            else
            {
                formattedXml.Append(new string('\t', indention));
                if (!tag.EndsWith("/>"))
                {
                    indention++;
                }
            }
        }
        else
        {
            indention = Math.Max(indention - 1, 0);
        }

        formattedXml.Append("<");
        formattedXml.Append(tag);
        previousTag = tag;
    }

    return formattedXml.ToString();
}

Sofar the method produces reasonable output for all cases I came up with.
I'm mostly worried that I missed some special cases of valid XML that would get messed up.

There's a test suite of 2000 test cases available at https://www.w3.org/XML/Test/ - try it out.
From a quick glance, it's not clear to me how you're handling content within comments or CDATA sections - which might be well-formed XML, or it might be something approximating to well-formed XML.
Another comment is that messing with whitespace is dangerous in mixed content. With inline markup (bold, italic etc) preserving whitespace as written may be important.


Locating people by roleId Basically, I'm trying to find all active people with a roleId of 3 (analyst id). But I want to exclude ids that were created as test analysts, i.e. "130", "252", etc. I see that I'm repeating the part to exclude the test ids. Also, I'm repeating the part about the roleId. Isn't there a way to optimize it? My query works fine, but I'm trying to optimize to make it more DRY.

Here's what my code looks like (I've labeled where I think the code can be optimized):

from people in xDoc.Descendants("People")
where (int?) Convert.ToInt16(people.Element("active").Value) == 1 &&
    (int?)Convert.ToInt16(people.Element("id").Value) != 130 &&  // <-- to optimize
    (int?)Convert.ToInt16(people.Element("id").Value) != 252 &&  // <-- to optimize
    (int?)Convert.ToInt16(people.Element("id").Value) != 264 &&  // <-- to optimize
    (int?)Convert.ToInt16(people.Element("id").Value) != 302 &&  // <-- to optimize
    people.Descendants("roleIds").Descendants("int")
        .Select(p => (int?)Convert.ToInt16(p.Value)).FirstOrDefault() == 3  // <-- to optimize
select new PeopleClass
{
    ResAnId = (int) Convert.ToInt16(people.Element("id").Value),
    DpInit = (string)people.Element("clientCode").Value,
    ResAnFName = (string)people.Element("firstName").Value,
    ResAnLName = (string)people.Element("lastName").Value,
    ResAnName = String.Format("{0} {1}", (string)people.Element("firstName").Value,
        (string)people.Element("lastName").Value),
    RoleId = people.Descendants("roleIds").Descendants("int")
        .Select(p => (int?) Convert.ToInt16(p.Value)).FirstOrDefault(),  // <-- to optimize
    DivisionId = (int?) Convert.ToInt16(people.Element("divisionId").Value)
}).ToList();

If you're interested, here's my XML feed:

<People>
  <id>269</id>
  <dummy>false</dummy>
  <firstName>Steve</firstName>
  <lastName>Jobs</lastName>
  <initial></initial>
  <title>CEO</title>
  <active>1</active>
  <analystPosition></analystPosition>
  <clientCode>SJ</clientCode>
  <clientCode2></clientCode2>
  <divisionId>2</divisionId>
  <updatedBy>245</updatedBy>
  <associates>
    <Associate>
      <analystId>269</analystId>
      <associateId>257</associateId>
      <rank>0</rank>
    </Associate>
  </associates>
  <roleIds>
    <int>3</int>
    <int>4</int>
    <int>13</int>
  </roleIds>
</People>

You can introduce range variables to hold data which you need several times in your query:

var analystRoleId = 3;
var testAnalystIds = new[] { 130, 252, 264, 302 };

var activeAnalysts =
    from p in xdoc.Descendants("People")
    let id = (int)p.Element("id")
    let isActive = (int)p.Element("active") == 1
    let isAnalyst = p.Descendants("roleIds").Elements().Any(e => (int)e == analystRoleId)
    let firstName = (string)p.Element("firstName")
    let lastName = (string)p.Element("lastName")
    where isActive && isAnalyst && !testAnalystIds.Contains(id)
    select new PeopleClass {
        ResAnId = id,
        DpInit = (string)p.Element("clientCode"),
        ResAnFName = firstName,
        ResAnLName = lastName,
        ResAnName = $"{firstName} {lastName}",
        RoleId = analystRoleId,
        DivisionId = (int?)p.Element("divisionId")
    };

Also

You can use interpolated strings instead of String.Format
I believe id always should present in your file and it will not be null. There is no need to cast this value to nullable
I believe you should check all role ids for verifying whether person is analyst. But you can use FirstOrDeafult as well
You don't need to query for roleId second time - if person matched where clause then it's analyst and you know his role id
You don't need to access Value property of XElement. It's better to use explicit cast operator. Otherwise you can get NullReferenceExceptions
If you can change implementation of PeopleClass then start with renaming it to Person and give more descriptive names to its fields. You can also remove backing storage from ResAndName filed and make it calculated on the fly.


Multidimensional Array indexed by integer array I am in need of a multidimensional array that can be initialized easily with arbitrary dimensions. I have found this code here, which is beautiful, however poses some difficulties for me. its hard to initialize with arbitrary size and also difficult to access since I have to input each index using a comma to seperate it.

In my application each array 'slot' has the same size. Furthermore it would be easy to work with, if I could index and access everything using an array, instead of having to input every index seperated by a comma. So I wrote the following code which provides something like that, however feels very heavy due to only using one large array. I am basically shifting the 'multidimensional' array layers into one huge array... abusing high valued indices. Is there a way to approve apon the code, making it faster, and using less memory and high value calculations?

#include <iostream>
#include <cmath>
#include <array>

using namespace std;

template <class T, size_t depth, size_t each_size>
class MultiArray
{
    array<T, static_cast<size_t>( pow(each_size, depth) )> multiArray;

public:
    MultiArray()
    { ; }

    void put(array<size_t, depth> indices, T thing_to_put)
    {
        size_t index = 0;
        int dimension_counter = 0;
        for (const size_t& i : indices)
        {
                index += i * nearbyint(pow(each_size, dimension_counter));
                dimension_counter++;
        }

        this->multiArray[index] = thing_to_put;
    }

    void get(array<size_t, depth> indices, T &thing_to_get)
    {
        size_t index = 0;
        int dimension_counter = 0;
        for (const size_t& i : indices)
        {
                index += i * nearbyint(pow(each_size, dimension_counter));
                dimension_counter++;
        }
        thing_to_get = this->multiArray[index];
    }
};

if you would like you can find a slightly longer version of the code above with some debugging, simple testing and very basic error handling here.

EDIT: I would like to find a very fast and efficiant way to do things! I would be happy about input on how to get a fast multidimensional array class :) Thank you all for your help!!!

You don't need to call pow() and calculate powers "from scratch" within each loop iteration - it's probably cheaper to multiply the value calculated during previous iteration as you are calculating consecutive powers of each_size:

size_t temp = 1;
for (const size_t& i : indices)
{
    temp *= each_size;
    index += i * temp;
}

Maybe also come up with a better name for this temp variable than I did ;p

std::nearbyint() is also redunant since you are calculating integral powers of an integer, so your result is going to be an integer by definition.

Apart from this, if your array is really huge, consider using some other container that doesn't store data contiguously as with the fragmented memory there is a risk of not being able to allocate space for such a large object. std::deque might fit your needs, but it's certainly less efficient, so you have to weigh pros and cons. Another advantage would be dynamic size that adds more flexibility to the way your array is used (you can also get it with std::vector, but it's also a contiguous container).

Last but not least, there is no advantage to passing primitive types such as size_t by reference as the associated pointer that's copied is of similar size to the variable that would be copied by value.


Validating a date with C# and ASP.NET Core 3.1 I have a simple class to ensure the date entered into a form is a valid date. Where can I improve it, have I missed anything?
public class ValidDate : ValidationAttribute
{
    protected override ValidationResult IsValid(object value, ValidationContext validationContext)
    {
        var dateToParse = value.ToString();
        var parsedDate = new DateTime();

        if (DateTime.TryParseExact(dateToParse, "dd/MM/yyyy", System.Globalization.CultureInfo.InvariantCulture,
            System.Globalization.DateTimeStyles.None, out parsedDate))
            return ValidationResult.Success;

        return new ValidationResult("Invalid date, please try again with a valid date in the format of DD/MM/YYYY.");
    }
}

Thanks,

You could easily turn your method into a one-liner expression-bodied method with a few tricks:

use the _ discard variable instead of the explicit parsedDate variable (also, it doesn't need initializing to new DateTime()).
use the conditional operator since you're returning one of two ValidationResults.
value.ToString() can be passed directly into TryParseExact without the extra local variable dateToParse.

Giving:
protected override ValidationResult IsValid(object value, ValidationContext validationContext) =>
    DateTime.TryParseExact(value.ToString(), "dd/MM/yyyy", System.Globalization.CultureInfo.InvariantCulture, System.Globalization.DateTimeStyles.None, out _)
        ? ValidationResult.Success
        : new ValidationResult("Invalid date, please try again with a valid date in the format of DD/MM/YYYY.");

Couple more items:

Recommend -- for ease of reading -- adding a using System.Globalization; to your namespace so you don't have to type that long prefix out twice.
As this is an attribute, add the suffix Attribute to your class name.


Mad Libs using recursive IO for user input I'm writing my first small programs in Haskell and still getting a feel for the syntax and idioms. I've written this Mad Libs implementation using recursive IO. I've used IO actions throughout and I'm sure there must be a better way of splitting up this code to separate pure functions from IO actions. Also, I'm not happy with the printf statement, but I couldn't find a native way to apply an arbitrary number of list items to printf.

import Text.Printf

getAnswer :: String -> IO String
getAnswer question = do
    putStrLn question
    answer <- getLine
    return answer

getAnswers :: [String] -> [String] -> IO [String]
getAnswers [] ys = return ys
getAnswers (x:xs) ys = do
    answer <- getAnswer x
    let answers = ys ++ [answer]
    getAnswers xs answers

main = do
    let questions = ["Enter a noun:", "Enter a verb:", "Enter an adjective:", "Enter an adverb:"]
    let madlib = "Your %s is %s up a %s mountain %s."
    answers <- getAnswers questions []
    printf madlib (answers!!0) (answers!!1) (answers!!2) (answers!!3)
    putStrLn ""

Can we make getAnswer simpler or out of IO? Well, not really. You want to ask the use a question, and you want to get an answer. So all we could do is to reduce the amount of unnecessary code:

getAnswer :: String -> IO String
getAnswer question = putStrLn question >> getLine
-- or
--                 = do
--      putStrLn question
--      getLine

However, getAnswers can be refactored quite heavily. First of all, its interface isn't really developer-friendly. What are the questions? What are the answers? We should probably hide that in the bowels of our function:

getAnswers :: [String] -> IO [String]
getAnswers xs = go xs []
  where go [] ys = return ys
        go (x:xs) ys = do
          answer <- getAnswer x
          let answers = ys ++ [answer]
          go xs answers

But ++ [...] isn't really best-practice. Instead, you would ask all other questions and then combine them:

  where go [] = return []
        go (x:xs) = do
          answer <- getAnswer x
          otherAnswers <- getAnswers x
          return (answer : otherAnswers)

But at that point, we're merily copying mapM's functionailty. Therefore, your getAnswers should be

getAnswers :: [String] -> IO [String]
getAnswers = mapM getAnswer

A lot simpler.

Now for your main. If you don't know how many words you'll get you will need a list, correct. But lets check the structure of your result:

 "Your %s is %s up a %s mountain %s."
       1     2       3           4

There is a pattern. We have our text, then whatever the user gave us, then again our text, and so on. Let's split that into fragments:

["Your ","%s"," is ","%s"," up a ","%s"," mountain ","%s","."]
--       ^^^^        ^^^^          ^^^^              ^^^^

This brings up the following idea: if you have a list of your answers, you only need the list of the other words, right?

["Your "," is "," up a "," mountain ","."]

And then we need to "zip" that list with yours:

interleave :: [a] -> [a] -> [a]
interleave (x:xs) (y:ys) = x : y : interleave xs ys
interleave xs     _      = xs

We end up with the following main:

main = do
    let questions = ["Enter a noun:", "Enter a verb:", "Enter an adjective:", "Enter an adverb:"]
    let madlib = ["Your "," is "," up a "," mountain ","."]
    answers <- getAnswers questions

    putStrLn $ interleave madlib questions

Here's all the code at once:

getAnswer :: String -> IO String
getAnswer q = putStrLn q >> getLine

getAnswers :: [String] -> IO [String]
getAnswers = mapM getAnswer

interleave :: [a] -> [a] -> [a]
interleave (x:xs) (y:ys) = x : y : interleave xs ys
interleave xs     _      = xs

main :: IO ()
main = do
    let questions = ["Enter a noun:", "Enter a verb:", "Enter an adjective:", "Enter an adverb:"]
    let madlib = ["Your "," is "," up a "," mountain ","."]
    answers <- getAnswers questions

    putStrLn $ interleave madlib questions

Exercises

The interleave function above is left-biased. Why? Could this pose problems for your program? Why not?


A simple wrapper around fetch I am writing a simple wrapper around fetch.
async function apiCall(
  endpoint: string,
  {
    data,
    headers: customHeaders,
    ...customConfig
  }: { data?: Object; headers?: Object } = {}
) {
  const config = {
    method: data ? 'POST' : 'GET',
    body: data ? JSON.stringify(data) : undefined,
    headers: {
      'content-type': data ? 'application/json' : undefined,
      ...customHeaders,
    },
    ...customConfig,
  }

  return fetch(endpoint, config as any).then(async (response) => {
    if (response.ok) {
      const json = await response.json() //
      return json
    } else {
    //   what if `response` contains error messages in json format?
      return Promise.reject(new Error('Unknown Error'))
    }
  })
}

It works fine. The problem is with this snippet:
 return fetch(endpoint, config as any).then(async (response) => {
    if (response.ok) {
      const json = await response.json()
      return json
    } else {
    //   what if `response` contains error messages in json format?
      return Promise.reject(new Error('Unknown Error'))
    }
  })

If response is not ok, it rejects with a generic Error. This is because by default, window.fetch will only reject a promise if the actual network request failed. But the issue is that, even if response is not ok, it might still be able to have error messages in json format. This depends on the backend implementation details but sometimes you are able to get the error messages in the response body by response.json(). Now this use case is not covered in the wrapper I built.
So I wonder how I am going to be able to account for that? I guess you can do something like
fetch(endpoint, config as any).then(async (response) => {
      if (response.ok) {
        const json = await response.json()
        return json
      } else {
          try {
            const json = await response.json()
            return Promise.reject(json)
          } catch {
            return Promise.reject(new Error('Unknown Error'))
          }
      }
    })

I wonder if there is some more elegant way to do that?
Lastly, I am very aware of libraries like Axios. I built this partly to satisfy my intellectual curiosity.
By the way, a lightly unrelated question but I wonder if the following snippets are equivalent?
 if (response.ok) {
        const json = await response.json()
        return json
      }

 if (response.ok) {
        return response.json()
      }

If you need to account for mixed response types (in your case "unknown errors" or JSON-formatted errors), then you could consider checking the Content-Type of the response. See this StackOverflow answer for reference. This, of course, trusts in the server echoing the correct content type header for a response.
As you mentioned, fetch does not reject on HTTP error status, instead response.ok will be set to false. You can use this to decide if you want to reject or resolve your promise.
/* ... */.then(async response => {
    const contentType = response.headers.get("content-type");
    if (contentType && contentType.indexOf("application/json") !== -1) {
        const json = await response.json();
        return response.ok
            ? Promise.resolve(json)
            : Promise.reject(json);
    } else {
        const text = await response.text();
        return response.ok
            ? Promise.resolve(text)
            : Promise.reject(new Error("Unknown Error!"));
    }
});

You could also (which is what the post linked above suggests aswell) use a try-catch block, if you do not trust the response headers.
/* ... */.then(async response => {
    const text = await response.text();
    try {
        const data = JSON.parse(text);

        // If the line above has not produced an error, the response contained valid JSON.
        // We can either choose to just return the resulting data, or, as in
        // example above, resolve/reject the Promise based on the status code.

        return response.ok
            ? Promise.resolve(data);
            : Promise.reject(data);
    }
    catch(err) {
        // This means that the response probably contained text. As above, you can
        // decide if this means an error has occurred, or if the result will just
        // be handled differently.

        return response.ok
            ? Promise.resolve(text);
            : Promise.reject(new Error("Unknown Error!"));
    }
});

As for your additional question on the difference between:
if (response.ok) {
    const json = await response.json();
    return json;
}
// ... and ...
if (response.ok) {
    return response.json();
}

This essentially boils down to the difference between:
return await response.json();
// ... and ...
return response.json();

The difference lies within the way that JavaScript handles async/await methods. The first line returns a generic T, while the second one returns Promise<T>. What this means is that the first one "delays" the execution of the return statement until the promise returned by response.json() is resolved, while the second one says "I don't care that execution is continued, whoever calls me later will have to wait for my value to be resolved.", which is what happens when you perform .then(...) on a promise.
Note that the first method still ultimately returns a Promise<T> though, since async methods are essentially just fancy wrappers for promise generations.


Sieve of Eratosthenes using Java 8 streams I am learning streams and lambdas in Java 8.

I wanted to implement the classic Sieve of Eratosthenes using lambdas and streams. This is my implementation.

 /**
 * @author Sanjay
 */
class Sieve {
    /**
     * Implementation of Sieve of eratosthenes algorithm using streams and lambdas
     * It's much slower than the version with traditional for loops
     * It consumes much memory than the version with traditional for loops
     * It computes all primes upto 100_000_000 in 2 secs (approx)
     */
    public static List<Integer> sieveOfEratosthenes(int _n) {
        // if _n is empty return an empty list
        if (_n <= 1) return Arrays.asList();
        // create a boolean array each index representing index number itself
        boolean[] prime = new boolean[_n + 1];

        // set all the values in prime as true
        IntStream.rangeClosed(0, _n).forEach(x -> prime[x] = true);
        // make all composite numbers false in prime array
        IntStream.rangeClosed(2, (int) Math.sqrt(_n))
                .filter(x -> prime[x])
                .forEach(x -> unsetFactors(x, prime, _n));
        // create a list containing primes upto _n
        List<Integer> primeList = new ArrayList<>((_n < 20) ? _n : (_n < 100) ? 30 : _n / 3);
        // add all the indexes that represent true in prime array to primeList
        IntStream.rangeClosed(2, _n).filter(x -> prime[x]).forEach(primeList::add);
        // return prime list
        return primeList;
    }

    /*
     * makes all the factors of x in prime array to false
     * as primes don't have any factors
     * here x is a prime and this makes all factors of x as false
     */
    private static void unsetFactors(int x, boolean[] prime, int _n) {
        IntStream.iterate(x * 2, factor -> factor + x)
                .limit(_n / x - 1)
                .forEach(factor -> prime[factor] = false);
    }
}

What is its efficiency compared to normal for-loops?
Are there any imporvements to be made?

Implementation

You can invert the values for prime and not prime to avoid the initial rangeClosed(0, _n).forEach(x -> prime[x] = true). The innermost loop can start at x*x instead of x*2:

// (1)
public static List<Integer> sieveOfEratosthenes(int n) {
    boolean[] notPrime = new boolean[n + 1];

    IntStream.rangeClosed(2, (int) Math.sqrt(n))
            .filter(x -> !notPrime[x])
            .forEach(x -> {
                IntStream.iterate(x * x, m -> m <= n, m -> m + x)
                        .forEach(y -> notPrime[y] = true);
            });

    List<Integer> list = new ArrayList<>();
    IntStream.rangeClosed(2, n)
            .filter(x -> !notPrime[x])
            .forEach(x -> list.add(x));
    return list;
}

This implementation still feels like a literal translation of the for-loop approach. Streams offer options to improve readability, i.e. the inner loop can be flattened:

// (2)
public static List<Integer> sieveOfEratosthenes(int n) {
    boolean[] notPrime = new boolean[n + 1];

    rangeClosed(2, (int) Math.sqrt(n))
            .filter(x -> !notPrime[x])
            .flatMap(x -> iterate(x * x, m -> m <= n, m -> m + x))
            .forEach(x -> notPrime[x] = true);

    return rangeClosed(2, n)
            .filter(x -> !notPrime[x])
            .boxed().collect(toList());
}

Stream vs Loop

The main problem I see with a stream based approach is (besides the overhead of the streams), that it is more difficult to optimize compared to a loop approach as the abstraction level is higher. For example converting the implementation to a parallel approach is in my opinion way more complicated with a stream approach.

Algorithm improvements (not directly related to the question)

You can use a BitSet (or directly a long/int/byte array) to ensure that each entry in the sieve consumes only one bit (instead of currently (likely) 8). You are storing all values from 2 to n in the sieve, you can save memory by skipping multiples of 2 (and 3, 5, ...) at the cost of some additional calculations to convert between sieve position and value.

For larger values of n it might be advantageous to divide the sieving process in smaller steps and use a small array instead of a large array for all values and sieving the complete range at once.

The sieving process can be converted to a parallel implementation with nearly linear speedup as two or more values can be sieved simultaneously (sieving process of each value is independent from other values).

Approx. performance for n=100_000_000, included my implementation (which utilizes most of the improvements mentioned above) as reference value:

// 1 Thread
Initial            3406676978ns
Version (1)        2487619662ns
Version (2)        2459434320ns
For loop           2158261344ns
Nevay to List      1436579484ns
Nevay to Sieve     1040829243ns
// 8 Threads
Nevay to List       682085628ns
Nevay to Sieve      172952467ns


Two code snippets involving time-change I have two snippets that are going to be used in a walkthrough and I'd like some feedback on which one is more easily understandable

1

    property int hours, minutes, seconds
    property real shift: 0.0
    property bool night

    function timeChanged() {
        var date = new Date();
        hours = date.getUTCHours() + Math.floor(clock.shift);
        minutes = date.getUTCMinutes() + (clock.shift % 1) * 60;
        seconds = date.getUTCSeconds();
        night = (hours < 7 || hours > 19);
    }

2

    property int hours, minutes, seconds
    property real shift
    property bool night

    function timeChanged() {
        var date = new Date;
        hours = shift ? date.getUTCHours() + Math.floor(clock.shift) : date.getHours()
        night = ( hours < 7 || hours > 19 );
        minutes = shift ? date.getUTCMinutes() + ((clock.shift % 1) * 60) : date.getMinutes()
        seconds = date.getUTCSeconds();
    }

I personally feel that instantiating the shift variable and skipping the ?: operator altogether is a better approach.

Note that both these snippets are aimed at a tutorial for people who are new to QML, and therefore I am charged with picking the one that seems simplest and clearest to people who don't know a lot about QML.

I definitely think sample #1 is more readable. The main reason is the (lack of) ternary operator. Although any programmer should understand it, it increases the amount of code you have to read. With sample #1 you can quickly skip the parts after + in the hours and minutes assignments, whereas with ? : you have to read more carefully.

Also, in sample #2 you basically add handling of a special case without needing it, adding unneeded complexity.

Another bonus in sample #1 is that night is not defined in the middle of the hours/minutes/seconds block, so the code is more logically grouped.

By the way, I think the use of shift should be documented with a comment.


wrapper class for random access iterator to hide template method implementation from header file I have a set of template functions that have itrator types as template parameters, so can be used with any kind of iterators. std::find can be a good example.
Also I have a class that uses this function in its implementation and I don't want to expose my algorithms' header. Regarding the template function, inside my class I expect the iterators to be of random access type. So I wrote a wrapper class which can be constructed from any iterator Type that has the same pointer type and std::random_access_iterator_tag
template <typename IterR, typename IterL>
using iter_same_value_type = std::is_same<
        typename std::iterator_traits<IterR>::value_type,
        typename std::iterator_traits<IterL>::value_type>;

template <typename IterR, typename IterL>
using iter_same_category = std::is_same<
        typename std::iterator_traits<IterR>::iterator_category,
        typename std::iterator_traits<IterL>::iterator_category>;

template <typename T>
class WrapperRandomAccessIterator
{
    using this_type = WrapperRandomAccessIterator<T>;
public:

    using difference_type = std::ptrdiff_t;
    using value_type = T;
    using pointer = T*;
    using reference = T&;
    using iterator_category = std::random_access_iterator_tag;

    template <typename RandomAccessIter, typename  = typename
            std::enable_if<iter_same_category<this_type, RandomAccessIter>() &&
                    iter_same_value_type<this_type, RandomAccessIter>()>::type>
    constexpr explicit WrapperRandomAccessIterator(RandomAccessIter iter)
        : ptr_(&*iter)
    {}

    reference operator*()
    {
        return *ptr_;
    }

    // Warning says "const has no effect on type reference"
    const T& /*reference*/ operator*() const
    {
        return *ptr_;
    }

    const this_type& operator++()
    {
        ptr_ = std::next(ptr_);
        return *this;
    }

    this_type operator++(int)&
    {
        auto res = *this;
        ++(*this);
        return res;
    }

private:
    T *ptr_;
};

Here I have clang-tidy complaining about operator++(int)& that it returns non-const object.
It also says that for const reference operator*() that `const qualifier have no effect.

How can it be improved?
Do I have to use the rule of 3 or 5 in such case?
Does it make sense to create a generic iterator wrapper class and a specialization for each iterator type? (I know, this question probably belongs with engineering stack exchange).

I’m going to answer your questions first, then get into the actual code review.
Questions
How can “it” [the code/the design] be improved?
I’ll get into details in the review, but the broad answer to this question is: I don’t know.
The reason I don’t know is two-fold:

you don’t provide even a single comment to explain the logic of what this code is doing; and
you don’t provide even a single example to show what the intended usage is.

Without any clue about what the code is supposed to be doing or how it’s supposed to be used, how can anyone answer the question of whether it can be improved? Maybe it’s absolute perfection; maybe there is no possible way to make this code any better for what it was written for. Or maybe it’s the absolute worst possible solution for what it’s for. All that depends on what it’s for.
Obviously I’m going to try to review the code anyway, but to do so I’ll have to make a lot of assumptions about what you mean in your very brief, wildly unclear description of what the purpose of this wrapper is. I may guess wrong—that’s very likely, in fact—which means a lot of what I say may be incorrect, or even gibberish. For that I apologize, but it can’t be helped; I have to work with what I have.
So I guess one way to answer the question is: documentation… lots of documentation, and some example usage code. For something this small, there’s really no reason one couldn’t provide a complete, compile-able test program (even if you want to keep that algorithm secret; you could just include a stub or dummy implementation just for the purposes of illustration and testing).
Do I have to use the rule of 3 or 5 in such case?
Yes. You always✻ have to. That’s why they call it a “rule”.
✻ (Okay, technically the rule says “almost always”. But that just means that unless you’re 100% sure you don’t need to follow the rule, then the answer is yes.)
The only question is which rule: 3, 5, or 0?
Unfortunately, I can’t really give a coherent answer to that without making some guesses. So… just looking at what you’ve got there, and going by your description, I don’t… think… you need the rule of 3 or 5 (that is, you can use the rule of 0).
BUT…
I suspect that if the intended usage is more than what you explicitly say it is—that is, if I have to guess at practical usage of this wrapper, at least in my imagination—then the answer is that you do need to use the rule of 3 or 5. In fact, I think this wrapper is dangerously broken, and I’ll explain why in the review.
Does it make sense to create a generic iterator wrapper class and a specialization for each iterator type?
Absolutely. It’s not a bad idea at all. In fact, I wouldn’t be surprised if there were already “type-erased iterator” types out there. You could even make an argument for including one in the standard library. It would be hella useful.
Now, does it make sense to do it this way? Mm, maybe? Impossible to say for sure. For the general use case I’d say no, but for the specific use case of random-access iterators only—with some additional caveats on both the iterator and algorithm characteristics—then maybe.
So, onto the code review.
Code review
I’m going to skip the type traits, because I’ll talk about them when we get to the constructor that uses them.
template <typename T>
class WrapperRandomAccessIterator

Okay, so, here’s the first point of confusion. The type is called WrapperRandomAccessIterator… but it doesn’t look like you’re actually wrapping a random-access iterator. It looks like you’re trying to create a random-access iterator (actually, technically a contiguous iterator, but if you’re still stuck with C++11, then you don’t have that concept yet—seriously, though, who’s still using C++11 in 2020… almost 2021!) to a value sequence, and you’re trying to do so by stealing the value out from under a random-access iterator, and creating a pointer directly to it.
The point is… the name is misleading. This is not an iterator wrapper. It does not wrap an iterator. It just transforms an iterator to a pointer.
using this_type = WrapperRandomAccessIterator<T>;

As type aliases go, this is really… kinda useless. So long as you’re inside the class body, you can omit the template arguments. Sure WrapperRandomAccessIterator is a little longer than this_type… but you really shouldn’t need to repeat it so often that it becomes a problem.
using difference_type = std::ptrdiff_t;
using value_type = T;
using pointer = T*;
using reference = T&;
using iterator_category = std::random_access_iterator_tag;

These are all okay, but I would suggest defining value_type as:
using value_type = typename std::remove_cv<T>::type;

This is to bring it in line with the way people expect to use value_type. If you have a case like this…:
template <typename Iterator>
void max_value(Iterator first, Iterator last)
{
    using type = typename iterator_traits<Iterator>::value_type;

    type val = *first;

    while (++first != last)
        if (*first > val)
            val = *first;

    return val;
}

// used with:
const vector<string> vec{"string1", "string2", ... };

string v = max_value(vec.begin(), vec.end());

… you would normally expect this to work because val should be a copy of whatever *first references, and that copy is not const. (And that would be true if you’d just used auto instead of type.) But with your WrapperRandomAccessIterator, it won’t work, because value_type will be const string, not string.
(It’s correct to use plain T for pointer and reference, because that makes them const string* and const string&… which is what you want.)
template <typename RandomAccessIter, typename  = typename
        std::enable_if<iter_same_category<this_type, RandomAccessIter>() &&
                iter_same_value_type<this_type, RandomAccessIter>()>::type>
constexpr explicit WrapperRandomAccessIterator(RandomAccessIter iter)
    : ptr_(&*iter)
{}

Now, here’s where the problems start. What exactly is the purpose of iter_same_category and iter_same_value_type? Let’s look at the first one to show you what I mean:
iter_same_category<this_type, RandomAccessIter>

Now, this expands to:
std::is_same<
    typename std::iterator_traits<this_type>::iterator_category,
    typename std::iterator_traits<RandomAccessIter>::iterator_category>

And this_type is just WrapperRandomAcessIterator<T>. But why all the shenanigans going through iterator_traits to get the iterator_category of this_type? There’s no mystery; it’s iterator_category. typename std::iterator_traits<this_type>::iterator_category is always iterator_category.
So you could just do:
std::is_same<
    iterator_category,
    typename std::iterator_traits<RandomAccessIter>::iterator_category>

… and there’s no need for iter_same_category at all.
Same goes for iter_same_value_type.
That gives:
template <
    typename RandomAccessIter,
    typename = typename std::enable_if<
        std::is_same<
            iterator_category,
            typename std::iterator_traits<RandomAccessIter>::iterator_category>()
        && std::is_same<
            value_type,
            typename std::iterator_traits<RandomAccessIter>::value_type>()>::type
>
constexpr explicit WrapperRandomAccessIterator(RandomAccessIter iter)
    : ptr_(&*iter)
{}

Also, iterator_category is always going to be std::random_access_iterator_tag. That’s kinda the point of the class.
To be safe, it might be wise to use remove_cv on the RandomAccessIterator’s value_type. You shouldn’t need to, because most well-behaved containers and iterators will already have removed the const/volatile… but it pays to be safe, just in case. (I think, for example, there was a bug in the standard in C++11 that raw pointers didn’t behave properly.)
There is another nasty little bug lurking here with types that overload the address-of operator (unary operator&). To avoid that, you should use std::address_of(). Note that means you can’t be constexpr anymore in C++11, because address_of() wasn’t constexpr until C++17, but… . Can’t be helped. constexpr wasn’t really well-supported in C++11.
But even in C++11, this can be noexcept, and there’s no reason it shouldn’t be.
operator* is okay, though it too should be noexcept. And it should probably be const. You’re not changing the iterator (wrapper) with this function, or with returned value (which is a pointer to something else entirely, not the iterator). That is why the next function is superfluous.
// Warning says "const has no effect on type reference"
const T& /*reference*/ operator*() const
{
    return *ptr_;
}

Okay, to understand what this warning is telling you, I first have to break your misconception of const. const (and volatile, for that matter), does NOT apply to what’s on its RIGHT; it applies to what’s on its LEFT.
In other words, const T is WRONG. T const is correct. This is the subject of religious fights in the C++ community, but there is an objectively correct answer, and it really matters… as you’ve discovered.
Now, C++ has a silly little loophole that allows you to write const T&, and the compiler will understand that you REALLY mean T const&. Same for const T*; it gets quietly translated to T const*. So most of the time, you don’t notice that you’re using const wrong. But in this case, you’ve hit one of the cases where the mistake actually causes a problem.
reference is T&. When you const reference… or, better, when you write it CORRECTLY as reference const, that expands to T& const… NOT T const& (or const T&).
Now do you understand the warning?
You can’t have a “const reference”, or thing& const. You can have a “reference to a const (thing)”, or thing const&. The thing-being-referenced can be const, but the reference itself cannot. References are always const, because unlike pointers, you can’t change what they are referenced to.
Note that pointers CAN be const. You can have a T const * const, which is a const pointer to a const T (which is basically T const&, except it can be nullptr and you have to use * everywhere). You can have a T const* which is a non-const pointer to a const T. You can have a T* const, which is a const pointer to a non-const T (basically T&). And of course you can have T*, a non-const pointer to a non-const T.
I suggest to people I teach C++ to that they read the type backwards. So T& is a “reference to T”, and T const& is a “reference to const T”. (And T const * const is a const pointer to a const T, and so on.)
So to silence the warning, you have tried to replace reference const with T const&. But that’s wrong. Those are two completely different things. That’s similar to trying to replace a T* const with a T const*. It “works” (without a warning for pointers) only because dropping the const on the pointer doesn’t matter when you’re doing with a copy, and adding the const on the T is always allowed—you’re always allowed to add const. Assigning the former to the latter “works”… but T const* and const T* still are very different things. Assigning T& const to T const& “works” for references too; the only reason you get the warning is because T& const is gibberish.
All this, by the way, is why CONTAINERS have reference (which is usually T&) and const_reference (which is usually T const&). (And pointer and const_pointer.) If reference was just reference const, there wouldn’t be any need for const_reference.

reference is T&
reference const would be T& const (which is gibberish); and
const_reference is T const&.

Similarly, for CONTAINERS:

pointer is T*
pointer const is T* const; and
const_pointer is T const*.

ITERATORS don’t have const_reference or const_pointer because they don’t make any sense for iterators. Instead, there are iterators and const_iterators:

iterator::reference is T&
iterator::pointer is T*
const_iterator::reference is T const&; and
const_iterator::pointer is T const*.

So let’s circle back to the operator functions here:
reference operator*() const // <- I recommend both const (and noexcept)
{
    return *ptr_;
}

// Warning says "const has no effect on type reference"
const T& /*reference*/ operator*() const
{
    return *ptr_;
}

Notice now that the two functions are literally identical? That’s because you don’t need a non-const version, and because the correct return type for the const version is just reference (not reference const or T const&).
So all you need is:
reference operator*() const noexcept
{
    return *ptr_;
}

And maybe constexpr, though I don’t know if C++11’s constexpr rules will allow it. (C++17+ will, though.)
const this_type& operator++()
{
    ptr_ = std::next(ptr_);
    return *this;
}

You mentioned that clang-tidy gave a warning about const for the postfix increment operator… are you sure it wasn’t for this function, the prefix increment operator? Because returning a const reference is wrong here. Try doing ++ ++i for this type; it won’t compile. The correct form for this operator is to return a non-const reference.
As for the postfix increment operator, I don’t really see anything wrong with it.
Now, I’m honestly not sure what you think std::next() is doing here. ptr_ is a T*. std::next() for any pointer p is just ++p. All this function needs to be is:
WrappedRandomAccessIterator operator++() noexcept
{
    ++ptr_;
    return *this;
}

Alright, that’s it for the code as written. Now I’m going to go on to do a design review.
Design review
Ceci n’est pas un iterator
Okay, this first problem here is that you claim this to be a random-access iterator… but it’s not. It’s not even a bidirectional iterator. It can’t possibly be any better than a forward iterator… except it’s not even that! It’s not even a legal iterator at all!
Why? Because an iterator needs more than just operator++ (prefix and postfix) and unary operator*. At the bare minimum, the most basic kinds of iterators—input, output, and forward iterators—also require equality operators… that is, both operator== and operator!=. How can you possibly even implement the simplest algorithms, like std::for_each(), without them?
template <typename Iterator, typename Func>
void example_for_each(Iterator first, Iterator last, Func&& func)
{
    while (first != last) // won't work without operator !=
        func(*first++);
}

I’d have to check, but I’m pretty sure that the most basic iterators also need operator->, too.
A random-access operator also needs operator-- (prefix and postfix). And it needs operator+= and operator-= with difference_type. AND it needs binary operator+ and binary operator- with difference_type (both ways around). AND it ALSO needs operator- between two iterators, returning a difference_type. And I’m probably forgetting a bunch of other stuff, too. There’s actually quite a lot of operations a random-access iterator can do.
You need to add a whole lot of extra operators to this class to make it a random-access iterator.
Random-access ≠ contiguous
Okay, so, here is where I really have to guess at what the real purpose of this class is. I imagine you have some function, call it foo(), that takes a pair of iterators to a sequence—let’s assume it’s a sequence of ints. To actually think this problem concretely through without resorting to vague and nebulous hand-waving, I’m going to assume that foo() actually does something real: I’m going to assume foo() does what std::minmax_element() does.
Now the tricky thing here is that you want foo() to NOT be a function template. You want only the declaration in the header, and the definition in a source file:
// foo.hpp
auto foo(??? first, ??? last) -> std::pair<???, ???>;

// foo.cpp
auto foo(??? first, ??? last) -> std::pair<???, ???>
{
    auto result = std::pair<???, ???>{first, first};

    for (; first != last; ++first)
    {
        if (*first < *(result.first))
            result.first = first;

        if (*(result.second) == *first or *(result.second) < *first)
            result.second = first;
    }

    return result;
}

Now we need to figure out what the type ??? is. This is what I presume you intend WrapperRandomAccessIterator for. You want to do:
// foo.hpp
auto foo(WrapperRandomAccessIterator<int> first, WrapperRandomAccessIterator<int> last)
    -> std::pair<WrapperRandomAccessIterator<int>, WrapperRandomAccessIterator<int>>;

// foo.cpp
auto foo(WrapperRandomAccessIterator<int> first, WrapperRandomAccessIterator<int> last)
    -> std::pair<WrapperRandomAccessIterator<int>, WrapperRandomAccessIterator<int>>
{
    auto result =
        std::pair<WrapperRandomAccessIterator<int>, WrapperRandomAccessIterator<int>>{
            first, first};

    for (; first != last; ++first)
    {
        if (*first < *(result.first))
            result.first = first;

        if (*(result.second) == *first or *(result.second) < *first)
            result.second = first;
    }

    return result;
}

And you want to be able to call it with a vector of ints like this:
auto int_vec = std::vector<int>{};
// ... fill it with ints somehow...

auto result = foo(
    WrapperRandomAccessIterator<int>{int_vec.begin()},
    WrapperRandomAccessIterator<int>{int_vec.end()});

Something like that, right?
So the question is… will this work? Yes. Once you add operator!= to the class, then yes, this will work.
So all good, right?
No. In fact, you have some nasty bugs lurking, which could easily lead to a segfault, but just as easily not while doing catastrophic damage to your machine… almost certainly not for a machine running a modern desktop OS, of course, but who knows? That’s the problem with UB. Anything could happen.
Here’s the problem. You assume that given a pair of random-access iterators to a sequence, you can degrade them to pointers, and then treat them the same as the original iterators. For example:
auto sequence = std::vector<int>{};
// ... fill sequence with a million ints...

// You assume that this...:
auto iter_beg = sequence.begin();
auto iter_end = sequence.end();
std::for_each(iter_beg, iter_end, [](int i) { std::cout << i << ' '; });

// ... is basically the same as this...:
auto ptr_beg = &(*(sequence.begin()));
auto ptr_end = &(*(sequence.end()));
std::for_each(ptr_beg, ptr_end, [](int i) { std::cout << i << ' '; });

For a vector, you’d be right. The two cases above are functionally identical, and your WrapperRandomAccessIterator would work just fine, no problems. It would also work for std::array, std::string, and C arrays (and std::valarray, but who even uses that anymore?).
But you know what? It won’t work for std::deque.
Why not? Because you have confused the concept of “random-access iterator” with the concept of “contiguous iterator”. Not all random-access iterators are contiguous iterators:

Container
Random access?
Contiguous?

std::vector
✓
✓

std::array
✓
✓

std::string
✓
✓

C array
✓
✓

std::deque
✓
✗

Internally, a deque is an vector of pointers to arrays. This structure makes appending to the front and back fast—if there’s no space, just add another array chunk, at either the beginning or the end. It also allows for random access—element N is at position “num_elements_in_first_chunk + ((chunk_number * chunk_size) + n)”, and you just solve for chunk_number and n (which is just subtracting num_elements_in_first_chunk from N, then doing N / chunk_size to get chunk_number and N % chunk_size to get n).
But while random-access is possible in a deque, that doesn’t mean that the elements are contiguous. They are if they all happen to be in the same array chunk. But if the elements are in two different chunks, all bets are off.
So if you did the example above with a deque…:
auto sequence = std::deque<int>{};
// ... fill sequence with a million ints...

// No problems:
auto iter_beg = sequence.begin();
auto iter_end = sequence.end();
std::for_each(iter_beg, iter_end, [](int i) { std::cout << i << ' '; });

// Boom, UB, possible segfault:
auto ptr_beg = &(*(sequence.begin()));
auto ptr_end = &(*(sequence.end()));
std::for_each(ptr_beg, ptr_end, [](int i) { std::cout << i << ' '; });

So you might think that, okay, if random-iterators won’t work, you could just use contiguous iterators instead. Well, sorry. There are no contiguous iterators in C++11. (Well, there are, but the concept wasn’t standardized.) Contiguous iterators didn’t come around until C++17… but you couldn’t actually use them until C++20.
So… what does that mean?
Well, it means your WrapperRandomAccessIterator won’t actually work for all random-access iterators. Sure, it will work for most random-access iterators (if you actually implement all the required functions)… but not all. And there’s no safe way to detect which iterators will cause a crash (not until C++20).
So you have two options:

throw the whole design out; or
accept the limitations, accept that it will only work for some types and maybe add checks to make sure only those types are used, and just live with that.

HOWEVER… if you chose option 2, there is one more thing to consider…
Going contiguous? Don’t over-engineer; just use pointers
Contiguous iterators are really just pointers. Your misnamed WrapperRandomAccessIterator is actually WrapperContiguousIterator, because it only works for contiguous iterators, because it uses pointers internally.
But… here’s the thing… if you’re just going to use pointers internally… why not just use them externally, too?
Let's go back to the foo() function:
// foo.hpp
auto foo(WrapperRandomAccessIterator<int> first, WrapperRandomAccessIterator<int> last)
    -> std::pair<WrapperRandomAccessIterator<int>, WrapperRandomAccessIterator<int>>;

// foo.cpp
auto foo(WrapperRandomAccessIterator<int> first, WrapperRandomAccessIterator<int> last)
    -> std::pair<WrapperRandomAccessIterator<int>, WrapperRandomAccessIterator<int>>
{
    auto result =
        std::pair<WrapperRandomAccessIterator<int>, WrapperRandomAccessIterator<int>>{
            first, first};

    for (; first != last; ++first)
    {
        if (*first < *(result.first))
            result.first = first;

        if (*(result.second) == *first or *(result.second) < *first)
            result.second = first;
    }

    return result;
}

// Usage in other.cpp
auto int_vec = std::vector<int>{};
// ... fill it with ints somehow...

auto result = foo(
    WrapperRandomAccessIterator<int>{int_vec.begin()},
    WrapperRandomAccessIterator<int>{int_vec.end()});

Since WrapperRandomAccessIterator<int> is ultimately just an int*… why not do:
// foo.hpp
auto foo(int* first, int* last) -> std::pair<int*, int*>;

// foo.cpp
auto foo(int* first, int* last) -> std::pair<int*, int*>
{
    auto result = std::pair<int*, int*>{first, first};

    for (; first != last; ++first)
    {
        if (*first < *(result.first))
            result.first = first;

        if (*(result.second) == *first or *(result.second) < *first)
            result.second = first;
    }

    return result;
}

// Usage in other.cpp
auto int_vec = std::vector<int>{};
// ... fill it with ints somehow...

auto result = foo(&(*(int_vec.begin())), &(*(int_vec.end())));
// or:
auto result = foo(int_vec.data(), int_vec.data() + int_vec.size());

Note that if you were going this route, rather than taking a pair of pointers, a safer option might be to take a pointer and a size. It’s easy to mix up pointers, or to one pointer from one sequence and another pointer from another sequence. It’s a lot harder to screw up using a pointer and a size.
Actual type-erased iterators…?
Okay, so pointers work fine for contiguous iterators, but let’s say you really, really want to use random-access iterators. Or maybe even bidirectional or forward iterators. Can it be done? How?
Well, I’m not going to put any real thought into solving this problem right now… but if I had to just barf something up, I might do something like this:
Rather than trying to degrade iterators to pointers, I would ask what are the fundamental operations an iterator needs. A basic iterator—input, output, and forward—needs to be able to do 3 things:

dereferencing (so *i returns a possibly-const reference)
movement (so ++i advances the i); and
equality (so i == j returns true if they reference the same thing).

A bidirectional iterator only adds the ability to do “negative movement”; movement backwards via decrement.
A random-access iterator adds a TON of stuff… but it really boils down to:

really big movement (i += n or i -= n)
distance measuring (i - j); and
comparison (i < j).

“Really big movement” is just movement, as is “negative movement”. And we can join equality and comparison if we keep track of what kind of comparison we’re actually doing.
So we can boil this down to 4 operations. Assuming p and q are type-erased (std::any—I know this requires C++17, but you can always use boost::any) iterators:

deref(std::any p) handles dereferencing (returns a possibly-const reference to whatever the iterator references);
advance(std::any p, std::ptrdiff_t n) handles movement (normally n must be 1, but can be -1 for bidirectional iterators, and any number for random-access iterators);
compare(std::any p, std::any q, bool eq) returns int(i == j) if eq is true using the iterators under p and q, or 1, 0, or −1 if eq is false doing a standard 3-way compare between i and j, handling both equality and comparison; and
distance(std::any p, std::any q) handles distance measurement.

Then we define a class that holds functions:
template <typename T, typename IterCat>
class type_erased_iterator
{
private:
    std::any                                                         _iter;
    std::function<T& (std::any&)>                                    _deref;
    std::function<void (std::any&, std::ptrdiff_t)>                  _advance;
    std::function<int (std::any const&, std::any const&, bool)>      _compare;
    std::function<std::ptrdiff_t (std::any const&, std::any const&)> _distance;

public:
    using difference_type = std::ptrdiff_t;
    using value_type = typename std::decay_cv<T>::type;
    using pointer = T*;
    using reference = T&;
    using iterator_category = IterCat;

    template <typename Iterator>
    explicit type_erased_iterator(Iterator it) :
        _iter{it},
        _deref{[](std::any& p) -> T& { return *std::any_cast<Iterator>(p); }},
        _advance{[](std::any& p, std::ptrdiff_t n) { std::advance(std::any_cast<Iterator>(p), n); }},
        // you get the idea...
    {}

    template <typename Iterator>
    type_erased_iterator(
            Iterator it,
            std::function<T& (std::any&)> deref,
            std::function<void (std::any&, std::ptrdiff_t)> advance,
            std::function<int (std::any const&, std::any const&, bool)> compare,
            std::function<std::ptrdiff_t (std::any const&, std::any const&)> distance
        ) :
        _iter{it},
        _deref{std::move(deref)},
        _advance{std::move(advance)},
        _compare{std::move(compare)},
        _distance{std::move(distance)}
    {}

    // Now just implement the iterator ops in terms of the core functions.

    auto operator++() -> type_erased_iterator&
    {
        _advance(_iter, 1);
        return *this;
    }

    auto operator*() -> T&
    {
        return _deref(_iter);
    }

    // and so on...
};

// usage:
auto int_vec = std::vector<int>{};
// ... fill it with ints somehow...

auto result = foo(
    type_erased_iterator<int, std::random_access_iterator_tag>{int_vec.begin()},
    type_erased_iterator<int, std::random_access_iterator_tag>{int_vec.end()});

As you can see, it’s used pretty much exactly like the way I assumed WrapperRandomAccessIterator would be used. You could even make a helper function to make it simpler:
auto result = foo(
    make_type_erased_iterator(int_vec.begin()),
    make_type_erased_iterator(int_vec.end()));

Given that most iterators are small (often just pointers), and that the functions will probably never be closures, there shouldn’t be any dynamic allocation required. This class won’t be fast, but it will allow you to type-erase ANY iterator. Not just random-access; you can even type erase input or output iterators.
If you really just want to support random-access iterators, that eliminates much of the complexity of SFINAE-ing away illegal iterator operations (like preventing operator-- for forward iterators).
Summary
WrapperRandomAccessIterator is unfortunately named, because it is not a wrapper, and it does not work with random-access iterators.
It does work with contiguous iterators, and ONLY contiguous iterators… or at least it would if you added all the required iterator operations. However, contiguous iterators aren’t really a thing until C++17, and not a practical thing until C++20.
But if you really only want it to work with contiguous iterators, you might as well just use pointers.
If you really need other iterator categories (including actual random-access iterators), you will need a much more advanced type-erasure system.


All-core MediaWiki website upgrade script I wrote the following Bash script to upgrade All-core MediaWiki websites (no added extensions/skins or images - besides logo image).

I already tested the essence of this script and it worked for me; MediaWiki was upgraded from 1.32.0. to 1.33.0 and I can use the site regularly; yet I'd be glad for a review:

#!/bin/sh

date="$(date +%F-%T)"
db_user_name_and_db_name="SOME_IDENTICAL_NAMES" # DB user name and DB name must be the same;
war="SOME_WEB_APPLICATION_ROOT"
domain="SOME_DOMAIN" # As site's directory;
target_url="LINK_TO_LATEST_MEDIA_WIKI_ARCHIVE_DOWNLOAD"

mkdir -p "${war}/mediawiki_general_backups"
mkdir -p "${war}/mediawiki_specific_backups"

zip -r "${war}/mediawiki_general_backups/${domain}-directory-backup-${date}.zip" "${war}/${domain}"
mysqldump -u "${db_user_name_and_db_name}" -p "${db_user_name_and_db_name}" > "${war}/mediawiki_general_backups/${domain}-database-backup-${date}.sql"

files=(
    LocalSettings.php
    robots.txt
    .htaccess*
    ${domain}.png
    googlec69e044fede13fdc.html
)

cd "${war}/${domain}"
cp "${files[@]}" "${war}/mediawiki_specific_backups"

rm -rf "${war}/${domain}"
mkdir "${war}/${domain}"
wget ${target_url} -O - | tar -xzv --strip-components 1 -C ${war}/${domain}

cp -a "${war}/mediawiki_specific_backups"/* "${war}/${domain}"
cd "${war}/${domain}"/maintenance
php update.php

### Sitemap creation ###

mkdir -p "${war}/${domain}/sitemap"

php maintenance/generateSitemap.php \
--memory-limit=50M \
--fspath=/"${war}/${domain}/sitemap" \
--identifier="${domain}" \
--urlpath=/sitemap/  \
--server=https://"${domain}" \
--compress=yes

# Sitemap should be declared in robots.txt / preferred search engine's search console, or both;

Naming
Try not to use generic names like $file. Make your code read easy for others. Say $files_to_backup for example.

Check for errors when it matters
If backup is important: check that the backups succeeded:

if cp "${files[@]}"${war}/mediawiki_specific_backups";then
     rm -rf "${war}/${domain}"
     mkdir "${war}/${domain}"
     ...
     ...
 else
    # handle error condition
fi

Document the code
I prefer a comment in the script header describing what the script. If the script does useful work, document what it does so others understand what it does and how to use it. Similarly, comment anything in the code which is important and you think might require some explanation to help readers.

#!/bin/sh
# Usage: how-to-use-script
# Purpose: upgrade all core MediaWiki websites
# etc


A Sine Template Function For Boost.MultiArray in C++ This is a follow-up question for An element_wise_add Function For Boost.MultiArray in C++. Besides the basic add operation applying onto each element, I am trying to implement a sine template function which can apply std::sin() on each element. A new concept with_std_sin is created as below.
template<typename T>
concept with_std_sin = requires(T x)
{
    std::sin(x);
};

The main body of this sin template function is here. The similar recursive technique is also used in order to go through all elements.
template<class T> requires (with_std_sin<T>)
auto sin(const T& input)
{
    return std::sin(input);
}

template<class T> requires (is_multi_array<T>)
auto sin(const T& input)
{
    boost::multi_array output(input);
    for (decltype(+input.shape()[0]) i = 0; i < input.shape()[0]; i++)
    {
        output[i] = sin(input[i]);
    }
    return output;
}

The test of this sin template function:
// Create a 3D array that is 3 x 4 x 2
typedef boost::multi_array<double, 3> array_type;
typedef array_type::index index;
array_type A(boost::extents[3][4][2]);

// Assign values to the elements
int values = 1;
for (index i = 0; i != 3; ++i)
    for (index j = 0; j != 4; ++j)
        for (index k = 0; k != 2; ++k)
            A[i][j][k] = values++;

for (index i = 0; i != 3; ++i)
    for (index j = 0; j != 4; ++j)
        for (index k = 0; k != 2; ++k)
            std::cout << A[i][j][k] << std::endl;

auto test_result = sin(A);

for (index i = 0; i != 3; ++i)
    for (index j = 0; j != 4; ++j)
        for (index k = 0; k != 2; ++k)
            std::cout << test_result[i][j][k] << std::endl;

All suggestions are welcome.

Which question it is a follow-up to?
An element_wise_add Function For Boost.MultiArray in C++

What changes has been made in the code since last question?
The previous question is the implementation of applying add operation onto each element in Boost.MultiArray. The main idea in this question is to implement a sin template function for calculating trigonometric sine value of each element.

Why a new review is being asked for?
In my opinion, I am not sure is this a good idea about the implementation and the usage of the with_std_sin concept here. If there is any possible improvement, please let me know.

It looks very simple and clean. I would change the name of with_std_sin though. There are some guidelines for naming concepts that suggest that it should be named sinable. That looks a bit weird, but everything I can think of to describe "something that std::sin supports" looks weird in some way, so it is as good as any name.
However, consider that you might want to apply many other functions to the contents of a boost::multi_array as well, not just std::sin. In that case, a recursive_transform() that handles multi_arrays is the way to go, so you could "just" write:
auto test_result = recursive_transform(A, [](auto &x){return std::sin(x);});

Or at least use that to define sin(), cos() and so on in therms of recursive_transform() without having to duplicate code.


Shell script to parse variable definitions from Scala build file The build.sbt text file contains versions like this:

name := "happy"
scalaVersion := "2.11.8"
sparkVersion := "2.2.0"

I wrote a Bash script to parse out the PROJECT_NAME and SCALA_VERSION from the build.sbt file:

PROJECT_NAME=$(cat build.sbt | grep "name :=" | cut -f 3 -d " " | tr -d '"')
SCALA_VERSION=$(cat build.sbt | grep "scalaVersion :=" | cut -f 3 -d " " | tr -d '"')

How can I write this more elegantly / robustly?  I'm ok with an awk or sed approach, but don't want to add an external dependency to the script.

Here's some more of the script to see how the variables are being used.

if [ "$SCALA_VERSION" = "" ]
  then
    echo "SCALA_VERSION variable cannot be empty"
    exit 1
fi

SCALA_BINARY_VERSION=${SCALA_VERSION%.*}
if [ "$SCALA_BINARY_VERSION" = "" ]
  then
    echo "SCALA_BINARY_VERSION variable cannot be empty"
    exit 1
fi

echo "Create a GitHub release"
JAR_PATH=target/scala-${SCALA_BINARY_VERSION}/${PROJECT_NAME}_${SCALA_BINARY_VERSION}-${SPARK_VERSION}_${PROJECT_VERSION}.jar
hub release create -a $JAR_PATH -m "Release v${PROJECT_VERSION}" v${PROJECT_VERSION}

I'd use awk to generate shell syntax, and source it in the shell:

$ source <(
    awk '
        $1 == "name"         {print "PROJECT_NAME=" $NF}
        $1 == "scalaVersion" {print "SCALA_VERSION=" $NF}
    ' build.sbt
)

$ echo $PROJECT_NAME,$SCALA_VERSION
happy,2.11.8

If the name or version strings might contain spaces, then we need to be more specific about awk's field separator:

$ cat build.sbt
name := "hello world"
scalaVersion := "2.11.8"
sparkVersion := "2.2.0"

$ awk '
    $1 == "name"         {print "PROJECT_NAME=" $NF}
    $1 == "scalaVersion" {print "SCALA_VERSION=" $NF}
' build.sbt

PROJECT_NAME=world"              # << oops
SCALA_VERSION="2.11.8"

$ awk -F '"' '
    $1 ~ /^name/         {printf "PROJECT_NAME=\"%s\"\n",  $2}
    $1 ~ /^scalaVersion/ {printf "SCALA_VERSION=\"%s\"\n", $2}
' build.sbt

PROJECT_NAME="hello world"
SCALA_VERSION="2.11.8"


Is there a more efficient way of loading views within my java app? I'm building a web application in Java.  I'm new to Java, so before I get too far into this... I'm hoping to take advantage of all the experience here and learn the problems with this setup.

My Servlet

package cms.content;

import java.io.IOException;
import javax.servlet.ServletException;
import javax.servlet.annotation.WebServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

/**
 *
 * @author Ben
 */
@WebServlet(name = "EditServlet", urlPatterns = {"/content/edit"})
public class EditServlet extends library.Controller {
    @Override
    public void doGet (HttpServletRequest request, HttpServletResponse response)
            throws ServletException, IOException {

        this.loadView(request, response, "/content/edit");

        response.getWriter().write("Hi");
        System.out.println("EditServlet loaded");
    }
}

My Controller

package library;

import java.io.IOException;
import javax.servlet.RequestDispatcher;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

/**
 *
 * @author Ben
 */
public class Controller extends HttpServlet {
    public final library.MySQL db;

    public Controller() {
        this.db = new MySQL();
    }

    public void loadView (HttpServletRequest request, HttpServletResponse response, String view)
            throws ServletException, IOException {
        RequestDispatcher dispatcher = request.getRequestDispatcher(view+".jsp");

        response.setContentType("text/html;charset=UTF-8");

        System.out.println("MyServlet::LoadView() success");

        dispatcher.forward(request, response);
    }
}

My View

<%@page contentType="text/html" pageEncoding="UTF-8"%>
<!DOCTYPE html>

        <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
        <title>JSP Page</title>

This is edit.jsp

I haven't tried to pass any parameters to this view or anything like that.  But are there any obvious problems or improvements that need to be made?

First off, its not clear what you're trying to achieve, so its hard to evaluate.

But, looking a the code there comes after you forward to the view, that could be confusing, because it will still execute.

At a higher level, I also don't really see the need for this complicated of a setup to get to your edit page when you can just link straight to the edit jsp all nice an RESTful like.  If you're just experimenting and trying to learn that's cool, but if you're really trying to get something done then you should look at just using an existing framework.


"Sack" data structure in C# The idea is to have a data structure that holds any number of objects of any type. Objects can be retrieved, or removed permanently, at random. Also, it can be cleared (or emptied).

This class just provides the ability to select a random object from a collection, with the option of removing it.

public class Sack
{
    private List<object> objects;

    public Sack()
    {
        objects = new List<object>();
    }

    public void Add<T>(T obj)
    {
        objects.Add(obj);
    }

    public object Retrieve()
    {
        int numberObjects = objects.Count;

        Random rand = new Random();
        int selectedIndex = rand.Next(0, numberObjects);

        return objects[selectedIndex];
    }

    public object Remove()
    {
        int numberObjects = objects.Count;

        Random rand = new Random();
        int selectedIndex = rand.Next(0, numberObjects);

        object selectedObject = objects[selectedIndex];
        objects.RemoveAt(selectedIndex);

        return selectedObject;
    }

    public void Empty()
    {
        objects.Clear();
    }
}

You don't use any method which is specific to List<T>. You should always code against interfaces if possible, meaning you should use IList<T> instead of List<T>.
If Retrieved is called very often in a short time it is likely that you get the same object each time. This is because Random when instantiated will use the system clock to provide a seed value. You should have one Random object as a class-level field which should be instantiated in the constructor or directly.
The genericness of the method Add<T> doesn't buy you anything. A simple Add(object) would be enough and does the exact same.
objects should be made readonly because you don't change it.
For Retrieve and Remove I wouldn't introduce the numberObjects variable but if you want to keep it you should rename it to e.g numberOfObjects.

Implementing the mentioned points would look like this:

public class Sack
{
    private readonly IList<object> objects = new List<object>();
    private readonly Random rand = new Random();

    public void Add(object obj)
    {
        objects.Add(obj);
    }

    public object Retrieve()
    {
        int selectedIndex = rand.Next(0, objects.Count);

        return objects[selectedIndex];
    }

    public object Remove()
    {
        int selectedIndex = rand.Next(0, objects.Count);

        object selectedObject = objects[selectedIndex];
        objects.RemoveAt(selectedIndex);

        return selectedObject;
    }

    public void Empty()
    {
        objects.Clear();
    }
}