AngularJS controller to fetch and display employee data from database This is the code for an AngularJS controller that I've written:

app.controller('employeeController', function (employeeService) {

    var Controller = this;

    Controller.employees = {};
    Controller.fetchedEmployee = {};

    Controller.populateTable = function (data) {

        Controller.employees = data;
    };

    Controller.error = function (errorResponse) {
        console.log("error function called.");
        console.log(errorResponse);
        // Request Errors
        if(errorResponse.status === 400) alert("Error: " + errorResponse.data.error + ": There was a problem with the data you submitted. \nPlease enter valid form data.");
        else if(errorResponse.status === 401) alert("Error: " + errorResponse.data.error + ": You need to provide proper authorization credentials to continue.");
        else if(errorResponse.status === 403) alert("Error: " + errorResponse.data.error + ": You are not allowed to perform this action.");
        else if(errorResponse.status === 404) alert("Error: " + errorResponse.data.error + ": The requested file or resource was not found.");
        else if(errorResponse.status === 408) alert("Error: " + errorResponse.data.error + ": The connection timed out. Please try again later.");
        else if(errorResponse.status === 410) alert("Error: " + errorResponse.data.error + ": The requested resource is no longer available.");
        else if(errorResponse.status === 418) alert("Error: " + errorResponse.data.error + ": I'm a teapot, not a coffeepot.");
        // Server Errors
        else if(errorResponse.status === 500) {
            if(errorResponse.data.exception === "java.sql.SQLException") alert("Error establishing connection with database. Please try again later.");
            else alert("Error: " + errorResponse.data.error + ": Generic Server Error.");
        }
    };

    Controller.deleteEmployee = function(id) {

        employeeService.getEmployee(id).then(function(data) {
            Controller.fetchedEmployee = data.empInfo;
            var reallyDelete = confirm("Do you really want to delete " + Controller.fetchedEmployee.employeeName + "?");
            console.log(reallyDelete);
            if(reallyDelete === true) employeeService.deleteEmployee(id);

        }, Controller.error);

    };

    // Call Service to List all Employees
    console.log("Service called to populate table.");
    employeeService.getAllEmployees().then(Controller.populateTable, Controller.error);
    Controller.populateTable();
});

Not that this code hasn't gone through improvements; initially it was full of this.something or even worse, $scope.something (scope soup), which made it difficult to understand and even resulted in some of the code being broken.

However, it still feels sloppy. It feels like all the functions and variables have been 'injected' randomly in the controller; like someone randomly putting playing cards in a neatly shuffled deck or randomly placing books in a bookshelf.

Could anyone help me devise a methodology to organize this code to have a definite structure?

You should create a json object for your errors. Key - error number, value - custom error message. Try to be DRY. Just imagine situation, when i18n will be required.


Play audio files when buttons are clicked in Simon Says function playGreen(){
       var audio = document.getElementById("audioG");
       audio.play();
                 }
  function playRed(){
       var audio = document.getElementById("audioR");
       audio.play();
                 }
  function playYellow(){
       var audio = document.getElementById("audioY");
       audio.play();
                 }
  function playBlue(){
       var audio = document.getElementById("audioB");
       audio.play();
                 }

It will function as is.. but it seems like I should be able to reduce the four function's into one with a variable? can I get some guidance on that, along with any other pointers too.. just starting on the pseudo logic for the rest of the javascript.

        <audio preload="auto"  id="audioG" data-pad="1">
          <source src="https://s3.amazonaws.com/freecodecamp/simonSound1.mp3" type="audio/mpeg"/>
        </audio>

        <audio preload="auto"  id="audioR" data-pad="2">
          <source src="https://s3.amazonaws.com/freecodecamp/simonSound2.mp3" type="audio/mpeg"/>
        </audio>

        <audio preload="auto"  id="audioY" data-pad="3">
          <source src="https://s3.amazonaws.com/freecodecamp/simonSound3.mp3" type="audio/mpeg"/>
        </audio>

        <audio preload="auto"  id="audioB" data-pad="4">
          <source src="https://s3.amazonaws.com/freecodecamp/simonSound4.mp3" type="audio/mpeg"/>
        </audio>

Shall we
play a game?

https://codepen.io/TurtleWolf/pen/awdoxQ?editors=1010

I recommend removing the inline onclick event handlers from your HTML document for a cleaner separation of markup and logic / JavaScript similarly to how you separate markup and style / CSS.

You can then add all event handlers with a few lines of code as follows:

document.querySelectorAll('.pushPad').forEach(pad => {
  let audio = pad.querySelector('audio');
  pad.addEventListener('click', event => audio.play());
});

A better design would probably have the data-pad attributes on the colored buttons and not on hidden <audio> elements, introduce a mapping between pad and audio source and leverage the web audio API instead. Using <svg> shapes instead of heavily styled <div>s for the UI might further simplify your markup.


Navigation with arrow keys This is my attempt for C# navigation with arrow keys. Basically, when the arrow down/up key is pressed, the specified option is highlighted (so it's like a WPF/WinForms ComboBox). However, I think it looks a bit ugly, and I think many things can be made better.

I put some comments into the code, to understand better what it does.

using System;

namespace Test
{
    internal class Program
    {
        public static void Main(string[] args)
        {
            ConsoleKeyInfo consoleKeyInfo;
            int[] intArray = new int[5];
            // Index is -1 by default. That means nothing is selected.
            // I use this variable to determine, which option is selected.
            // Then I'm calling it when moving the cursor down/up.
            int index = -1;

            for (int x = 0; x < 5; x++) intArray[x] = x;
            foreach (var i in intArray) Console.WriteLine(i);
            Console.SetCursorPosition(0, 0);

            while ((consoleKeyInfo = Console.ReadKey()).Key != ConsoleKey.Enter)
            {
                if (consoleKeyInfo.Key == ConsoleKey.DownArrow)
                {
                    index++;
                    if (index == 0) // When the arrow down key is pressed first time
                    {
                        Console.SetCursorPosition(0, index);
                        Console.ResetColor();
                        Console.ForegroundColor = ConsoleColor.Blue;
                        Console.Write("\r" + new string(' ', Convert.ToString(intArray[index]).Length) + "\r"); // Clear current line
                        Console.Write(intArray[index]); // Rewrite it with matching index array item
                    }
                    else if (index >= 0 && index < intArray.Length)
                    {
                        Console.SetCursorPosition(0, index - 1);
                        Console.ResetColor();
                        Console.Write("\r" + new string(' ', Convert.ToString(intArray[index - 1]).Length) + "\r"); // Clear previous line
                        Console.Write(intArray[index - 1]); // Rewrite it

                        Console.SetCursorPosition(0, index);
                        Console.ResetColor();
                        Console.ForegroundColor = ConsoleColor.Blue;
                        Console.Write("\r" + new string(' ', Convert.ToString(intArray[index]).Length) + "\r"); // Clear current line
                        Console.Write(intArray[index]); // Rewrite it
                    }
                    // When the index is same/greater than intArray length, keep it with the same value
                    // So the index doesn't increment
                    else if (index >= intArray.Length) index = intArray.Length - 1;
                }
                else if (consoleKeyInfo.Key == ConsoleKey.UpArrow) // Up arrow is intended to work only, when the index is greater than 0 (so the second or greater option is selected)
                {
                    if (index > 0) index--;
                    if (index >= 0 && index < intArray.Length)
                    {
                        // Same as above
                        Console.SetCursorPosition(0, index + 1);
                        Console.ResetColor();
                        Console.Write("\r" + new string(' ', Convert.ToString(intArray[index + 1]).Length) + "\r");
                        Console.Write(intArray[index + 1]);

                        Console.SetCursorPosition(0, index);
                        Console.ResetColor();
                        Console.ForegroundColor = ConsoleColor.Blue;
                        Console.Write("\r" + new string(' ', Convert.ToString(intArray[index]).Length) + "\r");
                        Console.Write(intArray[index]);
                    }
                }
            }
        }
    }
}

You have lots of redundant code, and you don't use objects at all (even methods).

You should separate drawing logic from navigation logic.

Try to write something like this:

using System;
using System.Collections.Generic;
using System.Linq;

namespace Test
{
    //  logic for selecting specific option
    public class Menu
    {
        public Menu (IEnumerable<string> items)
        {
            Items = items.ToArray ();
        }

        public IReadOnlyList<string> Items { get; }

        public int SelectedIndex { get; private set; } = -1; // nothing selected

        public string SelectedOption => SelectedIndex != -1 ? Items[SelectedIndex] : null;

        public void MoveUp () => SelectedIndex = Math.Max (SelectedIndex - 1, 0);

        public void MoveDown () => SelectedIndex = Math.Min (SelectedIndex + 1, Items.Count - 1);
    }

    // logic for drawing menu list
    public class ConsoleMenuPainter
    {
        readonly Menu menu;

        public ConsoleMenuPainter (Menu menu)
        {
            this.menu = menu;
        }

        public void Paint (int x, int y)
        {
            for (int i = 0; i < menu.Items.Count; i++)
            {
                Console.SetCursorPosition (x, y + i);

                var color = menu.SelectedIndex == i ? ConsoleColor.Yellow : ConsoleColor.Gray;

                Console.ForegroundColor = color;
                Console.WriteLine (menu.Items[i]);
            }
        }
    }

    internal class Program
    {
        public static void Main (string[] args)
        {
            var menu = new Menu (new string[] { "John", "Bill", "Janusz", "Grażyna", "1500", ":)" });
            var menuPainter = new ConsoleMenuPainter (menu);

            bool done = false;

            do
            {
                menuPainter.Paint (8, 5);

                var keyInfo = Console.ReadKey ();

                switch (keyInfo.Key)
                {
                    case ConsoleKey.UpArrow   : menu.MoveUp ();   break;
                    case ConsoleKey.DownArrow : menu.MoveDown (); break;
                    case ConsoleKey.Enter     : done = true;      break;
                }
            }
            while (!done);

            Console.ForegroundColor = ConsoleColor.Cyan;
            Console.WriteLine ("Selected option: " + (menu.SelectedOption ?? "(nothing)"));
            Console.ReadKey ();
        }
    }
}


Generating a non binary tree structure I am writing a method to create a folder tree. I am getting the data from a database and creating the list of nodes. Is there a way to get the method to execute quicker? Perhaps using parallel task library?

Each node looks like this:

public class Node : INode
{
    public int Id { get; set; }
    public int? ParentId { get; set; }
    public string DisplayText { get; set; }
    public bool DefaultState { get; set; }
    public List<Node> ChildItems { get; set; }
}

The fastest implementation I have been able to come up with has been just two for loops using recursion:

/// <summary>
/// For loop incrementing: 00:00:28.2218157
/// </summary>
/// <param name="folderRepo"></param>
/// <param name="folders"></param>
private void GetChildFoldersIncrement(IRepository<Folder> folderRepo, List<Node> folders)
{
    for (var i = 0; i < folders.Count; i++)
    {
        var folderId = folders[i].Id;
        var childFolders = folderRepo.FindByExp(f => f.ParentFolderID == folderId).ToList();
        if (childFolders.Count == 0) continue;
        for (var j = 0; j < childFolders.Count; j++)
        {
            var node = new Node
            {
                Id = childFolders[j].FolderID,
                ParentId = childFolders[j].ParentFolderID,
                DisplayText = childFolders[j].FolderName,
                DefaultState = false,
                ChildItems = new List<Node>()
            };

            if (folders[i].ChildItems == null) folders[i].ChildItems = new List<Node>();
            folders[i].ChildItems.Add(node);
        }

        if (folders[i].ChildItems.Count > 0) GetChildFoldersIncrement(folderRepo, folders[i].ChildItems);
    }
}

I've attempted to optimize the loops by decrementing them but it was actually slightly slower:

/// <summary>
/// For loop decrement: 00:00:28.3496396
/// </summary>
/// <param name="folderRepo"></param>
/// <param name="folders"></param>
private void GetChildFoldersDecrement(IRepository<Folder> folderRepo, List<Node> folders)
{
    if (folders == null) return;
    var maxFolders = folders.Count();
    for (var i = maxFolders - 1; i >= 0; --i)
    {
        var folderId = folders[i].Id;
        var childFolders = folderRepo.FindByExp(f => f.ParentFolderID == folderId).ToList();
        var maxChildFolders = childFolders.Count();
        for (var j = maxChildFolders - 1; j >= 0; --j)
        {
            var node = new Node
            {
                Id = childFolders[j].FolderID,
                ParentId = childFolders[j].ParentFolderID,
                DisplayText = childFolders[j].FolderName,
                DefaultState = false,
                ChildItems = new List<Node>()
            };

            if (folders[i].ChildItems == null) folders[i].ChildItems = new List<Node>();
            folders[i].ChildItems.Add(node);
        }

        GetChildFoldersDecrement(folderRepo, folders[i].ChildItems);
    }
}

Let's start from your code. The recursion can be implemented in a much clearer way if the method returns the collection of all child folders, instead of updating parent folder's ChildItems collection. In this case it could look as simple as

private IEnumerable<Node> GetChildFolderNodes(IRepository<Folder> folderRepo, int folderId)
{
    var childFolders = folderRepo.FindByExp(f => f.ParentFolderID == folderId).ToArray();

    return childFolders.Select(folder => new Node
    {
        Id = folder.FolderID,
        ParentId = folder.ParentFolderID,
        DisplayText = folder.FolderName,
        DefaultState = false,
        ChildItems = GetChildFolderNodes(folderRepo, folder.FolderID).ToList()
    });
}

There are a number of articles describing why a IRepository<T> is quite a bad idea when defined on top of ORM framework. Main reasons are leaked abstractions and abstraction on top of abstraction. And your example shows why - any optimization requires the knowledge of underlying implementation (Entity Framework).

Running this process in multiple threads may help, but will put additional load on the database server and thus is not scalable. Note that Entity Framework's DbContext is not thread-safe, so you cannot share the IRepository in multiple threads unless you spin up a new DbContext per request there (which I wouldn't recommend doing).

Assuming that you're using Entity Framework 6.0, FindByExp method returns IQueryable<Folder>, you can try asynchronous implementation of the data retrieval:

private async Task<List<Node>> GetChildFolderNodesAsync(Func<IRepository<Folder>> folderRepoFactory, int folderId)
{
    Folder[] childFolders;
    using (var folderRepo = folderRepoFactory())
    {
        childFolders = await folderRepo.FindByExp(f => f.ParentFolderID == folderId).ToArrayAsync();
    }

    return (await Task.WhenAll(childFolders.Select(async folder => new Node
    {
        Id = folder.FolderID,
        ParentId = folder.ParentFolderID,
        DisplayText = folder.FolderName,
        DefaultState = false,
        ChildItems = await GetChildFolderNodesAsync(folderRepoFactory, folder.FolderID)
    }))).ToList();
}

Note that in this case you would need to spin up a new DbContext per request (which I expressed as a repository factory).

And finally, assuming that this is a critical path in your application, the best way to optimize the code you're running is to reduce the number of round trips to database server, instead of trying to squeeze microseconds on a .NET layer.

For example, if your underlying DB engine is SQL Server, you can leverage the recursive CTE to load full folder hierarchy in one go. It can be optimized even further if CTE returns the depth level for folders, but I'll leave it for you as an exercise:

private List<Node> GetChildFolderNodesAsync(DbSet<Folder> folderSet, int folderId)
{
    var allFolders = folderSet.SqlQuery(@"WITH cte (FolderID, ParentFolderID, FolderName) AS
    (
        SELECT  FolderID, ParentFolderID, FolderName
        FROM    Folders
        WHERE   ParentFolderID = @p0
    UNION ALL
        SELECT  F.FolderID, F.ParentFolderID, F.FolderName
        FROM    Folders F
        JOIN    cte
        ON      F.ParentFolderID = cte.FolderID
    )
    SELECT  *
    FROM cte", folderId).AsNoTracking();

    //Converting all folders into nodes, but skipping the ChildItems property for now...
    var nodesByParent = allFolders
        .GroupBy(folder => folder.ParentFolderID)
        .ToDictionary(folders => folders.Key, folders => folders.Select(folder => new Node
        {
            Id = folder.FolderID,
            ParentId = folder.ParentFolderID,
            DisplayText = folder.FolderName,
            DefaultState = false,
        }).ToList());

    List<Node> childNodes;

    //Now that we have all nodes grouped by parent, we can look up and assign ChildItems for all nodes
    foreach (var node in nodesByParent.Values.SelectMany(nodes => nodes))
    {
        node.ChildItems = nodesByParent.TryGetValue(node.Id, out childNodes)
            ? childNodes
            : new List<Node>();
    }

    return nodesByParent.TryGetValue(folderId, out childNodes)
        ? childNodes
        : new List<Node>();
}


Checking the status of a ZFS pool I'd like to see if there are any suggestions on how to improve a simple conditional test at the beginning of a backup script that verifies a ZFS volume is "online" and has the name "tank".

For example:

% zpool list
NAME      SIZE  ALLOC   FREE  EXPANDSZ   FRAG    CAP  DEDUP  HEALTH  ALTROOT
tank  54.5T  36.0T  18.5T         -    35%    66%  1.00x  ONLINE  -

The following simple script works, but I suspect there may be a more elegant way to do this. ShellCheck notes $STATUS is unused...

#!/bin/sh
#
STATUS=$(/sbin/zpool list | grep tank | grep ONLINE)
if [ $? -gt 0 ]; then
        echo "Problems"
        exit 1
else

echo all system go!
exit 0

fi

You can simplify the zpool command, by specifying the volume name as an argument:

/sbin/zpool list tank | grep ONLINE

This will avoid cases where tank might be part of another volume's name. Better yet, use zpool get:

/sbin/zpool get -H health tank

This returns output which is more specific and easier to parse, since the status is the third, tab-separated field:

tank     health    ONLINE   -

So:

STATUS=$(/sbin/zpool get -H health tank | awk '{print $3}')

Then, you probably should test the value of STATUS, instead of the exit code:

if [ "$STATUS" != ONLINE ]; then
        echo "Problems"
        exit 1
...

Next, be consistent in indentation:

if [ "$STATUS" != ONLINE ]; then
        echo "Problems"
        exit 1
else
        echo "All systems go!"
        exit 0
fi

Also: all system go! should be All systems go!.

exit 0 is unnecessary in this case, without it, the shell will exit the else block and then the script itself, and return the exit status of the last command executed (echo, which would return 0 unless it couldn't write output, in which case you probably shouldn't have the echo there anyway).


Automapper - Mapping a nested collection to a list I'm using Automapper to do some mapping from XSD-generated serialization object to more sane POCO's.

I'm having an issue with a particular type of mapping.

public class SourceOuterObject
{
  public SourceSet SourceSet { get; set; }
}

public class SourceSet
{
  public List<SourceObject> SourceList{ get; set; }
}

I want to map this to:

public class TargetOuterObject
{
  public List<TargetObject> TargetList{ get; set; }
}

I have tried a wide variety of mapping configurations, but the only thing that I've been able to make work is this:

Mapper.CreateMap<SourceOuterObject, TargetOuterObject>();
Mapper.CreateMap<SourceObject, TargetObject>();
Mapper.CreateMap<SourceSet, List<TargetObject>>()
    .ConvertUsing(ss => ss.SourceList.Select(bs => Mapper.Map<SourceObject, TargetObject>(bs)).ToList());

This works but seems to be way more complicated than it should be - I feel dirty calling a mapping within a mapping config.

Is this the best I can hope for or is there a better way?

When dealing with collections, the trick is always to set up a mapping of the items in the collections and never (well, I've never seen a case that needed) a mapping from one collection to another. That's the point of the generic.

The trick I always use is to work backwards, starting with a mapping between the types inside the collections and then going up a level. If one object needs to go up 2 levels, it's time to use ForMember to get to a nested property.

What you want to do in this case is to set up the following mappings:

Mapper.CreateMap<SourceObject, TargetObject>();
Mapper.CreateMap<SourceOuterObject, TargetOuterObject>()
    .ForMember(dest => dest.TargetList, opt => opt.MapFrom(src => src.SourceSet.SourceList);


Computing six dot products involving one common input vector I have created a Matlab Simulink model with a series of dot products.

Embedded Coder generates a for loop for each dot product, as evidenced by this excerpt of the generated output:

/* DotProduct: '<S3>/Dot Product1' incorporates:
 *  Inport: '<Root>/LngPwr'
 */
EPAR_SPAF_dot_B.DotProduct1 = 0.0F;
for (temp_ = 0; temp_ < 24; temp_++) {
  EPAR_SPAF_dot_B.DotProduct1 += temp_DataTypeConversion[temp_] *
  LngPwr[temp_];
}
/* End of DotProduct: '<S3>/Dot Product1' */

/* DotProduct: '<S3>/Dot Product2' incorporates:
 *  Inport: '<Root>/ShrtPwr'
 */
EPAR_SPAF_dot_B.DotProduct2 = 0.0F;
for (temp_ = 0; temp_ < 24; temp_++) {
  EPAR_SPAF_dot_B.DotProduct2 += temp_DataTypeConversion[temp_] *
  ShrtPwr[temp_];
 }
/* End of DotProduct: '<S3>/Dot Product2' */

Can I improve the Simulink model such that it will compile to better code?

I want to avoid using embedded matlab or a S-function for the purposes of maintainability.

There is a common mathematical operation involving many dot products: it's called matrix multiplication.

$$ \left[\begin{matrix}v_1 & v_2 & v_3 & v_4\end{matrix}\right]
\left[\begin{matrix}
a_1 & b_1 & c_1 \\
a_2 & b_2 & c_2 \\
a_3 & b_3 & c_3 \\
a_4 & b_4 & c_4
\end{matrix}\right] =
\left[\begin{matrix}(\vec{v}\cdot\vec{a}) & (\vec{v}\cdot\vec{b}) & (\vec{v}\cdot\vec{c})\end{matrix}\right]
$$

So, I recommend that you concatenate vectors \$\color{lightgreen}1\$ to \$\color{lightgreen}6\$ into a matrix and perform matrix multiplication.

You can also transpose and swap the matrices:

$$
\left[\begin{matrix}
a_1 & a_2 & a_3 & a_4 \\
b_1 & b_2 & b_3 & b_4 \\
c_1 & c_2 & c_3 & c_4 \\
\end{matrix}\right]
\left[\begin{matrix}v_1 \\ v_2 \\ v_3 \\ v_4\end{matrix}\right]
 =
\left[\begin{matrix}\vec{a}\cdot\vec{v} \\ \vec{b}\cdot\vec{v} \\ \vec{c}\cdot\vec{v}\end{matrix}\right]
$$


Search algorithm to find a point from a given X value I implement the following algorithm in order to find a point from a given X value in a sorted list with the same distance between each X values. Here's an example of my list content with a delta of 10:

(687;10)
(12;20)
(24;30)
(10125;40)
(0;50)
(111;60)
(-2;70)

The delta can be any float number;
The list can be up to 2.000.000pts so it needs to be as fast as possible;
I can't change the type of the points container (IPointList from ZedGraph).

Here's the code:

private ZedGraph.PointPair findPoint(ZedGraph.CurveItem curve, double x)
{
    if (curve.NPts == 1)
        return curve.Points[0];

    // Look for the min and max value
    double min = curve.Points[0].X;
    double max = curve.Points[curve.NPts - 1].X;

    double delta = curve.Points[1].X - min;

    // Prevent error if mouse is out of bounds.
    if (x > max)
        x = curve.Points[curve.NPts - 1].X;
    else if (x < min)
        x = curve.Points[0].X;

    double index = (x - min) / delta;
    int nearestIndex = (int)Math.Round(index, 0);

    if (nearestIndex < curve.NPts)
    {
        return curve.Points[nearestIndex];
    }
    return curve.Points[curve.NPts - 1];
}

The NPts == 1 case shouldn't need to be handled separately.  The min-max clamping should take care of it, if you write it smartly (using <= and >= tests).

The min-max clamping should should do a short-circuit return.

Once clamped, there shouldn't be any need to test if (nearestIndex < curve.NPts).

I would prefer to calculate the delta based on the min and max, for better precision.  Extrapolating based on Points[1].X - Points[0].X could let floating-point inaccuracies accumulate.

A sanity check before returning the value might be a good idea — it's up to you depending on how much you trust the data.

private ZedGraph.PointPair findPoint(ZedGraph.CurveItem curve, double x)
{
    // Look for the min and max value
    double min = curve.Points[0].X;
    double max = curve.Points[curve.NPts - 1].X;

    // Prevent error if mouse is out of bounds.
    if (x <= min) return curve.Points[0];
    if (x >= max) return curve.Points[curve.NPts - 1];

    double delta = (max - min) / (curve.NPts - 1);
    int index = (int)Math.round((x - min) / delta, 0);

    Debug.Assert(Math.Abs(x - curve.Points[index].X) <= delta / 2, "Interpolation error");
    return curve.Points[index];
}


Finding .txt files that exist in one directory but not the other I need to check a condition, and based on the condition perform a very similar for-loop.

import os

folder = r'path'
folder_list = os.listdir(folder)
folder2 = r'path/folder2'
args = True

the_list = []

if args is True:

    for x in folder_list:

        if x.endswith('.txt'):

            if not os.path.exists(os.path.join(folder2, x)):
                full_path = os.path.join(folder, x)
                the_list.append(full_path)
else:

    for x in folder_list:

        if x.endswith('.txt'):
                full_path = os.path.join(folder, x)
                the_list.append(x)

I would like to avoid having the redundant for-loop code if possible.
I originally had the following, which I wasn't happy with because args was being checked every single iteration, which is unnecessary.

import os

folder = r'path'
folder_list = os.listdir(folder)
folder2 = r'path/folder2'
args = True
the_list = []

for x in folder_list:

    if x.endswith('.txt'):
        full_path = os.path.join(folder, x)
        if args['i'] is True:

            if not os.path.exists(os.path.join(folder2, x)):
                the_list.append(full_path)

        else:
            the_list.append(full_path)

That's where generators can be very helpful, for example you could use:

if args:
    folderlist = (f for f in folderlist if f.endswith('.txt') and not os.path.exists(os.path.join(folder2, f))
else:
    folderlist = (f for f in folderlist if f.endswith('.txt'))

for x in folderlist:
    full_path = os.path.join(folder, x)
    the_list.append(full_path)

The checks are now done in the "generator expression"s.

You could simplify this some more because the if x.endswith('.txt') is done in both branches:

folderlist = (f for f in folderlist if f.endswith('.txt'))
if args:
    folderlist = (f for f in folderlist if not os.path.exists(os.path.join(folder2, f))

for x in folderlist:
    full_path = os.path.join(folder, x)
    the_list.append(full_path)


aggregate sheets from many excel workbooks into one workbook I want to put the worksheets from several (2-5) excel workbooks (files) into one single workbook.

The names of the workbooks are not standardized and so I opted for a dialog box to select the workbooks.

Here is the code. It meets the basic requirement that all the worksheets (sheets actually) from the selected files are collected into a single file (the ActiveWorkbook).

Sub openDialogBoxAndSelectFiles()

  Dim wb1 As Workbook
  Set wb1 = ActiveWorkbook

  With Application.FileDialog(msoFileDialogFilePicker)

    .AllowMultiSelect = True
    .InitialFileName = ThisWorkbook.path & "\"
    .Title = "Paddington Bear Selection Window"
    .ButtonName = "Omlette"

    .Filters.Clear
    .Filters.Add "All Files", "*.*"

    If .Show = True Then

      Dim file As Variant
      For Each file In .SelectedItems

        Dim wb2 As Workbook
        Set wb2 = Workbooks.Open(Filename:=file, ReadOnly:=True)

        Dim i As Long
        For i = 1 To wb2.Sheets.Count
          wb2.Sheets(i).Copy before:=wb1.Sheets(1)
        Next i

        wb2.Close
      Next

    End If

  End With

End Sub

The code is pretty clean (I think) for the time being and pretty much fits the bill, but that's because it doesn't do the following:

If the ActiveWorkbook which I've defined as wb1 has blank sheets (and a wb has  to have at least 1 sheet), once this process is done, I will have a few blank sheets left over.

So if file1.worksheets.count = 4, and file2.worksheets.count = 5, I will have a minimum of 10 (1 + 4 + 5) worksheets in the final workbook.

Questions:

Insofar as aggregating the sheets of the files is concerned, is this a reasonable way of doing it?
Is there a simple of ensuring there are no blank worksheets left over? Unless the input files have blank sheets that is.

Kindly,

I think the way you are doing this is reasonable.  Regarding pre-existing blank sheets..The rewritten code below removes blank worksheet(s) in the importing workbook, after the selected workbooks' worksheets have been inserted.

Sub openDialogBoxAndSelectFiles()

  Dim wb1 As Workbook
  Set wb1 = ActiveWorkbook

    'Cache worksheet references
    Dim originalWorksheets As Collection
    Set originalWorksheets = New Collection
    For i = 1 To wb1.Sheets.Count
      originalWorksheets.Add wb1.Sheets(i)
    Next i

    With Application.FileDialog(msoFileDialogFilePicker)
        .AllowMultiSelect = True
        .InitialFileName = ThisWorkbook.Path & "\"
        .Title = "Paddington Bear Selection Window"
        .ButtonName = "Omlette"

        .Filters.Clear
        'only interested in Exel workbooks
        .Filters.Add "All Files", "*.xls*"

        If .Show = True Then
            For Each file In .SelectedItems
                ImportWorksheets wb1, file
            Next file
        End If
    End With

    'Delete non-imported blank worksheets
    If wb1.Sheets.Count > originalWorksheets.Count Then
        DeleteBlankSheets originalWorksheets
    End If

End Sub

Private Sub ImportWorksheets(ByRef wb1 As Workbook, ByVal filename As Variant)
On Error GoTo ErrorExit
    Dim wb2 As Workbook
    Set wb2 = Workbooks.Open(filename:=filename, ReadOnly:=True)

On Error GoTo WorkbookOpenError
    Dim i As Long
    For i = 1 To wb2.Sheets.Count
      wb2.Sheets(i).Copy before:=wb1.Sheets(1)
    Next i

WorkbookOpenError:
    wb2.Close

ErrorExit:
End Sub

Private Sub DeleteBlankSheets(ByRef originalWorksheets As Collection)
On Error GoTo ErrorExit

    Dim displayAlertsFlagCurrentValue As Boolean
    displayAlertsFlagCurrentValue = Application.DisplayAlerts

    'Prevent Sheet deletion prompts
    Application.DisplayAlerts = False

    Dim wksht As Worksheet
    For Each wksht In originalWorksheets
        If IsBlank(wksht) Then
            wksht.Delete
        End If
    Next wksht

ErrorExit:
    Application.DisplayAlerts = displayAlertsFlagCurrentValue
End Sub

Private Function IsBlank(ByRef wksht As Worksheet) As Boolean
    IsBlank = WorksheetFunction.CountA(wksht.UsedRange) = 0 And wksht.Shapes.Count = 0
End Function


Loop Animation on several UIButton's NSLayoutConstraint I took the belated plunge to look at Autolayout.  Normally I'd animate based on a UIView's frame (i.e I'd have used the following):

    NSArray *sortedArray = [self.buttonArray sortedArrayUsingDescriptors:[NSArray arrayWithObject:ascendingSort]];

for (int i = 0;i<sortedArray.count;i++) {

    UIButton * theButton = [sortedArray objectAtIndex:i];

    [UIView animateWithDuration:1 delay:i *0.05 usingSpringWithDamping:10 initialSpringVelocity:20 options:UIViewAnimationOptionCurveEaseIn animations:^{
        [theButton setFrame:frameToAnimateTo];
    } completion:nil];
}

Am I right in thinking that I now have to animate based on the Constant property of a buttons constraint?

NSArray *sortedArray = [self.ButtonHorizonalConstraints sortedArrayUsingDescriptors:[NSArray arrayWithObject:ascendingSort]];

for (int i = 0;i<sortedArray.count;i++) {

    NSLayoutConstraint * constraint = [sortedArray objectAtIndex:i];
    UIButton * theButton = constraint.firstItem;

    constraint.constant = toShow?0:-theButton.frame.size.width;
    [self.view setNeedsUpdateConstraints];

    [UIView animateWithDuration:1 delay:i *(toShow?0.1:0.05) usingSpringWithDamping:10 initialSpringVelocity:toShow?20:-20 options:UIViewAnimationOptionCurveEaseIn animations:^{
        [self.view layoutIfNeeded];
    } completion:nil];
}

The end result I get is what I desire, but something doesn't seem right.

I'm not completely familiar with animating auto-layout constraints.  Fortunately (or unfortunately), I've not yet ever had the need to do this.  So, I'm not sure if there's a better way, but I'd say if it's working, that's a good sign.

What I do want to comment on is your readability:

constraint.constant = toShow?0:-theButton.frame.size.width;
[self.view setNeedsUpdateConstraints];

I don't mind ternary operators.  But I think you really need to include some white space.  When it's all smushed together, and including the negative sign, I had to do a double take before I realized what's going on.

constraint.constant = toShow ? 0 : -theButton.frame.size.width;

[UIView animateWithDuration:1 delay:i *(toShow?0.1:0.05) usingSpringWithDamping:10 initialSpringVelocity:toShow?20:-20

  options:UIViewAnimationOptionCurveEaseIn animations:^{
          [self.view layoutIfNeeded];
      } completion:nil];

This method call suffers the same problems as before with the smushed ternary operator, except it's worse because this method has so many more arguments and you're calculating so much in line and you're not using any vertical space.

I'd rewrite this as such:

CGFloat animationDelay = i * (toShow ? 0.1 : 0.05);
CGFloat springVelocity = toShow ? 20 : -20;

[UIView animateWithDuration:1
                      delay:animationDelay
     usingSpringWithDamping:10
      initialSpringVelocity:springVelocity
                    options:UIViewAnimationOptionCurveEaseIn
                 animations:^{

    [self.view layoutIfNeeded];

               } completion:nil];

With that spacing, even if we kept the ternaries in-line, it's still significantly more readable with a single argument per line.


I would like to know if the following code is 'good' I would like a serious, blunt and cold judgement on this code.  Would it be considered 'good' code?  In general, is it well written?  Is the design reasonable?

#include <iostream>   // C++ IO library code to include.
#include <string>     // C++ string functions.
using namespace std ; // Use standard C++ names for functions.

//====== class declaration.

class Tpacket_list
{public: // functions that are available to code outside the class.
    // put the command line into the list.
    Tpacket_list(int param_index, string param) ;
    void print_list() ; // print the list.
    Tpacket_list* find( string target) ;
    int    tell_id() ;
    string tell_payload() ;

 protected:                //  Data in a class is normally hidden from other code.
    string payload ;       // C++ strings are classes so this is aggregation.
    int id ;               // Such strings are more powerful and easier to use
    Tpacket_list *next ;   //   than the older C string routines, also slower!
} ;

Tpacket_list *list_head ;  // Start of linked list, set to 1st item.
Tpacket_list *work_ptr ;   // Work pointer to work along list.

//====== Class Body =========================================================

//------ Constructor gets called when the class is created. It puts in the
//       payload and sets up the chain of pointers.All this is hideen from
//       the user of the class.
Tpacket_list::Tpacket_list(int param_index, string param)
{//--- save information for this parameter.
   id = param_index ;
   payload = param ;
   cout << "    Adding parameter # " << id << ", value = " << payload << endl ;
 //--- form list.
   next = list_head ;
   list_head = this ;
}

//------ Print the list.  Note how the function calls the print_list function
//       in the next item in the link list.
//       Function prints from the current link list item to the end.
void Tpacket_list::print_list()
{//---
   cout << "    id = " << id << ",  payload = " << payload << ", length " << payload.length() << " characters."<< endl ;
   if ( next != NULL) // if there is a next item, get it to print itself.
     next->print_list() ;
}

//------ Find item in the list.
Tpacket_list* Tpacket_list::find( string target)
{//---
   if ( payload == target)
      return ( this) ;          // return pointer to current object.
   if ( next == NULL)
      return( NULL) ;           // end of list, not match, return NULL.
   //--- if got here there is a valid next.
   return( next->find( target)) ;
}

//------ Tell of internal data.
int    Tpacket_list::tell_id()
 { return( id) ;
 }

string Tpacket_list::tell_payload()
 { return( payload) ;
 }

//====== Start program here =================================================

int main(int argc, char *argv[]) // argc = number strings on command line.
                                 // argv is array of strings each holding
                                 //   a parameter.

{//--- Initialize link list with command parameters.
   cout << endl << "  Form link list of command line parameters." << endl ;
   list_head = NULL ;
   int i = 1 ;
   while ( i < argc)
    { new Tpacket_list(i, argv[i]) ;
      i++ ;
    }

 //--- print out the list.
   cout << "  Print paramters from head of list the end of list." << endl ;
   if ( list_head != NULL)
        list_head->print_list() ;
   else cout << "   List is empty." << endl ;

 //--- search for parameters
   cout << "  Find first 0 parameter (working last to first parameter, list head to tail)." << endl ;
   Tpacket_list *found ;
   if ( list_head != NULL)
     { found = list_head->find("0") ;
       if ( found != NULL)
            cout << "    Found target on parameter # " << found->tell_id()  << endl ;
       else cout << "    No \"0\" found." << endl ;
     }

   cout << endl ;
   return(0) ;
}

P.S. It's not my code, it's actually the lecturer's.

For overall quality, I'd give this about a 2 or maybe a 3 (on a scale of 1-10). Honestly, even 3 is being pretty generous though.

#include <iostream>   // C++ IO library code to include.
#include <string>     // C++ string functions.
using namespace std ; // Use standard C++ names for functions.

using namespace std; is generally frowned upon. There are reasons for using a namespace, but doing it globally like this (especially with the stdnamespace) is just a lousy idea.

//====== class declaration.

I'm sort of split on this one. On one hand, vacuous comments like this are generally worse than useless. At the same time, given that it's written exclusively for teaching, it may be excusable to include a few things we'd normally reject, describing basics of how C++ works. This would be a no-no in real code, but under the circumstances it's understandable and (I guess) reasonable.

class Tpacket_list
{public: // functions that are available to code outside the class.

Your teacher seems to be working hard at disproving the usual wisdom about formatting. Most people figure that the exact formatting you choose is less important than using it consistently. This is one of the few I've seen that really does seem to make the code more difficult to read.

    // put the command line into the list.
    Tpacket_list(int param_index, string param) ;
    void print_list() ; // print the list.
    Tpacket_list* find( string target) ;
    int    tell_id() ;
    string tell_payload() ;

The comment here is misleading at best. The code to put the command line into the list is elsewhere (somewhere down in main).

 protected:                //  Data in a class is normally hidden from other code.
    string payload ;       // C++ strings are classes so this is aggregation.
    int id ;               // Such strings are more powerful and easier to use
    Tpacket_list *next ;   //   than the older C string routines, also slower!
} ;

Here we have a real design problem. He's conflating two ideas that should be completely separate: a linked list, and a node in a linked list. To work at all well, a linked list nearly needs to separate the two:

class list {
    struct Node {
        int id;
        std::string param;
        Node *next;

        Node(int param_index, std::string const &p) :
            id(param_index), param(p)
        {}
    }

public:
    list(list const &);
    // ...
private:
    node *list_head;
};

In this case, a node is pretty much just dumb data; the list class has all the real "knowledge" of how to do useful things. Also note that in reality, this should almost certainly be a class template instead of an actual class. Instead of hard-coding an int and a std::string as the types of data being stored, we really want to make those template parameters so we can create a linked list holding any kind of data we want.

Tpacket_list *list_head ;  // Start of linked list, set to 1st item.
Tpacket_list *work_ptr ;   // Work pointer to work along list.

"Daddy, he defined a global!"
"Son, what have I told you about using language like that?"

//====== Class Body =========================================================

//------ Constructor gets called when the class is created. It puts in the
//       payload and sets up the chain of pointers.All this is hideen from
//       the user of the class.

I'll repeat the earlier comment about comments for emphasis: while (barely) acceptable under the circumstances, these would clearly have no place in real code.

Tpacket_list::Tpacket_list(int param_index, string param)
{//--- save information for this parameter.
   id = param_index ;
   payload = param ;
   cout << "    Adding parameter # " << id << ", value = " << payload << endl ;
 //--- form list.
   next = list_head ;
   list_head = this ;
}

This code has quite a problems. First of all, you should generally prefer initializing the members in an initializer list:

Tpacket_list::Tpacket_list(int param_index, string param) :
    id(param_index),
    payload(param)

OTOH, per the earlier comment, this code should really be in Node::Node instead. From the viewpoint of the linked list, this code shouldn't be in a ctor at all. Following the usual conventions for C++, this would be a function named push_front (and it would normally only take one data type as the input -- to store an int and a std::string, we'd put those together into a struct of some sort -- an std::pair, if nothing else.

//------ Print the list.  Note how the function calls the print_list function
//       in the next item in the link list.
//       Function prints from the current link list item to the end.
void Tpacket_list::print_list()
{//---
   cout << "    id = " << id << ",  payload = " << payload << ", length " << payload.length() << " characters."<< endl ;
   if ( next != NULL) // if there is a next item, get it to print itself.
     next->print_list() ;
}

IMO, the very existence of this function is a problem. To print out the contents of a list, we really want to apply an algorithm (e.g., std::copy) to the list (or some subset thereof). To do that, our list should define begin() and end(), which return list iterators. The iterator type should define (at least) operator++ to traverse the list. In our case, the iterator type would be a fairly thin wrapper around a pointer to a node:

class list {
    class Node { /* ... */ };

    // warning: typing this in off the top of my head. It probably has at least a few
    // problems.
    class iterator {
        Node *pos;
    public:
        Node operator*() { return *pos; }
        iterator &operator++() {
            if (NULL == pos)
                return NULL;
            pos = pos -> next;
            return *this;
        }
        bool operator==(iterator const &other) {
            return pos == other.pos;
        }
    };

This way, when we want to print out a list, we can use the same algorithms (and idioms in general) that we can with any of the other containers in the standard library.

//------ Find item in the list.
Tpacket_list* Tpacket_list::find( string target)
{//---
   if ( payload == target)
      return ( this) ;          // return pointer to current object.
   if ( next == NULL)
      return( NULL) ;           // end of list, not match, return NULL.
   //--- if got here there is a valid next.
   return( next->find( target)) ;
}

Again, probably pointless -- instead of a find member function, provide a conforming iterator type so you use use std::find (among other things).

//------ Tell of internal data.
int    Tpacket_list::tell_id()
 { return( id) ;
 }

string Tpacket_list::tell_payload()
 { return( payload) ;
 }

I can't say I like these much, but they're about the only thing so far that isn't obviously/blatantly wrong...

//====== Start program here =================================================

int main(int argc, char *argv[]) // argc = number strings on command line.
                                 // argv is array of strings each holding
                                 //   a parameter.

{//--- Initialize link list with command parameters.
   cout << endl << "  Form link list of command line parameters." << endl ;
   list_head = NULL ;
   int i = 1 ;
   while ( i < argc)
    { new Tpacket_list(i, argv[i]) ;
      i++ ;
    }

Here we see the bad effects of the poor design. This code claims to create (and, incidentally, leak) argc-1 separate Tpacket_lists. In reality, it creates a single linked list, because the ctor does an implicit push_front on the (singular) global linked list represented by the nasty global. For usability we clearly want something like this:

    linked_list<std::pair<int, std::string> > list;

    for (int i=1; i<argc; i++)
        list.push_back(std::make_pair(i, argv[i]));

 //--- print out the list.
   cout << "  Print paramters from head of list the end of list." << endl ;
   if ( list_head != NULL)
        list_head->print_list() ;
   else cout << "   List is empty." << endl ;

Then, to print out the list, we'd have code like:

if (list.empty())
    std::cout << "List is empty.\n";
else
    std::copy(list.begin(), list.end(),
        std::ostream_iterator<std::pair<int, std::string>(std::cout, "\n"));

and of course:

 //--- search for parameters
   cout << "  Find first 0 parameter (working last to first parameter, list head to tail)." << endl ;
   Tpacket_list *found ;
   if ( list_head != NULL)
     { found = list_head->find("0") ;
       if ( found != NULL)
            cout << "    Found target on parameter # " << found->tell_id()  << endl ;
       else cout << "    No \"0\" found." << endl ;
     }

...searching in the linked list would be done with std::find instead of writing our own (clumsy) imitation.

As a final note, since the linked list allocates Nodes dynamically, we almost certainly need to provide a dtor that will delete those nodes. We also want to either provide a copy ctor that will copy the nodes, or else declare the copy ctor privately1 to prevent anybody from copying a linked list at all.

1 In C++11, we could use the =delete syntax to remove it as well, but it may be a bit premature to teach that as normal C++ usage just yet.


Method to check header token if valid I've written the following function which reads an authorization header, compares it and returns whether it is valid or not.

The function works correctly:

func Auth(req *http.Request) bool{
    sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 2)
    if sHeader[0] != "" {
        fmt.Printf("sHeader: ", sHeader[1])
        devEnv := os.Getenv("authorization")
        if (devEnv == sHeader[1]) {
            fmt.Println("valid")
            return true
        } else {
            fmt.Println("Not valid: " + sHeader[1])
            return false
        }
    } else {
        log.Fatal("Missing Authorization Header")
        return false
    }
}

What can I improve?

Here are my notes that I would bring to the code review.

package main

import (
    "fmt"
    "log"
    "net/http"
    "os"
    "strings"
)

func AuthCodeReview(req *http.Request) bool { // why bool? why not error
    sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 3) // 3 for fields after 0 & 1?
    if len(sHeader) == 0 || sHeader[0] == "" {                         // check index in range
        log.Print("Missing Authorization Header") // never Fatal!
        return false
    }
    if len(sHeader) > 1 { // why 1? why ignore 0? check index in range
        fmt.Printf("sHeader: ", sHeader[1])  // debug?
        devEnv := os.Getenv("authorization") // LookupEnv?
        if devEnv == sHeader[1] {
            fmt.Println("valid") // debug?
            return true
        }
    }
    fmt.Println("Not valid: " + sHeader[1]) // debug?
    return false
}

func Auth(req *http.Request) bool {
    sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 2)
    if sHeader[0] != "" {
        fmt.Printf("sHeader: ", sHeader[1])
        devEnv := os.Getenv("authorization")
        if devEnv == sHeader[1] {
            fmt.Println("valid")
            return true
        } else {
            fmt.Println("Not valid: " + sHeader[1])
            return false
        }
    } else {
        log.Fatal("Missing Authorization Header")
        return false
    }
}

func main() {}

Playground: https://play.golang.org/p/_MwBV80vWx

  log.Fatal is equivalent to log.Print() followed by a call to
  os.Exit(1).

  os.Exit causes the current program to exit with the given status code.
  The program terminates immediately; deferred functions are not run.

Your program is running 100,000 client connections using goroutines. One connection has a problem, so you crash the program. We have 100,000 very upset customers. Handle errors and fail gracefully; return errors; don't panic or Exit.

See Errors.

fmt.Println("valid") // debug?

Your sysout is a massive clutter of messages. That may make sense for debugging, but it doesn't make sense for production. What is the purpose of this and similar statements.

fmt.Println("Not valid: " + sHeader[1]) // debug?

If this message is not for debugging, it should be logged (log not fmt).

  strings.SplitN The count determines the number of substrings to
  return: n > 0: at most n substrings; the last substring will be the
  unsplit remainder.

// 3 for fields after 0 & 1?
sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 3)

If you are expecting two fields SplitN(2) will return the first field in index 0 and any remainder in index 1. SplitN(3) will return the first field in index 0, the second field in index 1 and any remainder in index 2. Having a remainder may or may not be valid.

Playground: https://play.golang.org/p/IQA8iqIdN5

sHeader[1] // why 1? why ignore 0?

You don't say what the format of req.Header.Get("authorization") is. What does sHeader[1] represent. Add a comment to define sHeader and sHeader[1].

I reorganized your code.

Don't write code as a heavily indented stream of consciousness. First, write the normal flow. The first step in a code review is to read and verify the normal flow, it should be obvious. Second, add exception handling. Third, add error handling. Sensibly minimize indentation.

See Indent Error Flow.

For example, a simple first draft,

func AuthFirstDraft(req *http.Request) error {
    // TODO: improve error detection and handling
    sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 3)
    devEnv := os.Getenv("authorization")
    if len(sHeader) > 1 && devEnv == sHeader[1] {
        return nil
    }
    return fmt.Errorf("error")
}

Code review is an iterative process. If there are significant changes, open a new question with the revised code.


System to handle the short lived async task I'm developing an application in which there are many short-lived tasks which are run in separate threads. I've developed a generic and reusable system to handle such a requirement. The code snippet given below is using this system code plus the example client code to show how to use the system's API.

package com.demo;

import java.util.concurrent.Callable;

import java.util.concurrent.ExecutionException;

import java.util.concurrent.ExecutorService;

import java.util.concurrent.Executors;

import java.util.concurrent.Future;

import java.util.concurrent.TimeUnit;

////////////////////////API to handle short lived async task///////////////////////////////////////////

/*
 * This class holds the threadpool executor instance and provide the submit method which in turn submit the task
 * to executor
 */

class AsyncTaskManager
{
    private ExecutorService service;

public AsyncTaskManager(ExecutorService service){
    this.service = service;
}

public <T> Future<T>  submit(Callable<T> callable){
    Future<T> result = service.submit(callable);
    return result;
}

public void shutdown(){
    service.shutdown();
    try {
        service.awaitTermination(60, TimeUnit.SECONDS);
    } catch (InterruptedException e) {
        service.shutdownNow();
    }
    service.shutdownNow();
}

}

/**
 * This is a wrapper class on Task object to make it async
 * @author jayesh
 *
 * @param <T>
 * @param <V>
 */

class AsyncTask<T extends Task<V>, V> implements Callable<V>{
private Task<V> task;

public AsyncTask(Task<V> task){
    this.task = task;
}

@Override
public V call() throws Exception {
    return task.doTask();
}

}

/**
 * Client will implement this interface to perform the task
 * @author jayesh
 *
 * @param <V>
 */

interface Task<V>{
    public  V doTask();
}

/////////////////////////////////////////End of API ////////////////////////////////////////

//////////////////////Use of API by client code /////////////////////////////////////////////

public class AsyncTaskManagerDemo {

public static void main(String[] args) throws InterruptedException, ExecutionException {

    AsyncTaskManager taskManager = new AsyncTaskManager(Executors.newCachedThreadPool());

    Task<String> urlLoader = new UrlLoaderTask("http://www.google.com");
    Task<Integer> wordCounter = new WordCounterTask("/var/tmp/test.txt");

    AsyncTask<Task<String>, String> asyncUrlLoader = new AsyncTask<Task<String>, String>(urlLoader);
    AsyncTask<Task<Integer>, Integer> asyncWordCounter = new AsyncTask<Task<Integer>, Integer>(wordCounter);

    Future<String> urlLoaderResult = taskManager.submit(asyncUrlLoader);
    System.out.println(urlLoaderResult.get());

    Future<Integer> wordCounterResult = taskManager.submit(asyncWordCounter);
    System.out.println(wordCounterResult.get());

    taskManager.shutdown();
}
}

/**
 * Task implementation which do the  task and returns result
 * @author xxx
 *
 */

class UrlLoaderTask implements Task<String>
{
private String url;
public UrlLoaderTask(String url){
    this.url = url;
}
@Override
public String doTask(){
    try {
        Thread.sleep(3000);
    } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }
    return "returning content of url - " + url;
}
};

/**
 * Task implementation which do the  task and returns result
 * @author xxx
 *
 */

class WordCounterTask implements Task<Integer>
{
private String filePath;
public WordCounterTask(String filePath){
    this.filePath = filePath;
}
@Override
public Integer doTask(){
    try {
        Thread.sleep(5000);
    } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

    return 1000; //returning no of words in a file
}
};

I am looking for a review on my system and its API.

I would strongly recommend a use of Java 8 features to make your library (which has an API) an easier to use system.

Your Task interface should be a @FunctionalInterface. It is one already, but you should make it formal with the annotation:

@FunctionalInterface
interface Task<V>{
    public  V doTask();
}

Now, any time you want a task, you can do things like:

Task<Long> wordcount = () -> countWords("Hello World");

The above is equivalent to:

Task<Long> wordcount = new Task<Long>() {
    @Override
    public Long doTask() {
        return countWords("Hello World");
    }
}

This leads on to the next concern I have with the TaskManager. Here's your code copied:

class AsyncTaskManager
{
    private ExecutorService service;

    public AsyncTaskManager(ExecutorService service){
        this.service = service;
    }

    public <T> Future<T>  submit(Callable<T> callable){
        Future<T> result = service.submit(callable);
        return result;
    }

    public void shutdown(){
        service.shutdown();
        try {
            service.awaitTermination(60, TimeUnit.SECONDS);
        } catch (InterruptedException e) {
            service.shutdownNow();
        }
        service.shutdownNow();
    }

}

The following concern me:

The service should be final, make it immutable.
The service is created outside the class, but then shut down inside the class. This is asymmetrical. If the service is created outside then it should be shut-down outside.
The class should be AutoClosable instead of having the shutdown() method.

Again, I would use a Java-8 concept called a supplier. Instead of passing in a service, pass in a method that can create a service. That way, you can still add a custom service, but you create it, not the people who call you.

About the try/catch block in the shutdown... you should use a finally block instead of what you currently have, and also, you should reset the interrupted flag if there is an interrupt. This is better than doing nothing (which is what you currently do). It avoids the exception, but makes the next thing that checks have to handle it. In other words, you pass the buck.

This is how it would look:

class AsyncTaskManager implements AutoClosable {
    private final ExecutorService service;

    public AsyncTaskManager(Supplier<ExecutorService> service){
        this.service = service.get();
    }

    public <T> Future<T>  submit(Callable<T> callable){
        return service.submit(callable);
    }

    @Override
    public void close() {

        service.shutdown();

        try {
            service.awaitTermination(60, TimeUnit.SECONDS);
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
        } finally {
            service.shutdownNow();
        }
    }

}


Staging/release nginx config I don't like how this config has so much duplication. Any suggestions on how to at least shorten it somewhat? Normally we only have one server block as the staging server is separate from production, but the client has gotten their own server so we'll need to stage on that.

upstream upgrade-release{
    server 127.0.0.1:8000;
}

upstream upgrade-staging{
    server 127.0.0.1:8010;
}

server {
    listen 80;
    server_name www.upgrade.sg upgrade.sg;

    access_log  /var/log/nginx/upgrade.access.log;
    error_log   /var/log/nginx/upgrade.error.log;

    root /home/upgrade/npcec/release/src/npcec;

    location /media/ {
        expires max;
        access_log off;
    }

    location /static/ {
        expires max;
        access_log off;
    }

    location /robots.txt {
        alias /home/upgrade/npcec/release/src/npcec/static/robots.txt;
        access_log off;
    }

    location /favicon.ico {
        alias /home/upgrade/npcec/release/src/npcec/static/favicon.ico;
        expires max;
        access_log off;
    }

    location / {
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header Host $http_host;
        proxy_redirect off;
        proxy_pass http://upgrade-release;
    }
}

server {
    listen 80;
    server_name staging.upgrade.sg;

    access_log  /var/log/nginx/upgrade.access.log;
    error_log   /var/log/nginx/upgrade.error.log;

    root /home/upgrade/npcec/staging/src/npcec;

    location /media/ {
        expires max;
        access_log off;
    }

    location /static/ {
        expires max;
        access_log off;
    }

    location /robots.txt {
        alias /home/upgrade/npcec/staging/src/npcec/static/robots.txt;
        access_log off;
    }

    location /favicon.ico {
        alias /home/upgrade/npcec/staging/src/npcec/static/favicon.ico;
        expires max;
        access_log off;
    }

    location / {
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header Host $http_host;
        proxy_redirect off;
        proxy_pass http://upgrade-staging;
    }
}

My little attempt basically takes items that are repeated and puts them into their own separate conf files and just include them into the main conf files. On my servers, I structure them as such:

global
    include1.conf
    include2.conf
    include3.conf
sites-available
    site1.conf
    site2.conf

And the includes are included in the SA conf files. Not good for performance, but negligible.

Anyways, here's an idea:

upstream upgrade-release{
    server 127.0.0.1:8000;
}

upstream upgrade-staging{
    server 127.0.0.1:8010;
}

server {
    listen 80;
    server_name www.upgrade.sg upgrade.sg;

    access_log  /var/log/nginx/upgrade.access.log;
    error_log   /var/log/nginx/upgrade.error.log;

    root /home/upgrade/npcec/release/src/npcec;

    include media_static.conf;

    location /robots.txt {
        alias /home/upgrade/npcec/release/src/npcec/static/robots.txt;
        access_log off;
    }

    location /favicon.ico {
        alias /home/upgrade/npcec/release/src/npcec/static/favicon.ico;
        expires max;
        access_log off;
    }

    location / {
        include proxy.conf;
        proxy_pass http://upgrade-release;
    }
}

server {
    listen 80;
    server_name staging.upgrade.sg;

    access_log  /var/log/nginx/upgrade.access.log;
    error_log   /var/log/nginx/upgrade.error.log;

    root /home/upgrade/npcec/staging/src/npcec;

    include media_static.conf;

    location /robots.txt {
        alias /home/upgrade/npcec/staging/src/npcec/static/robots.txt;
        access_log off;
    }

    location /favicon.ico {
        alias /home/upgrade/npcec/staging/src/npcec/static/favicon.ico;
        expires max;
        access_log off;
    }

    location / {
        include proxy.conf;
        proxy_pass http://upgrade-staging;
    }
}

In media_static.conf:

location /media/ {
    expires max;
    access_log off;
}

location /static/ {
    expires max;
    access_log off;
}

In proxy.conf:

proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header Host $http_host;
proxy_redirect off;

Also, instead of proxy_pass http:// perhaps consider using proxy_pass $scheme. Perhaps someone with more experience with Nginx confs will follow up with a way to store certain bits and pieces in variables or will offer a better solution than mine (I for one would love to know a better way of doing things)


Secure random password generator I am developing an application that must generate a random password for each new user in the system (initial password).

My idea is to use passwords of 16 characters long.

Would like to know if the code below is a secure way to generate passwords and if it can be improved. The system will be used by hundreds of thousands of users and security is a priority.

import java.security.SecureRandom;
import java.util.Random;

public class SecureRandomPasswordGenerator {

    private static final Random RANDOM = new SecureRandom();
    private static final String POSSIBLE_CHARACTERS = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_-+=!@#$%&*()[]{}<>:.,?";

    public String generatePassword(int length) {
        StringBuilder password = new StringBuilder(length);
        for (int i = 0; i < length; i++) {
            password.append(POSSIBLE_CHARACTERS.charAt(RANDOM.nextInt(POSSIBLE_CHARACTERS.length())));
        }
        return password.toString();
    }

}

Top hit on Google looks very similar; in any case no need to reinvent this.

But really, if this is the initial password, the users are going to get it how? Are the being forced to reset it immediately? All that is more a question for https://security.stackexchange.com/ than here.

But sure, looks okay, AFAIK SecureRandom is thread-safe (is that important?) and it's going to produce "random enough" output, the loop is okay too.

Though why specifically 16 characters and not e.g. 32? Do they have to enter them from a sheet of paper? Then there would be a couple of issues with it, like possibly mistaking i, I, l, L and 1, or 0, o and O, ... and so on.


Loading text into an array when fields are in random order This script reads records from a CSV file of customer names and addresses. The first record of the file is the header (field names); the fields could be in any order, and sometimes have different variations on the field names.

I import the header into a one-dimensional array of field names, and use this loop to assign a variable to each column number:

' Read the first line from the file.
  TextLine = MyFile.ReadLine

' Split the line into an array
  arrCustomers = Split(TextLine, ",")

For i = 0 To  UBound(arrCustomers, 1)
    Select Case LCase(arrCustomers(i))
        case "customer name", "custname", "name"
              CustName  = i
        case "customer number", "custno","custnum"
              CustNo  = i
        case "address", "street"
              Address = i
        case "city"
              City = i
        case "country"
              Country = i
        case "state"
              State = i
        case else
            ' list of unused fields
            s = s & ", " & arrCustomers(i)
    End Select
Next

msgbox "These fields are unused: " & s

For instance, if "Customer Name" is in field 3, then CustName = 3 and arrCustomers(CustName) is the Customer Name field of the current record.

This loop processes each record:

Do While MyFile.AtEndOfStream <> True
  ' Read the first line from the file.
    TextLine = MyFile.ReadLine
  'Split the line into an array
    arrCustomers = Split(TextLine, ",")
  'Process the current record
    msgbox "Our customer " & arrCustomers(CustName) & " lives in " & arrCustomers(City) & ", " & arrCustomers(State)
Loop

My main problem is that the Case statements in the first loop must include every possible value for the field names. If the address field is called "Address1" or something else I didn't forsee, the Address variable becomes 0, and arrCustomers(Address) becomes the same as whatever's in the first column.

I suspect that there's a well-known algorithm somewhere for loading an array when the fields could be different each time – one that's better than my method. If not, maybe there's a way to map the "unused fields" on the fly?

A more compact and extensible solution would be to use pattern matching to try and take common parts of your field names and use them to map the uneven field headers you get in your csv files.

But I don't think this will work in your case, because you don't seem to have common patterns for all your field names. For instance, adress and street are field headers that should match to the Address field, but as you can see, there's no common usable pattern there. And in cases where there is a common pattern like cust, it won't work either because it could be for the fields CustName or CustNo.

So in this particular case I think you will definitively need to provide the full list of possible field names for each field. Now, you can do this in a more compact way using a VBScript dictionary that allows you to check in one line if certain field name (key) exists using dict.Exists(key), or see the field address matching a particular field name using dict.item(key). You still need to add all possible combinations of field names and addresses to the dictionary, but after doing that (just one time), you can use your dictionary very efficiently to get the address for all your headers.


I want to most efficiently use array values and variables in a WordPress template while abiding by PHP/WordPress best practices and standards Is it better to validate and put an array value into a variable at the top of a file or to check the array value directly right before output in the file?
I work at a WordPress shop and we have a function that some devs commonly use to check the value of indexes in an array of custom field values:
//Check if an array has a key and return its value if so
function pkav($arr,$key){
    return isset($arr[$key]) ? $arr[$key] : false;
}

This leads to the following code in a template:
<?php
    $options = get_fields('options');
    $footer_text = pkav($options, 'footer_text');
    $footer_button = pkav($options, 'footer_button');
?>

<?php if ( $footer_text || $footer_button ) : ?>

        <?php if( $footer_text ) : ?>

<?php echo esc_html( $footer_text ); ?>

        <?php endif; ?>
        <?php if( $footer_button ) : ?>
            <?php pk_output_button( $footer_button ); ?>
        <?php endif; ?>

<?php endif; ?>

Some devs think this is more readable and a better way to code and check values/variables. It also allows us to neatly declare all variables at the top of a file so we immediately know what variables are used in a given template/template part. We have other devs who think pkav is unnecessary and an incorrect way to use variables. They'd rather see code like this:
<?php
    $options = get_fields('options');
?>

<?php if ( ! empty( $options['footer_text'] ) || ! empty( $options['footer_button'] ) ) : ?>

        <?php if( ! empty( $options['footer_text'] ) ) : ?>

<?php echo esc_html( $options['footer_text'] ); ?>

        <?php endif; ?>
        <?php if( ! empty( $options['footer_button'] ) ) : ?>
            <?php pk_output_button( $options['footer_button'] ); ?>
        <?php endif; ?>

<?php endif; ?>

Is there a right answer on what should be used in terms of php/wordpress/programming best practices/standards? The pkav functions seems to create more readable code by eliminating duplicate empty() checks and shortening each line but using only empty() checks seems to prevent creating extra variables.

I would not use the pkav function. Beside the fact that the name means nothing (which was stated in the comments) it also restricts the default value to false.
This is wrong when you might expect strings or something else for a certain variable.
From the looks of it, the footer text and button vars look like strings.
I would go with this
<?php
    $options = get_fields('options');
    $footer_text = $options['footer_text'] ?? '';
    $footer_button = $options['footer_button'] ?? '';
?>

I would also go with declaring all variables at the top of the file. It makes the code more readable and will allow a frontend developer to focus on the html part instead of trying to navigate in the if statements.
<?php if ($something) :?>

<?= esc_html($something) ;?>

<?php endif;?>

is easier to read than
<?php if (!empty( $options['footer_text'])) : ?>

<?php echo esc_html($options['footer_text']); ?>

<?php endif; ?>


Separate roles into modular parts that are interdependent I have a IRC bot that I'm writing (code is here, bot24-irc.py is the main module).

This bot needs to be able to perform many different, separate roles so that each can be reloaded and sandboxed (for stability). I'm defining a role as being a function that reads all PRIVMSGs sent and returns a list of responses. At this moment, each role is in a separate module (just one right now, phablookup.py). I'm using the class role as a sort of wrapper to create a uniform way to "talk" to each module, to translate what the role modules want (a (different) function called with the right (different) arguments) to what the main module wants (a list of responses).

Here is the role class:

class role:
    run = False
    moduleName = ""
    checkFunc = ""
    args = []

    def init(self):
        if self.moduleName:
            try:
                print("Importing: " + self.moduleName)
                self.module = __import__(self.moduleName)
            except SyntaxError:
                print("Syntax error with " + self.moduleName + ", skipping...")
                self.run = False
                return 1
            except ImportError:
                print("Could not find " + self.moduleName + ", skipping...")
                self.run = False
                return 1
            else:
                self.funcToCall = getattr(self.module, self.checkFunc)
                return 0
        else:
            return 0

    def stop(self):
        self.run = False

    def start(self):
        self.run = True

    def check(self):
        if self.run:
            if self.moduleName:
                results = self.funcToCall(*self.args)
            else:
                results = self.checkFunc(*self.args)
            return results
        else:
            return ""

    def reload(self):
        if self.moduleName:
            try:
                reload(self.module)
            except SyntaxError:
                print("Syntax error with " + self.moduleName + ", skipping...")
                self.run = False
                return 1
            except ImportError:
                print("Could not find " + self.moduleName + ", skipping...")
                self.run = False
                return 1
            else:
                return 0
        else:
            return 0

And this is how I specify a new role:

phabLookup = role()
roles.append(phabLookup)
phabLookup.run = True
phabLookup.moduleName = 'phablookup'
phabLookup.checkFunc = 'lookup'
phabLookup.args = [msg, config['phabricator']['site'], config['phabricator']['apitoken']]

Now what I'm asking is this: is this type of module-per-role idea a good way to separate roles? Does it conform to best practices? It feels like its over complicating a really easy task. What is a better way to separate roles?

In Python 2.x, when creating classes, you need to explicitly have them inherit from object, like this: class MyClass(object):. If you're using Python 3.x, then you can write them like this: class MyClass:

Secondly, the general style for naming in Python is snake_case for functions and variables, and PascalCase for classes. If you have a constant variable, then it should be in UPPER_SNAKE_CASE.

The following line of code in your role.reload function can simply be changed to return 0:

else:
    return 0

This also applies to your role.check function. else: return "" can be changed to return "".

Also, I noticed that you're returning either 1 or 0 in your role.reload function. Do you mean to be returning True or False? This also applies to your role.init function as well.

Finally, in your role.check function, the below block of code can be changed to this: return self.funcToCall(*self.args) if self.moduleName else self.checkFunc(*self.args).

if self.moduleName:
    results = self.funcToCall(*self.args)
else:
    results = self.checkFunc(*self.args)
return results


SQL queries in a dataframe I want to get table names and column names from queries in a dataframe. The dataframe is like this:

Date         Query
29-03-2019   SELECT * FROM table WHERE ..
30-03-2019   SELECT * FROM ... JOIN ... ON ...WHERE ..
....         ....
20-05-2019   SELECT ...

and I run function to that dataframe to get tablename from the queries.

import sqlparse
from sqlparse.tokens import Keyword, DML

def getTableKey(parsed):
    findFrom = False
    wordKey = set(
        [
            "FROM",
            "JOIN",
            "LEFT JOIN",
            "INNER JOIN",
            "RIGHT JOIN",
            "OUTER JOIN",
            "FULL JOIN",
        ]
    )
    for word in parsed.tokens:
        if word.is_group:
            yield from getTableKey(word)
        if findFrom:
            if isSelect(word):
                yield from getTableKey(word)
            elif word.ttype is Keyword:
                findFrom = False
                StopIteration
            else:
                yield word
        if word.ttype is Keyword and word.value.upper() in wordKey:
            findFrom = True

def getTableName(sql):
    tableReg = re.compile(r"^.+?(?<=[.])")
    tableName = []
    query = sqlparse.parse(sql)
    for word in query:
        if word.get_type() != "UNKNOWN":
            stream = getTableKey(word)
            table = set(getWord(stream))
            for item in table:
                tabl = tableReg.sub("", item)
                tableName.append(tabl)
    return tableName

Also, I run function to get columnname from queries.

def getKeyword(parsed):
    kataKeyword = set(["WHERE", "ORDER BY", "ON", "GROUP BY", "HAVING", "AND", "OR"])
    from_seen = False
    for item in parsed.tokens:
        if item.is_group:
            yield from getKeyword(item)
        if from_seen:
            if isSelect(item):
                yield from getKeyword(item)
            elif item.ttype is Keyword:
                from_seen = False
                StopIteration
            else:
                yield item
        if item.ttype is Keyword and item.value.upper() in kataKeyword:
            from_seen = True

def getAttribute(sql):
    attReg = re.compile(r"asc|desc", re.IGNORECASE)
    namaAtt = []
    kueri = sqlparse.parse(sql)
    for kata in kueri:
        if kata.get_type() != "UNKNOWN":
            stream = getKeyword(kata)
            table = set(getWord(stream))
            for item in table:
                tabl = attReg.sub("", item)
                namaAtt.append(tabl)
    return namaAtt

But as this is my first try, I need an opinion about what I've tried, because my code runs slowly with a large file.

That will not speedup your code, but there are some code improvements:

follow naming conventions getAttribute -> get_attribute https://visualgit.readthedocs.io/en/latest/pages/naming_convention.html
You can create set using set literal my_set = {1, 2, 3}
You can compile tableReg = re.compile(r"^.+?(?<=[.])") once


Matplot lib : scalling of the axes on bar plot I am using following script to plot cluster populations using two lists contained x and y data
# calculate clusters using encore method
cluster = encore.cluster(u, select="all", superposition=False, method=encore.DBSCAN(eps=1))

# make list of population of each cluster;
cluster_sizes= [len(c) for c in cluster.clusters]
# make list of the cluster number;
cluster_numbers=[x+1 for x in range(0,len(cluster_sizes))]

# plot data
plt.bar(cluster_numbers, cluster_sizes)
plt.title('Cluster Populations')
plt.grid(True)
#plt.style.use('ggplot')
plt.savefig(f'clusters.png')
plt.close()

The problem that sometimes on the resulted bar plot the values are ranged by 0.5 along both X and Y (see the enclosed picture as the example). How I could plot only integers along the both axes? In my example graph the X should be scalled from 0 to 20 (avoiding 0.5 spacings) and the Y from 0 to 18 (thus avoiding these unused 0.5 spacings)

Use plt.xticks - https://matplotlib.org/3.1.1/api/_as_gen/matplotlib.pyplot.xticks.html.
One may to xticks np.arange(len(cluster.clusters)) as an argument
And for yticks it works in a same fashion -if you pass np.arange(int(max(cluster_sizes))), there will be integer ticks along the y-axis


Better way to extract country wise player list from html file using regex Problem statement:

Make country wise player list from the following html code

HTML CODE:

        Australia

Steven Smith

David Warner

        Bangladesh

Mashrafe Mortaza

Tamim Iqbal

        England

Eoin Morgan

Jos Buttler

Expected Output:

Australia- Steven Smith, David Warner

Bangladesh- Mashrafe Mortaza, Tamim Iqbal

England- Eoin Morgan, Jos Buttler

My Code:

import re

with open('playerlist.html', 'r') as f:
    text = f.read()

mytext = re.sub(r'[\n\t]', '', text)

pat = r'
(\w+?)

(\w+\s?\w+)

(\w+\s?\w+)
'

cpat = re.compile(pat)

result = cpat.findall(mytext)

for a,b,c in result:
    print('{0}- {1}, {2}'.format(a,b,c))

Regular expressions is not the right tool when it comes to parsing HTML. There are specialized HTML parsers that would do a better job resulting into a more robust and less fragile solution.

Just to name a few problems that exists in your current approach:

what if there are more than two players for a country
what if there are 0 players for a country
what if a country name contains a space or a single quote
what if a player's name consists of more than two words or contains a single quote
what if there are newlines after the opening or before the closing li tag

Instead, you may, for instance, use BeautifulSoup library:

from bs4 import BeautifulSoup

with open('playerlist.html', 'r') as input_file:
    soup = BeautifulSoup(input_file, "html.parser")

for country in soup.select("ul > li"):
    country_name = country.find(text=True, recursive=False).strip()
    players = [player.get_text(strip=True) for player in country.select("ol > li")]

    print('{country} - {players}'.format(country=country_name,
                                         players=', '.join(players)))


Finding the middle day of a month, given any day within that month Given a date, need to find the middle day of that day's month, or if there are an even number of days (with no exact middle day), the day should be the closest day to the middle of the month on the second half of the month.

For example, if the date given is August 2 (a month with 31 days), the middle day returned should be Aug 16.
If the date given is September 28 (a month with 30 days), the middle date returned should be 16 (15 would be closest day to middle in first half of month, 16 would be closest day to middle in second half of month).

Here's the code. Any better way to accomplish this with only version 2.4 and higher standard library and core?

#ruby 2.4.2
#require 'date'
def middleOfMonth(d)
  advancedMonth = d.next_month
  lastDayOfPreviousMonth = (Date.new(advancedMonth.year, advancedMonth.month, 1) - 1).day
  exactMiddle = lastDayOfPreviousMonth.to_f / 2
  actualMiddle = exactMiddle.floor + 1
  actualMiddle
end

Since February is the only month where the middle is 15 you could just write

d.month == 2 ? 15 : 16


Binary multiplication on elliptic curves in C++ I wrote a function in C++ which uses binary multiplication (a form of binary exponentation):

public: Point mul(Point p, unsigned int n)
{
    // actually these Points shouldn't have negative coordinates, I use Point(-1, -1) as neutral element on addition (like 0)
    Point r = Point(-1, -1);
    n = mod(n, m);  // the %-operator doesn't return a non-negative integer in every case so I wrote a function
    unsigned int mask = 1 << (sizeof (n) - 1);  // more flexible and platform independent than 1 << 31
    for (; mask; mask >>= 1)
    {
        r = add(r, r);
        if (mask & n)
            r = add(r, p);
    }
    return r;
}

Notes:

The function calculates points on elliptic curves over a finite field GF(m)
Point is a class I wrote for this. It doesn't do much beside holding two coordinates x and y
I just wondered if there is an easier / cleaner solution for binary multiplcation in C++ than I implemented
The algorithm is like 'double and add' instead of 'square and multiply'

EDITS:

This is my Point class:

class Point
{
    public:
        long long int x, y;

    public: Point(long long int _x, long long int _y)
    {
        x = _x;
        y = _y;
    }

    public: void print()
    {
        cout << "(";
        cout << x;
        cout << ", ";
        cout << y;
        cout ")";
    }
};

The mul function is part of the class EllipticCurve:

class EllipticCurve
{
    public:
        int a;
        int b;
        unsigned int m;

    public: EllipticCurve(int _a, int_b, unsigned int modul)
    {
        a = _a;
        b = _b;
        m = modul;
    }

    public: Point generate(unsigned long long int x)
    {
        // looks for Points on the curve with the given x coordinate
        // returns the first matching point
    }

    public: Point add(Point p, Point q)
    {
        // complex addition function with if-else trees
        // the function code is not needed for this question
    }

    public: Point mul(Point p, unsigned int n)
    {
        // see above
    }
};

Please remember that this question is about the mul function, not the rest of the code. I inserted it only for a better understanding.

n = mod(n, m);  // the %-operator doesn't return a non-negative integer in every case so I wrote a function

I would find a comment explaining m more useful than a comment explaining why you made a mod function, because everyone ends up writing a mod function which behaves sensibly.

I don't know much about elliptic curves, but I have worked with finite fields. Are you sure it makes sense to take n mod m as though they were integers? The only makes sense to me if m is also the characteristic (i.e. if it's a field of prime order).

    unsigned int mask = 1 << (sizeof (n) - 1);  // more flexible and platform independent than 1 << 31
    for (; mask; mask >>= 1)

Why loop down? If you loop up then you get the platform-independence for free, and also you loop fewer times when n is small. Obviously the loop invariant would change, but it would be closer to school long multiplication and so might also be more maintainable.

    for (; n; n >>= 1)
    {
        if (n & 1)
            r = add(r, p);
        p = add(p, p);
    }


Correctly applying the "rule of five" to a RAII socket wrapper I was trying to create a simple RAII wrapper with rule of 5 for a TCP POSIX socket. My aim was to try learn how to apply rule of five in different situations, but this one was somehow tricky.

class tcp_socket {
public:
    tcp_socket() {
        _s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
    }

    tcp_socket(const tcp_socket& other) = delete;
    tcp_socket& operator=(const tcp_socket& other) = delete;

    tcp_socket(tcp_socket&& other) noexcept {
        _s = other._s;
        other._s = -1;
    }

    tcp_socket& operator=(tcp_socket&& other) noexcept {
        if (this != &other) {
            _s = other._s;
            other._s = -1;
        }
        return (*this);
    }

    ~tcp_socket() {
        close(_s);
    }

private:
    int _s = -1;
};

In this class, I like to create socket in constructor and close it in destructor, a logical RAII approach. But I noticed closing socket in destructor creates a lot of side effects for writing other 4 routines of rule of five.

Move constructor/assignment:

As you see, I use other._s = -1; in move constructor/assignment. Because if I don't, when the moved object deletes, it will also close my socket in moved-to object. I think this will solve my problem, because when moved object will be destroyed, it will try to close -1 file descriptor which is an error, but will not ruin anything.

Copy constructor/assignment:

As you see, the only method that I could think of for copy constructor/assignment is deleting them. Because at first I thought to use dup() to duplicate file descriptor for copying, but if I do so, operations on one one instance (for example shutdown()) will affect another instance.

What do you think about this class? Do you think if this class does provide a simple RAII with correct rule of five or I need to adjust it somehow? Is it possible to write Copy constructor/assignment somehow or it is not logical to provide them?

These are good questions!

1. You should indeed have the copy constructor & assignment operator deleted.

The "rule of five" tells you to specifically define a copy c'tor and assignment operator - but it doesn't tell you that you have to make the available. It is a perfectly valid choice to decide to not allow your object to be copied or non-move-assigned - only moved. An example of this: std::unique_ptr.

It's also what I would recommend in your case, because:

Like you said, duplicating a file descriptor is weird and unexpected.
It's not obvious to the user of your class what the copy behavior should be.
There don't seem to be - AFAICT - common scenarios in which you would copy, rather than move or pass by reference, a TCP socket.

2. Consider using std::optional to indicate "no valid value" or "missing" or "none"

In C (and the C system call bindings on Unix-like systems), it is a convention to use the -1 value for an invalid/missing file descriptor is a convention. We know that the int type is actually larger than the actual space of possible file descriptor values, so we use a junk value, which we assume the OS never uses, to indicate "no valid value". Now, this works fine; and you could choose to, say, define a static class constant:

static constexpr const int no_file_descriptor { -1 };

and then write:

close(_s);
_s = other._s;
other._s = no_file_descriptor;

As @HolyBlackCat suggests, you must get _s to be closed somehow. You could also just swap the two descriptors, but I find that to contradict the element of least surprise.

but you might want to consider the more general (though less space-efficient) solution, which is the std::optional<T> type template. It is intended for exactly your case: Either holding some value of type T (in your case, int), or holding some indication of "no value". Using an optional, you could write:

close(_s.get());
_s = other._s;
other._s = std::nullopt;

You'll still need to write your move assignment and move construction code, unfortunately (thanks @CassioRenan for noticing these are both necessary).

Other suggestions

Don't use a plain int; either find a type definition of a file descriptor from some library you're using, or if you have no definition to borrow, have using file_descriptor = int; or using file_descriptor_index = int.
_s is a bad member name. Use something more explicit, e.g. descriptor_index_ or posix_descriptor_index_.
Always check the return value of library/system calls! And handle errors.

The modified code

class tcp_socket {
protected:
    void close_if_neccessary() {
       constexpr auto socket_close_failed { -1 };
        if (descriptor_index_.has_value()) {
            auto retval = close(descriptor_index_.value());
            if (retval == socket_close_failed) {
               // throw something here, e.g.:
               // throw std::system_error(errno, std::system_category(), "close()");
            }
    };
public:
    using file_descriptor_index = int;

    tcp_socket() : descriptor_index_() {
       constexpr auto socket_creation_failed { -1 };
       auto retval = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
       if (retval == socket_creation_failed) {
           // throw std::system_error(errno, std::system_category(), "socket()");
       }
       descriptor_index = retval;
    };

    tcp_socket(const tcp_socket& other) = delete;
    tcp_socket& operator=(const tcp_socket& other) = delete;
    tcp_socket& operator=(tcp_socket&& other) noexcept {
        if (other.descriptor_index_ != descriptor_index_) {
            close_if_neccessary();
            descriptor_index_ = other.descriptor_index_;
            other.descriptor_index_ = std::nullopt;
        }
        return this;
    };
    tcp_socket(tcp_socket&& other) noexcept :
        descriptor_index_(other.descriptor_index_)
    {
        other.descriptor_index = std::nullopt;
    }
    ~tcp_socket() {
        // You might want to wrap this in a try-catch, since
        // destructors shouldn't throw. Otherwise you're risking
        // a double-exception and immediate program termination.
        close_if_necessary();
    }

protected:
    std::optional<file_descriptor_index> descriptor_index_;
};

See this StackOverflow question about the weird exception code in the comments:


Rename and move images to a destination directory based on a text file followup This is my third (and hopefully final) time re-writing this program. It was recommended I use Pathlib and it simplified my code, it's easier to read and understand than the first two monstrosities.

I also changed the way it functions, instead of the user providing a text file, the user creates a directory called Repository, which contains sub directories of images that will be renamed, and a destination directory (R:\Pictures for me).

Suppose I have the following directories under Repository:

C:\Users\Kiska\Repository\Computers
   - comp-amd-12342.jpg
   - 7546345233-pcmag.jpg

C:\Users\Kiska\Repository\Landscape
   - land-345345.jpg
   - end-field.jpg

The program will move and rename it like this:

R:\Pictures\Pictures\Computers
   - computers_1.jpg
   - computers_2.jpg

R:\Pictures\Pictures\Landscape
   - landscape_1.jpg
   - landscape_2.jpg

Code:

import os
from pathlib import Path
import shutil

def check_if_folder_exists(folder : Path):
    if not Path.exists(folder):
        raise FileNotFoundError("{0} is not available".format(folder))

def main():

    try:

        repository = Path("{0}\\{1}".format(os.getenv("USERPROFILE"), "Repository"))
        desination_root = Path("R:\\Pictures")

        check_if_folder_exists(folder=repository)
        check_if_folder_exists(folder=desination_root)

        for folder in repository.iterdir():
            destination = Path(desination_root).joinpath(folder.stem)
            Path.mkdir(destination, exist_ok=True)

            start = len(os.listdir(destination)) + 1

            for number, image in enumerate(folder.iterdir(), start=start):
                image_name = "{0}_{1}{2}".format(folder.stem.lower().replace(" ","_"), number,image.suffix)
                destination_image = Path(destination).joinpath(image_name)
                print(f"{image} will be named {destination_image}")
                shutil.move(image, destination_image)

    except(ValueError, FileNotFoundError, OSError) as error:
        print(error)
    finally:
        pass

if __name__ == '__main__':
    main()

You have lots of unneeded empty lines, they also hinder readability.
fn(arg=value) is abnormal. It's common to use fn(value). It's common only to use the former when the argument is a keyword or default argument.
You don't need to make a path another path.

desination_root = Path("R:\\Pictures")
destination = Path(desination_root).joinpath(folder.stem)

Path.mkdir isn't a static method, and so it's abnormal to use it as Path.mkdir(my_path) rather than my_path.mkdir().
check_if_folder_exists isn't needed as repository.iterdir and destination.mkdir will error if they don't exist.

It should also be noted that you can pass parents=True to Path.mkdir to automatically create the folder if it doesn't exist, rather than force the user to do that.
It's better if you make another function that moves the files, and one, main, that passes it the values. Preferably main would be changed to not use static values and get the values from a config file.
finally isn't needed.
You should raise a SystemExit(1) or use sys.exit(1) if there is an error. They both do the same thing which notifies the system that the program failed. It didn't run without problems.
Given that generating a name that doesn't conflict with existing files is not a simple one-liner I'd recommend you move it out into a function.

I should note that it may overwrite files, if you have a folder with file_1.png and file_3.png your code's going to overwrite file_3.png, but not have a file_2.png.

You changed away from some of the above, and so I'd recommend you think about the differences, which is easier to re-use, which is easier to read, which performs the correct action in the situation. If you don't know what the difference is even with my explanation, such as with SystemExit(1), then you should look up what it does.

import os
from pathlib import Path
import shutil

def gen_name(path, number):
    orig_name = path.parent.name.lower().replace(" ", "_")
    return f"{orig_name}_{number}{path.suffix}"

def move_files(src, dest):
    for folder in Path(src).iterdir():
        destination = Path(dest) / folder.stem
        destination.mkdir(exist_ok=True, parents=True)
        start = len(os.listdir(destination)) + 1
        for number, image in enumerate(folder.iterdir(), start=start):
            image_name = gen_name(image, number)
            destination_image = destination / image_name
            print(f"{image} will be named {destination_image}")
            shutil.move(image, destination_image)

def main():
    try:
        move_files(
            "{0}\\{1}".format(os.getenv("USERPROFILE"), "Repository"),
            "R:\\Pictures",
        )
    except(ValueError, FileNotFoundError, OSError) as error:
        print(error)
        raise SystemExit(1) from None

if __name__ == '__main__':
    main()

I think it could make more sense for gen_name to be a closure, as then it can correctly handle existing files. I won't change how the code functions, but will show how it could be implemented.

def gen_name_builder(folder):
    orig_name = folder.name.lower().replace(" ", "_")
    number = len(os.listdir(destination))
    def gen_name(file):
        nonlocal number
        number += 1
        return f"{orig_name}_{number}{path.suffix}"
    return name

def move_files(src, dest):
    for folder in Path(src).iterdir():
        destination = Path(dest) / folder.stem
        destination.mkdir(exist_ok=True, parents=True)
        gen_name = gen_name_builder(destination)
        for image in folder.iterdir():
            image_name = gen_name(image)
            destination_image = destination / image_name
            print(f"{image} will be named {destination_image}")
            shutil.move(image, destination_image)