Refactoring ViewModel and mapping from multiple sources I need to refactor and tidy the following controller below, it's my first MVC application. I've successfully used AutoMapper as I began refactor controllers but I am not sure how to approach a scenario where the data for the ViewModel comes from multiple service calls. And that scenario seems to often present itself in slightly heavier view models where I have all kind of information to present.

Calling multiple mapping calls on top of the same view model seemed counter-intuitive and it feels like I am missing some good practice or common approach to the situation. Or maybe I am wrong and it's commonly used approach.

0) Thought of generally using AutoMapper somehow but as I said I cannot find a proper way to implement it, I'd like to keep the controller relatively thin but I also don't want to have too much unneeded/overkill abstraction.

1) Thought of having some build method on the view model and to pass needed data.

2) Thought of having a model between my database entities and the view model but I am not sure how to execute that either, I think I seen somewhere by examples that people return "Models" from service calls that contain complex data and then map those Models to the ViewModel with Automapper, instead of mapping service calls directly into the ViewModel like others are doing.

3) Saw people use a facade of sorts in one example and thought of that too.

It's just my first project under MVC and I would like to learn a robust general approach that is considered well written and good practice so I can use it to fully refactor this and continue to improve from there.

        BKQEventCustomData BKQEventData = BKQHelper.ParseBase64EventData(Server.UrlDecode(Request.Url.Query));
        var cutomData = JsonConvert.DeserializeObject<NopOrderData>(BKQEventData.custom.data.ToString());

        var model = new TaskEventViewModel();
        if (BKQEventData != null)
        {
            AuthenticateRequest(BKQEventData.xcm.clid, BKQEventData.xcm.opid);

            model.PlacesList = new SelectList(_placesService.GetPlacesByClientId(BKQProfile.ClientId).Where(p => p.Active), "PlaceId", "Name");
            model.ServicesList = new SelectList(_serviceService.GetServicesByClientId(BKQProfile.ClientId).Where(s => s.Active), "ServiceId", "Name");
            model.ContractorsList = new SelectList(_contractorService.GetContractorsByClientId(BKQProfile.ClientId), "ContractorId", "BKQFullName");
            model.StatusList = new SelectList(_statusService.GetStatusByClientId(BKQProfile.ClientId), "StatusId", "Name");

            model.TaskEvent = new TaskEvent();
            model.TaskEvent.Title = string.Empty;
            model.TaskEvent.BKQCustomData = BKQEventData.custom.data.ToString();
            model.TaskEvent.BKQCustomDataType = BKQEventData.custom.type.ToString();
            model.TaskEvent.BKQClientId = BKQEventData.xcm.clid;
            model.TaskEvent.BKQOperatorId = BKQEventData.xcm.opid;
            model.TaskEvent.BKQInfProductId = BKQEventData.xcm.iprodid;
            model.TaskEvent.BKQEventHistoryId = BKQEventData.xcm.evnhid;
            model.TaskEvent.BKQEventTypeId = BKQEventData.xcm.evntype;
            model.TaskEvent.BKQEventCaseId = BKQEventData.xcm.caseid;
            model.TaskEvent.BKQQuota = BKQEventData.xcm.quotaid;
            model.TaskEvent.BKQLangId = BKQEventData.xcm.langid;
            model.TaskEvent.BKQContactId = BKQEventData.xcm.contid;
            model.TaskEvent.CreatedBy = BKQEventData.xcm.opid;
            model.TaskEvent.ModifiedBy = BKQEventData.xcm.opid;
            model.BKQContact = BKQFramework.Contact.GetById(BKQEventData.xcm.contid);
        }
        else
        {
            return RedirectToActionPermanent("Login", "Login");
        }

When you have multiple sources of data you can still combine them into a single ViewModel in the Controller.  If you have really "heavy" ViewModels that's an indication that you're displaying a lot of data at once, maybe you can find a way to break that up.  Sometimes you can't due to requirements, but if you can that'd make both your life and your user's life easier (they wouldn't have to see/think about as much either).

In this case, your ViewModel doesn't look like it has that many properties: PlacesList, ServicesList, ContractorsList, StatusList, TaskEvent, and BKQContact.  ViewModels for standard contact forms can have more properties.  For example: FirstName, LastName, MiddleInitial, AddressLine1, AddressLine2, City, State, ZipCode, PhoneNumber.  I don't think anyone would bat an eye over those properties in a ViewModel.

The first thing I'd do is reverse the if condition and move it right under the first line, where you're initializing BKQEventData.  var cutomData... relies on BKQCustomData, but it's using it before the null check; if BKQEventData is null it looks like you'll get a Null Pointer Exception (NPE).  Reversing the conditional is a matter of preference, but it prevents some indentation in some cases.  So:

BKQEventCustomData BKQEventData = BKQHelper.ParseBase64EventData(Server.UrlDecode(Request.Url.Query));

if (BKQEventData == null)
{
    return RedirectToActionPermanent("Login", "Login");
}

The next big thing I see is how many lines you have dedicated to setting up the TaskEvent.  I'm guessing the TaskEvent is something you've defined in your project.  With the exception of the Title property, all thirteen properties are derived from BKQEventData.  You could add a method (or maybe a constructor) to TaskEvent that accepts a BKQEventCustomData and returns a TaskEvent.  Or, if you don't have access to TaskEvent, you could make an extension method:

public static class TaskEventExtensions
{
    public static void InitalizeWithBKQEventCustomData(this TaskEvent taskEvent, BKQEventCustomData customData)
    {
        taskEvent.BKQCustomData = customData.custom.data.ToString();
        taskEvent.BKQCustomDataType = customData.custom.type.ToString();
        taskEvent.BKQClientId = customData.xcm.clid;
        taskEvent.BKQOperatorId = customData.xcm.opid;
        taskEvent.BKQInfProductId = customData.xcm.iprodid;
        taskEvent.BKQEventHistoryId = customData.xcm.evnhid;
        taskEvent.BKQEventTypeId = customData.xcm.evntype;
        taskEvent.BKQEventCaseId = customData.xcm.caseid;
        taskEvent.BKQQuota = customData.xcm.quotaid;
        taskEvent.BKQLangId = customData.xcm.langid;
        taskEvent.BKQContactId = customData.xcm.contid;
        taskEvent.CreatedBy = customData.xcm.opid;
        taskEvent.ModifiedBy = customData.xcm.opid;
    }
}

Going by this snippet I don't think that cutomData is used anywhere; if you're not using it, get rid of it.  Dead code is a little messy and can make things confusing.

You could also use object initialization notiation to set up the majority of the ViewModel.  It's a bit of a matter of preference, but I like it.  Something along the lines of:

var model = new TaskEventViewModel {
    BKQContact = BKQFramework.Contact.GetById(BKQEventData.xcm.contid),
    ContractorsList = new SelectList(_contractorService.GetContractorsByClientId(BKQProfile.ClientId), "ContractorId", "BKQFullName"),
    PlacesList = new SelectList(_placesService.GetPlacesByClientId(BKQProfile.ClientId).Where(p => p.Active), "PlaceId", "Name"),
    ServicesList = new SelectList(_serviceService.GetServicesByClientId(BKQProfile.ClientId).Where(s => s.Active), "ServiceId", "Name"),
    StatusList = new SelectList(_statusService.GetStatusByClientId(BKQProfile.ClientId), "StatusId", "Name"),
    TaskEvent = new TaskEvent {
        Title = string.Empty
    }
};

model.TaskEvent.InitalizeWithBKQEventCustomData(BKQEventData);

Although I would probably pull the SelectLists out into their own variables.  They're a little long, but you'd have to figure out how to best name/break them up.

Something else that I'd look at offhand is what happens when AuthenticateRequest fails?  I have no way to tell in this snippet, but it doesn't look like it's handled.

So taking a pass at this using the extension method I mentioned earlier, this is what I got.  It's a bit of a readability improvement to me.  Again, the biggest thing that still stands out at me is how long those new SelectList lines are; I'd definitely break those into their own lines with a variable describing what exactly they are.  Maybe something like:

BKQEventCustomData BKQEventData = BKQHelper.ParseBase64EventData(Server.UrlDecode(Request.Url.Query));

if (BKQEventData == null)
{
    return RedirectToActionPermanent("Login", "Login");
}

AuthenticateRequest(BKQEventData.xcm.clid, BKQEventData.xcm.opid);

var clientsContractors = new SelectList(_contractorService.GetContractorsByClientId(BKQProfile.ClientId), "ContractorId", "BKQFullName");
var activePlaces = new SelectList(_placesService.GetPlacesByClientId(BKQProfile.ClientId).Where(p => p.Active), "PlaceId", "Name");
var activeServices = new SelectList(_serviceService.GetServicesByClientId(BKQProfile.ClientId).Where(s => s.Active), "ServiceId", "Name");
var clientStatuses = new SelectList(_statusService.GetStatusByClientId(BKQProfile.ClientId), "StatusId", "Name");

var model = new TaskEventViewModel {
    BKQContact = BKQFramework.Contact.GetById(BKQEventData.xcm.contid),
    ContractorsList = clientsContractors,
    PlacesList = activePlaces,
    ServicesList = activeServices,
    StatusList = clientStatuses,
    TaskEvent = new TaskEvent {
        Title = string.Empty
    }
};

model.TaskEvent.InitalizeWithBKQEventCustomData(BKQEventData);


Design of genetic algorithm which would allow TDD I'm implementing genetic algorithm in Java and I want to learn TDD with this project.

Currently I have this code:

package geneticAlgoritm;

import geneticAlgoritm.randomNumbers.IRandomGenerator;
import geneticAlgoritm.randomNumbers.RandomGenerator;
import lombok.Getter;
import lombok.NonNull;

import java.util.List;

public class Population {
    @NonNull private IRandomGenerator generator = new RandomGenerator();

    @Getter
    @NonNull
    private List<Chromosome> population;

    @NonNull final IFitnessCalculator calculator;

    public Population(int n, IFitnessCalculator calculator) {
        this.calculator = calculator;
        this.population = generatePopulation(n);
    }

    int nextGeneration() {

    }

    private List<Chromosome> generatePopulation(int n) {
        return null;
    }

    private Chromosome mutate(Chromosome chromosome, double probability) {
        return null;
    }

    private Chromosome crossover(Chromosome first, Chromosome second, double probability) {
        return null;
    }

    private List<Chromosome> pickBestOfPopulation(int n) {
        return null;
    }
}

Basically, I have Chromosome class, which is wrapper for byte[] array and fitness of that array.

nextGeneration generates new population with better traits. How I can change design to allow testing crossover, mutate etc functions?

nextGeneration is really inconvenient to test for all functionalities, especially with some randomness in each (I'm using custom interface to remove it).

You have already written the API of your implementation. Being strict: That's not how tdd works.

Write one failing test
Make the test pass
Refactor

Your test case should not only test your implementation, but also document the behaviour of your implementation - without having written the implementation. And if you're at the first point, your test case describes the expected behaviour of your implementation. And it guides the design of your implementation.

And what's very important, too: Know when to stop. If you have code which is not covered by your test cases - and should be covered by a test case - at the end of an iteration, you did it wrong.

And in my opinion: A code review does help, but not too much. Because with tdd, an application grows iteration by iteration. And the thoughts which have to be thought during those steps have to be learned and practiced.


Generator for the collatz conjecture sequence I tried to write this code as concisely as possible. Is this the best way to do it?

def collatz(n):
    """
    Generator for collatz sequence beginning with n

    >>> list(collatz(10))
    [5, 16, 8, 4, 2, 1]
    """
    while n != 1:
        n = n / 2 if n % 2 == 0 else 3*n + 1
        yield int(n)

The only improvement I see here is to divide n by 2 using // (since we are dealing with Python 3.x) and to remove the explicit conversion to int (int(n)):

while n != 1:
    n = n // 2 if n % 2 == 0 else 3*n + 1
    yield n

Also, I suggest you put a single space before and after the multiplication operator in 3*n, so that it becomes 3 * n.

Hope that helps.


Generating a 1-dim array with explanatory arrows and text and large braces I am still learning tikz and often have to use complicated solutions where I am sure are more simple ones. From my latest tikz files I picked one to ask for tips what I can do better, to learn about features of tikz.

The general layout should stay the same. Slight changes are fine. Suggestions should be about syntax.

I convert this file using

pdflatex 1.tex

and the result is a pdf/png like this

General prologue for all my tikz PNGs

\documentclass[11pt]{article}
\usepackage[utf8]{inputenc}
\usepackage[ngerman]{babel}
\usepackage{tikz}
\usepackage{pgf}
\usepackage[active,pdftex,tightpage]{preview}
  \usetikzlibrary{backgrounds}
  \usetikzlibrary{decorations.pathreplacing}
  \usetikzlibrary{positioning}
  \usetikzlibrary{shapes}
  \usetikzlibrary{shapes.multipart}
  \usetikzlibrary{trees}
  \usetikzlibrary{arrows}
\definecolor{gpcHintergrund}{RGB}{205,229,244}
\PreviewEnvironment[]{tikzpicture}
\PreviewEnvironment[]{pgfpicture}

\begin{document}

tikz intro

\begin{tikzpicture}[
      background rectangle/.style={fill=gpcHintergrund}, show background rectangle,
      every node/.style={align=center, font=\small, minimum height=1.5em, minimum width=1.0cm,node distance=0cm},
        line/.style={ draw, line width=3pt },
        cell/.style = {rectangle, fill=white, draw, text width=1.0cm,outer sep=0pt},
        capx/.style = {rectangle, draw, text width=1.3cm, color=black!60,outer sep=0pt},
      scale=0.95,
      every node/.append style={transform shape},   % (1) scale here first approx. lines stay.
    %  every node/.append style={draw},
    ]

Center nodes/boxes

   \node at (0,0) [cell] (n1) {10};
   \node[right=of n1,cell] (n2) {20};
   \node[right=of n2,cell] (n3) {30};
   \node[right=of n3,cell] (n4) {40};
   \node[right=of n4,cell] (n5) {50};
   \node[right=of n5,cell] (n6) {60};
   \node[right=of n6,cell] (n7) {70};
   \node[right=of n7,cell] (n8) {80};
   \node[right=of n8,cell,fill=black!20,text=black!60] (ne) {end};

Could this series of right=ofs be replaced by a totally different approach?

The text above and below with arrows

\node[above=of n2.west,yshift=1.0cm] (lb2) {lower\_bound(20)};
  \path [->,draw,shorten <=-1pt] (lb2) -- (n2.north west);

\node[above=of n7.west,yshift=1cm] (ub7) {upper\_bound(60)};
  \path [->,draw,shorten <=-1pt] (ub7) -- (n7.north west);

\node[above=of n4,yshift=0.5cm] (er4) {equal\_range(40)};
  \path [->,draw,shorten <=-3pt] (er4) -- (n4.north west);
  \path [->,draw,shorten <=-3pt] (er4) -- (n5.north west);

\node[below=of n3.west,yshift=-1.0cm] (lb25) {lower\_bound(25)};
  \path [->,draw,shorten <=-1pt] (lb25) -- (n3.south west);

\node[below=of n7.west,yshift=-1.0cm] (ub65) {upper\_bound(65)};
  \path [->,draw,shorten <=-1pt] (ub65) -- (n7.south west);

\node[below=of n4.east,yshift=-0.7cm] (er45) {equal\_range(45)};
  \path [->,draw,shorten <=-3pt] ([xshift=0.4ex]er45.north) -- ([xshift=0.4ex]n5.south west);
  \path [->,draw,shorten <=-3pt] (er45.north) -- (n5.south west);

equal_range(40) could have two straight/parallel arrows, not diagonal. Their tips must point exactly where they are now, but the backs could move apart until parallel. I know about |- and -| but did not manage to straighten the lines.

The curly braces "gefunden" and "nicht gefunden"

\draw[decoration={brace},decorate,color=black!50]
  ([yshift=1.3cm]ne.north west) -- node[right=6pt] {gefunden} ([yshift=0.2cm]ne.north west);

\draw[decoration={brace,mirror},decorate,color=black!50]
  ([yshift=-1.3cm]ne.south west) -- node[right=6pt] {nicht gefunden} ([yshift=-0.2cm]ne.south west);

\end{tikzpicture}

\end{document}

The placing of the braces using yshifts looks very bad to me. What could be a better way?

So here are some points. Do ask if you need clarification:

tikz loads pgf, so having \usepackage{pgf} is redundant.
Personally I'd say only load the libraries you need. You may also use a single \usetikzlibrary, but that is I think just a matter of preference.
The arrows.meta library has replaced arrows (the latter still works, but is considered deprecated, see the manual)
Instead of the right=of, you can use the chains library, and place the nodes on a chain, using a loop to reduce the code if desired. There are other methods of achieving the same thing I think, but chains works fine here.
An alternative of the yshifts and xshifts is to use relative coordinates, e.g. ++(1cm,0) is the point 1cm to the right of the previous active coordinate.
For the vertical arrows: The coordinate (a-|b) has the y-coordinate of a and the x-coordinate of b. See how it's used in the code.
Instead of having above=of x,yshift=y, use above=y of x, e.g. above=1cm of n2.west.
I also added an additional style for the arrows, where the length for shorten < is made an argument. In myarrow/.style={->,shorten <=#1}, #1 indicates the argument, and the style is used as myarrow={<some length>}.
I modified \path[draw] to \draw.
For e.g. lower_bound(20) you could alternatively make the arrow and node in the same path, i.e.

\draw [myarrow=-1pt] (n2.north west) -- ++(0,0.7cm) node[above] {lower\_bound(20)};

This is merely a matter of preference though. One might find it clearer to separate the nodes from the arrows, as in your original code.
The standalone class is very handy for making PDFs consisting of only a tikzpicture.

\documentclass[11pt,border=1pt]{standalone}
\usepackage[utf8]{inputenc}
\usepackage[ngerman]{babel}
\usepackage{tikz}
\usetikzlibrary{
  backgrounds,
  decorations.pathreplacing,
  chains}
\definecolor{gpcHintergrund}{RGB}{205,229,244}

\begin{document}
\begin{tikzpicture}[
      background rectangle/.style={fill=gpcHintergrund}, show background rectangle,
      every node/.style={align=center, font=\small, minimum height=1.5em, minimum width=1.0cm,node distance=0cm},
        line/.style={ draw, line width=3pt },
        cell/.style = {rectangle, fill=white, draw, text width=1.0cm,outer sep=0pt,on chain}, % added on chain
        capx/.style = {rectangle, draw, text width=1.3cm, color=black!60,outer sep=0pt},
        myarrow/.style={->,shorten <=#1},
      scale=0.95,
      every node/.append style={transform shape},   % (1) scale here first approx. lines stay.
    ]

\begin{scope}[start chain]
\foreach [count=\i] \x in {10,20,...,80}
  \node [cell] (n\i) {\x};

\node[cell,fill=black!20,text=black!60] (ne) {end};
\end{scope}

\node[above=1cm of n2.west] (lb2) {lower\_bound(20)};
  \draw [myarrow=-1pt] (lb2) -- (n2.north west);

\node[above=1cm of n7.west] (ub7) {upper\_bound(60)};
  \draw [myarrow=-1pt] (ub7) -- (n7.north west);

\node[above=0.5cm of n4] (er4) {equal\_range(40)};
  \draw [myarrow=-3pt] (er4.south-|n4.north west) -- (n4.north west);
  \draw [myarrow=-3pt] (er4.south-|n5.north west) -- (n5.north west);

\node[below=1cm of n3.west] (lb25) {lower\_bound(25)};
  \draw [myarrow=-1pt] (lb25) -- (n3.south west);

\node[below=1cm of n7.west] (ub65) {upper\_bound(65)};
  \draw [myarrow=-1pt] (ub65) -- (n7.south west);

\node[below=0.7cm of n4.east] (er45) {equal\_range(45)};
  \draw [myarrow=-3pt] (er45.north) ++(0.45ex,0) coordinate (tmp) -- (tmp|-n5.south west);
  \draw [myarrow=-3pt] (er45.north) -- (n5.south west);

\draw[decoration={brace},decorate,color=black!50]
  (ne.north west) ++(0,1.3cm) -- node[right=6pt] {gefunden} ++(0,-1.1cm);

\draw[decoration={brace,mirror},decorate,color=black!50]
  (ne.south west) ++(0,-1.3cm) -- node[right=6pt] {nicht gefunden} ++(0,1.1cm);

\end{tikzpicture}
\end{document}


Windows application that draws India's flag I have written a small Windows application that draws India's flag in the window. I am very new to using Visual C++ and I want to understand if my code can be improved further.

#include "afxwin.h"
#include <math.h>

class CMyApp : public CWinApp
{
public:
    virtual BOOL InitInstance ();
};

class CMainWindow : public CFrameWnd
{
public:
    CMainWindow();
protected:
    afx_msg void OnPaint();
    DECLARE_MESSAGE_MAP();
};

CMyApp myAPP;

BOOL CMyApp::InitInstance()
{
     m_pMainWnd = new CMainWindow;
     m_pMainWnd->ShowWindow(m_nCmdShow);
     m_pMainWnd->UpdateWindow();
     return TRUE;
}

BEGIN_MESSAGE_MAP (CMainWindow, CFrameWnd)
    ON_WM_PAINT()
    END_MESSAGE_MAP()

CMainWindow::CMainWindow ()
{
    Create(NULL,_T("India's Flag"), WS_OVERLAPPEDWINDOW );
}
void IndiaFlag(CDC &dc, int x, int y)
{
    dc.SetBkMode(TRANSPARENT);
    CRect rect;
    CPen pen(PS_SOLID, 1, RGB(0,0,0));
    CPen *oldPen = dc.SelectObject(&pen);
    {
        CBrush brush(RGB(255,130,0));
        CBrush *oldBrush = dc.SelectObject(&brush);
        dc.Rectangle(x,y,x+200,(y+50));
    }
    {
        CBrush brush(RGB(255,255,255));
        CBrush *oldBrush = dc.SelectObject(&brush);
        dc.Rectangle(x,(y+50),(x+200),(y+100));
        CPen pen2(PS_SOLID, 1,RGB(0,0,255));
        CPen *oldPen = dc.SelectObject(&pen2);
        dc.Ellipse((x+75),(y+50),(x+125),(y+100));
        double Nx,Ny;
        for (int angle=0;angle<360; angle+=15)
        {
            int angle2 = angle;
            double length = 25*(cos(double(angle2 *(3.14159265 / 180))));
            double len2 = 25*(sin(double(angle2 *(3.14159265 / 180))));
            int originX = (x+100);
            int originY = (y+75);
            Nx = originX + length;
            Ny = originY - len2;
            dc.MoveTo(originX,originY);
            dc.LineTo(Nx,Ny);
        }
    }
    {
        CBrush brush(RGB(34,139,34));
        CBrush *oldBrush = dc.SelectObject(&brush);
        dc.Rectangle(x,(y+100),(x+200),(y+150));
    }
}
void CMainWindow::OnPaint ()
{
    CPaintDC dc(this);
    IndiaFlag(dc, 150, 150);
}

In case you wondered, this is what the Indian flag looks like:

Thanks to your method IndiaFlag, I guess you're drawing a flag of India. The problem is, I don't know what it looks like. OK, I gonna google it.

It consists of 3 stripes: orange, white and green (top to bottom) and a sort of a wheel in the center. So, why don't you just say:

int const width = ...
int const height = ...
int const stripeHeight = height / 3;

drawStripe(0, 0, width, stripeHeight, Orange);
drawStripe(0, stripeHeight, width, 2 * stripeHeight, White);
drawStripe(0, 2 * stripeHeight, width, 3 * stripeHeight, Green);
drawWheel(width / 2, height / 2)

?

This code describes what your're drawing in fact. There are no brushes or pencils here, because if I ask you what the flag looks like, for sure you're not going to tell me about pencils and brushes and how GDI works. You'll probably say there are 3 stripes and a wheel in the center and that's what I want to hear.

So, why do you write the code like you're not going to tell anyone what happens in there?

Looking at your code, I'd say that all the names you give are terrible:

CBrush brush(RGB(255,255,255));

What does it mean? You're only going to have a single brush, so that's why you call it a "brush"? Nothing specific about it? If I see a name brush 20 lines later, do you expect me to remember it is white or any specific at all?

CBrush *oldBrush = dc.SelectObject(&brush);

An old brush? When I see oldBrush 20 lines later, do you expect me to remember what this brush is? Do YOU know why you need this oldBrush? I see no code where you switch back to it, so why do you even do it?

    dc.Rectangle(x,(y+50),(x+200),(y+100));

What is x here? Why y+50? What is 500? What is 200?

    CPen pen2(PS_SOLID, 1,RGB(0,0,255));

And once again, something called pen2. Is this pen any specific, or you're just telling me you're using 2 pens? Should I care? Etc.

P.S. Procedures/Functions/Methods describe actions, so their names are traditionally DoWhat. Your IndiaFlag violates the principle, because it says What.


PDO anonymous function inside a Factory In this question here on S.O, the accepted answer suggests to use both anonymous function and factory pattern for dealing with PDO connection. I believe the anonymous function is used in case a connection to a different database needs to be established, a different function will be defined for that. In that case, will it be alright to move the anonymous function to the factory class itself? With this approach  just passing the PDO parameters to the constructor will achieve the same as the original answer.

Something like:

class StructureFactory
{
    protected $provider = null;
    protected $connection = null;

    public function __construct( $PDO_Params )
    {
        $this->provider = function() {
            $instance = new PDO($PDO_Params["dsn"], $PDO_Params["username"], $PDO_Params["password"]);
            $instance->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $instance->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
            return $instance;
        };
    }

    public function create( $name)
    {
        if ( $this->connection === null )
        {
            $this->connection = call_user_func( $this->provider );
        }
        return new $name( $this->connection );
    }

}

TL;DR: no.

The construction that is shown in the original example provided the following functionality:

decoupling from MySQL (you dont, need disable emulation of prepared statements, if you are not using mysql, and you usually dont need to provide username and password with SQLite)
option to use MySQLi or SOAP or any other DAL without rewriting the factory (see OCP)
replacing PDO with a mock object, when running unit tests
a way to alter the anonymous function, so that you can share the same PDO instance among multiple factories (when appropriate changes added)
establishing a connection to data source, only when it is necessary (connecting to SOAP can be really slow, and RDBMS under heavy load can actually run out of "connections" that can be opened)


Finding the kth to last element of a singly linked list I'm not sure if this code is industry-standard or not. Please review it and let me know if any improvement is required.

class Node(object):
   def __init__(self,data,next):
        self.data=data
        self.next=next

class getLinkRest():
       head = None
       tail= None

   def insert(self,data):
      node=Node(data,None)
     if self.head is None:
       self.head=self.tail=node
     else:
       self.tail.next=node
     self.tail=node

 def showFromItem(self,data):
     curr_node=self.head
     flag=None
     while curr_node is not None:
          if curr_node.data == data or curr_node.data > data:
            print curr_node.data
            flag =1
            curr_node=curr_node.next
    if(flag is None):
       print "Not exist"

  s= getLinkRest()
  s.insert(1)
  s.insert(2)
  s.insert(3)
  s.insert(4)
  s.insert(5)
  s.showFromItem(3)
  s.showFromItem(7)

Fix the indentation

Before I say anything, let's reformat your code,
for everyone's sake,
because as you posted it, it just doesn't work,
because indentation is crucial in Python.
Please be more careful next time.

class Node(object):
    def __init__(self, data, next):
        self.data = data
        self.next = next

class getLinkRest():
    head = None
    tail = None

    def insert(self, data):
        node = Node(data, None)

        if self.head is None:
            self.head = self.tail = node
        else:
            self.tail.next = node
        self.tail = node

    def showFromItem(self, data):
        curr_node = self.head
        flag = None
        while curr_node is not None:
            if curr_node.data == data or curr_node.data > data:
                print curr_node.data
                flag = 1
            curr_node = curr_node.next

        if flag is None:
            print "Not exist"

s = getLinkRest()
s.insert(1)
s.insert(2)
s.insert(3)
s.insert(4)
s.insert(5)
s.showFromItem(3)
s.showFromItem(7)

Purpose of the code

  Finding the kth to last element of a singly linked list

This is not what the posted code does.
The showFromItem function searches for an element with the specified value,
and prints that element and the rest of the elements until the end of the linked list that are bigger than the the specified value, or else it prints the text "Not exist".
For example:

s.insert(11)
s.insert(12)
s.insert(13)
s.insert(4)
s.insert(15)
s.showFromItem(13)

The above code will print 13 and 15.

That's a very unusual logic, and somehow I doubt that this was really your intention.

It's good to have a clear purpose, and to include a clear problem statement in a question.

Unused constructor argument

The next value of the constructor is never used.
So you could simplify it:

class Node(object):
    def __init__(self, data):
        self.data = data
        self.next = None

Instead of using flag with None and 1 as values,
it would be more natural to make it a boolean, like this:

    found = False
    while curr_node is not None:
        if curr_node.data == data or curr_node.data > data:
            print(curr_node.data)
            found = True
        curr_node = curr_node.next

    if not found:
        print("Not exist")

You can write this condition shorter:

if curr_node.data == data or curr_node.data > data:

Like this:

if curr_node.data >= data:


Prime Number Sieving Algorithm Last semester in my Masters Program I developed this code for sieving. My professor wants me to write a report that he might use locally within the school, but I feel like I've done nothing that new, despite coming up with my algorithm without doing any research other than the bare numbers.

Essentially it skips over a large percentage of numbers (I estimate about 66%). It skips all factors of 2 and 3. The first loop throws in primes and potential primes through a modified formula of 6k+-1. The second nested loop takes the outer prime numbers and finds all multiples.

The inner if statement skips numbers as well - if the number is a factor of 3, it skips ahead. For instance if B = 13, B+2 = 15%3 = 0, then B jumps straight to 17. I've found this method to be faster for some reason than accessing the array every b+=2; and switching the boolean. Unfortunately, I wish I could find a way to make it more efficient (I may be able to use an additional counter to just figure out when the number is going to fall on a five or three).

Psuedo-code as follows (of course this implementation is different, but follows similar principles):

Initialize list with initial values (2,3).
Use the formula 6k±1 where k = 1 and must increment by 1. Store values to list.
Compute step 2 up to a product n, where n > 7 if k = 1.
Starting with x=5, remove all products of x*y (to n), where y starts at x and
increments to the next number in the list as x remains the same.
Repeat step four by assigning x to the next number in the list - stop when x*y > N.
Final step: We must remove all factors of the square of every prime number (25, 49)

Is this any decent, or is my implementation really slow?

//-----------------------------------------------------------------
//
//  Class:   SixSieve.java
//
//  Author:  Alex Lieberman
//
//  Purpose: Finds all prime numbers up to a specified
//           maxNumber and marks them as true in the boolean array.
//
//-----------------------------------------------------------------

public class SixSieve

 {

  public static void main (String[] args)

  {

   int maxNumber = 1000;

   boolean[] Numbers = new boolean [maxNumber];

   Numbers[2] = true;
   Numbers[3] = true;

   int num = Numbers.length;

   //----------------------------
   //
   //  The following loop finds
   //  potential primes and primes.
   //
   //----------------------------

   for (int i = 5, j = 7; i < num; i+=6, j+=6)

    {

     Numbers[i] = true;

     if (j < num)

      Numbers[j] = true;

    }

   //----------------------------
   //
   //  The following loop eliminates
   //  non-primes (multiples of everything
   //  in the previous loop).
   //
   //----------------------------

   for (int a = 5; a*a <= num; a+=2)

    {

     if (Numbers[a])

      {

       for (int b = a; b*a <= num; )

        {

         Numbers[b*a] = false;

         if ((b+2)%3 == 0)

          b+=4;

         else

          b+=2;

        }

      }

    }

    System.out.println("Done");

   //----------------------------
   //
   //  The following loop prints
   //  all the primes found.
   //
   //----------------------------

    for (int i = 0; i < num; i++)

     {

      if (Numbers[i] == true)

       System.out.print(i + " ");

     }

  }

 }

I'd rename Numbers to isPrime
num is not needed. maxNumber serves the same purpose.
Termination condition in the elimination loops must be <, not <=. With an unfortunate value of maxNumber, the latter would make an illegal access.
An inner elimination loop can be shortened to

for (int b = a; b*a <= num; ) {
    isPrime[b*a] = false;
    b += 2;
    if (b%3 == 0)
        b+=2;
}

It is worth noticing that i+=6, j+=6 serves the same purpose as if (b%3 == 0) b+=2. Better be unified (it looks like an expensive modulo operator can be avoided). I'd seriously consider splitting each loop into two independent loops with increment 6.
Finally, the implementation is not slow (other than using %), but it is not any faster than a standard sieve.


Ruby function to join array with commas and a conjunction Should take an array such as ['dog', 'cat', 'bird', 'monkey']  and return 'dog, cat, bird and monkey'.

Looking for a more elegant solution.

def self.english_join(array)
  return nil if array.nil?
  return array[0] if array.length == 1
  return array[0..-2].join(', ') + " and " + array[-1] if array.length > 1
end

Rails (actually ActiveSupport, part of the Rails framework) offers a very nice Array#to_sentence method.

If you are using Rails or ActiveSupport, you can call

['dog', 'cat', 'bird', 'monkey'].to_sentence
# => "dog, cat, bird, and monkey"

The method is automatically customized according to I18n settings. For example, in Italy you should omit the last comma before the and.

['dog', 'cat', 'bird', 'monkey'].to_sentence
# => "dog, cat, bird e monkey"

If you want something without depending on ActiveSupport, you can start using the method source code.

This is just an example

class Array
  def to_sentence
    default_words_connector     = ", "
    default_two_words_connector = " and "
    default_last_word_connector = ", and "

    case length
      when 0
        ""
      when 1
        self[0].to_s.dup
      when 2
        "#{self[0]}#{options[:two_words_connector]}#{self[1]}"
      else
        "#{self[0...-1].join(options[:words_connector])}#{options[:last_word_connector]}#{self[-1]}"
    end
  end
end


Find ASCII codes and replace with characters I have a text file that uses the convention #\d+ to mean ASCII character number \d+. I want to replace any such representations with the actual character.  This is what I've come up with. It works but it feels kludgy. Suggestions to improve efficiency?

<?php

$line= 'stuff #65 more #66  and #67; other';
preg_match_all("/\#\d+/",$line,$p);//find any #numbers that should be translated to ascii
$r=array_map(
    function($x){ return chr(trim($x,'#')); },
    $p[0]); //assemble set of replacements

$line=str_replace($p[0],$r,$line); //replace
echo $line; //stuff A more B  and C; other
?>

Warning: The code below is intended to be compatible with the original post.
Its main benefit is that it retains all of the useful detail discovered in the pattern match (unmatched substrings, relative position of matches), eliminating the need to rescan the line. I'd expect it to be faster for that reason.

It retains the problem that chr is a blunt instrument that can effortlessly
consume many digits (not just one, two, or three), using "wrapping"
(a.k.a. modulo 256 math) to scale the resulting integer down to a legal byte value,
possibly not the one intended.

In other words, this code would fail the conversion of a string with ASCII codes
immediately followed by digits, e.g.
if "From A1 to z52" needs to be expressable as "From #651 to #12252".
So, a different approach may be needed.

One approach is for the conversion step to convert the digit string to integer digits one digit at a time,
doing the (digits*10 + digit) math and stopping (or actually backing up one digit)
when a value of 256 or greater is reached.

Another approach, probably faster, is for the conversion step to convert a substring (up to three digits) of
the digit string to an integer and if it is 256 or greater "back up one digit", dividing
the integer by 10.

In either case, any unused/backed-up digits would be appended to the result
of chr applied to the integer.

In either of these approaches, but especially the first,
some work might be saved by having the regex match at most
three digits instead of any number of digits -- leaving the rest as unmatched characters
at the head of the next "surrounding text" part.
That way, there would be no need to test for integer overflow from way too many digits.

A "higher-tech" approach could go further, enabling the current simple
$parts[$i] = chr($parts[$i]) conversion to be retained.
This would require beefing up the regex so that it matched, in order of preference,
a 3-digit number less than 256 OR a 2-digit number (that possibly had another
unmatched digit following it) OR a 1-digit number (that was not followed by another digit).

That seems like a lot of trouble, except maybe for a regex aficionado, so I'd probably opt for the middle approach, converting and testing a 3-digit substring.

<?php
    $line= 'stuff #65 more #66  and #67; other';
    // break out any #numbers that should be translated to ascii,
    // retaining the surrounding text and the numbers parts.
    // The '#' parts are kept outside the parens to be matched and discarded.
    $parts = preg_split("/\#(\d+)/",$line,NULL,PREG_SPLIT_DELIM_CAPTURE);
    $count = count($parts);
    // Even-indexed entries (possibly empty) contain unchanging surrounding text.

    // Odd-indexed entries are the numbers part of #numbers, so convert them.
    for ($i = 1; $i < $count; $i += 2) {
        $parts[$i] = chr($parts[$i]); // Problem here for input >= 256.
    }

    $line = implode($parts);
?>


Navigation bar React component, with a "back" and a "close" variant I am working on a component for a navigation bar. This navigation bar currently has two variations: a "back" version, and a "close" version.

I have come up with three different implementations but I am uncertain of which one is the best. Currently, I am leaning towards this implementation as it reads the best in my opinion.

const NAVIGATION_BAR_TYPE = {
  back: "back",
  close: "close",
}

const NavigationBar = ({ type = NAVIGATION_BAR_TYPE.back, text, linkTo }) => {
  return (
    <div className={`navigation-button ${type}`}>
      <Link to={linkTo}>{text}</Link>

  )
}

NavigationBar.Close = (props) => (
  <NavigationBar { ...props } type={NAVIGATION_BAR_TYPE.close} />
)

NavigationBar.Back = (props) => (
  <NavigationBar { ...props } type={NAVIGATION_BAR_TYPE.back} />
)

// Usage: <NavigationBar.Close linkTo="/" />

This is similar to the first one, but you need to import the object that holds the navigation bar types:

const NAVIGATION_BAR_TYPE = {
  back: "back",
  close: "close",
}

const NavigationBar = ({ type = NAVIGATION_BAR_TYPE.back, text, linkTo }) => {
  return (
    <div className={`navigation-button ${type}`}>
      <Link to={linkTo}>{text}</Link>

  )
}

// Usage: <NavigationBar type={NAVIGATION_BAR_TYPE.back} linkTo="/" />

Or this one, leveraging props:

const NavigationBar = ({ back, close, text, linkTo }) => {
  const navigationType = back ? 'back' : 'close'

  return (
    <div className={`navigation-button ${navigationType}`}>
      <Link to={linkTo}>{text}</Link>

  )
}

// Usage: <NavigationBar back linkTo="/" />

I don't like the third solution because if I need to add more variations, I feel like it clutters the props. And I don't like the second solution because then you are importing this enum-like object to define the type.

Without knowing the context of how this navigation bar is used in your app its hard to say which one would be more preferable. Does the navigation type dictate only the styling of the navigation bar or also some type of action too? Is the navigation type only tied to some sort of button within the navigation bar that displays differently and points to a different direction? Maybe doing something like this would work for that:

<NavigationBar>
    <NavigationBar.ActionButton type='back' />
</NavigationBar>

All the variants you provided seem like good solutions, I think you are in best judgement here to decide what fits your apps needs.


A simple function to rename columns in data frame to standardized names in R I wrote this simple function to rename columns in a data frame to standardized names in R. This function is useful when we have a very large data set with large number of columns especially for machine learning applications.

colRename<-function(x){
  for(i in 1:ncol(x)){
    colnames(x)[i] <- paste("column",i,sep="")
  }
  return(x)
}

An example

library(quantmod)

colRename<-function(x){
  for(i in 1:ncol(x)){
    colnames(x)[i] <- paste("column",i,sep="")
  }
  return(x)
}

aapl=getSymbols("AAPL",from="2015-01-01",auto.assign=F)
head(colRename(aapl))

Result

           column1 column2 column3 column4  column5  column6
2015-01-02  111.39  111.44  107.35  109.33 53204600 105.6986
2015-01-05  108.29  108.65  105.41  106.25 64285500 102.7209
2015-01-06  106.54  107.43  104.63  106.26 65797100 102.7306
2015-01-07  107.20  108.20  106.70  107.75 40105900 104.1711
2015-01-08  109.23  112.15  108.70  111.89 59364500 108.1736
2015-01-09  112.67  113.25  110.21  112.01 53699500 108.2896

Most R functions are vectorized, you don't need the for loop:

colRename <- function(x) {
   setNames(x, paste0("column", seq_len(ncol(x))))
}


Testing my comprehension of loops I want to make sure I'm understanding loops. Below are the two files I used to test what I've learned. Is there anything that I'm not using that would help simplify my code?

//Program.cs
using System;

namespace _99_Bottles
{
    class Program
    {
        static void Main(string[] args)
        {
            Bar b = new Bar();
            Console.WriteLine(b.SingASong(SingingMethod.DoWhile));

            Console.WriteLine(b.SingASong(SingingMethod.ForLoop));

            Console.WriteLine(b.SingASong(SingingMethod.WhileLoop));

            Console.WriteLine(b.SingASong(SingingMethod.ForEach));
        }
    }
}

//Bar.cs
using System.Text;

namespace _99_Bottles
{
    public enum SingingMethod
    {
        ForLoop,
        WhileLoop,
        DoWhile,
        ForEach
    }
    class Bar
    {
        private StringBuilder _chorus = new StringBuilder();
        private int _bottleCount;

        public string SingASong(SingingMethod singingMethod, int numberOfBottles = 99)
        {
            _bottleCount = numberOfBottles;
            switch (singingMethod)
            {
                case SingingMethod.DoWhile:
                    return DoWhileLoop();
                case SingingMethod.ForLoop:
                    return ForLoop();
                case SingingMethod.WhileLoop:
                    return WhileLoop();
                case SingingMethod.ForEach:
                    return ForEach();
                default:
                    return "Nothing valid";
            }
        }

        private string ForLoop()
        {
            _chorus.AppendLine("ForLoop");
            for (int i = _bottleCount; i >= 1; i--)
            {
                _chorus.AppendLine(SingTheLine(i));
            }
            return _chorus.ToString();
        }

        private string WhileLoop()
        {
            _chorus.AppendLine("WhileLoop");
            int i = _bottleCount;
            while (i > 0)
            {
                _chorus.AppendLine(SingTheLine(i));
                i--;
            }
            return _chorus.ToString();
        }

        private string DoWhileLoop()
        {
            int i = _bottleCount;
            do
            {
                _chorus.AppendLine(SingTheLine(i));
                i--;
            } while (i > 0) ;

            return _chorus.ToString();
        }

        private string SingTheLine(int drinksLeft)
        {
            StringBuilder sb = new StringBuilder();
            string plurality = ((drinksLeft != 1) ? "s" : string.Empty);
            string pluralityMinusOne = ((drinksLeft + 1 != 1) ? "s" : string.Empty);
            sb.AppendLine(string.Format(string.Format("{0} bottle{1} of beer on the wall, {0} bottle{1} of beer.", drinksLeft, plurality)));
            sb.AppendLine(string.Format("Take one down and pass it around, {0} bottle{1} of beer on the wall.",((drinksLeft-1 > 1) ? (drinksLeft-1).ToString() : "no"), pluralityMinusOne));

            return sb.ToString();
        }

        private string ForEach()
        {
            const string ymca = "YMCA";

            StringBuilder sb = new StringBuilder();
            sb.AppendLine("It's fun to stay at the:");
            foreach (char letter in ymca)
            {
                sb.AppendLine(letter.ToString());
            }

            return sb.ToString();
        }
    }
}

Aside from the inconsistency in the for loop.  It would make more sense to have the limit as

i > 0

Your implementations seems sound to me.

However the SingTheLine method could use some work.

First off this:

string pluralityMinusOne = ((drinksLeft + 1 != 1) ? "s" : string.Empty);

doesn't look right.  Shouldn't it be (drinksLeft - 1 != 1)?

Also the new string concatenation introduced in c#6, I believe, is much easier to understand than using string.Format.

sb.AppendLine($"{drinksLeft} bottle{plurality} of beer on the wall, {drinksLeft} bottle{plurality} of beer.");
sb.AppendLine($"Take one down and pass it around, {((drinksLeft - 1 > 1) ? (drinksLeft - 1).ToString() : "no")} bottle{pluralityMinusOne} of beer on the wall.");


Forcing type-safe IDs for use with Collections and Maps Introduction

I have a hierarchically nested tree class (like SampleFolder, s.b.) that I want to serialize.

The problem arises with the collection of sub items like this one:

List<SampleFolder> subFolders;

I do not want to load the entire tree but only one folder at a time, when I deserialize!
So instead of creating a list of the actual objects I need to use IDs. Naturally I am using a primitive based data type for reduced overhead:

List<Double> subFolders;

All good with serialization here. But in a more complex project handling all those Double-IDs, esp. within maps starts to become very confusing during development. As the project increases in complexity it happens that I would hand the wrong ID to a collection.

Map<Double, List<Double>> myAssociations; // what? which id goes where?

So to battle this issue of lost-type safety I created an interface Item and inner class Id (s.b.). This is the resulting usage (more in-depth example see SampleFolder, below):

Map<Id<Car>, List<Id<Wheel>>> myAssociations; // a lot more readable, isn't it?

Question

I couldn't find a best-practice for this issue, so if there is a better way to solve this "type-safety" issue please let me know.
Even though this seems to work, I am wondering if this is good code in respect to the use of generics and if the implementation of hashCode() and equals() is correct.
I am open for optimization suggestions and other critics.

code to review: Item with Id

interface Item {

    double getId();

    class Id<T extends Item> implements Serializable {

        private double id;

        public Id(T item) {
            this.id = item.getId();
        }

        public Id(double id) {
            this.id = id;
        }

        public double asDouble() {
            return id;
        }

        @Override
        public boolean equals(Object obj) {
            if (this == obj) {
                return true;
            }

            if (!(obj instanceof Id)) {
                return false;
            }

            Id<?> cast = (Id<?>) obj;

            return id == cast.id;
        }

        @Override
        public int hashCode() {
            return Double.valueOf(id).hashCode();
        }

        public static <E extends Item> Id<E> from(double id) {
            return new Id<E>(id);
        }

        public static <E extends Item> Id<E> from(E enumItem) {
            return new Id<E>(enumItem);
        }

        @Override
        public String toString() {
            return String.valueOf(id);
        }
    }
}

Sample usage (class not part of the actual project; for illustration purposes only):

public class SampleFolder implements Item, Serializable {

    private final double id;
    private List<Id<SampleFolder>> subFolders;
    private List<Id<MoreItem>> moreItems;
    private List<Id<EvenMoreItem>> evenMoreItems;

    public SampleFolder() {
        id = new Date().getTime();
        subFolders = new ArrayList<>();
    }

    public SampleFolder(double id, List<Id<SampleFolder>> subFolders) {
        this.id = id;
        this.subFolders = subFolders;
    }

    @Override
    public double getId() {
        return id;
    }

    public List<Id<SampleFolder>> getSubFolders() {
        return subFolders;
    }

    public void setSubFolders(List<Id<SampleFolder>> subFolders) {
        this.subFolders = subFolders;
    }
}

Do not use floating point numbers as IDs! Floating point is a horrible minefield of gotchas out there to sabotage your program in subtle ways: Losing precision, imprecise equality, NaNs, …. Using a double only defers, but not solves these issues.

For IDs, always use an integral type like long.

Now you've found a sufficiently elegant solution to represent typesafe IDs by using generics. There are a few other possibilities you might want to consider:

Lazy self-loading classes. Each instance has a boolean field initialized which defaults to false. In each public method you put in a if (!initialized) this.initialize() test to load the other fields – but only when they are needed.

Usage example:

class LazyThing {
    private boolean initialized = false;
    ... // state that is loaded lazily

    private void initialize() {
        ... // initialization code
        initialized = true;
    }

    public void frobnicate() {
        if (! initialized) initialize();
        ...
    }
}

The advantage here is that this pattern is completely invisible to the user.
A Deferred<T> wrapper which is basically a “promise” or “future” (or any other single-element monad-like container of your choice). A deferral wrapper represents an object that may or may not be fully loaded. You can get() the element to force loading. This is a bit more awkward to use but properly separates the lazy loading from the wrapped type. The full interface might look like:

abstract class Deferred<T> {
  boolean initialized = false;
  T instance;

  abstract T initialize();  // sadly, Java does not have traits

  public T get() {
    if (! initialized) {
      instance = initialize();
      initialized = true;
    }
    return instance;
  }
}

Usage example:

Deferred<Thing> foo = new Deferred<Thing>() {
  private long id;

  // anon classes can't have constructors :(
  Deferred<Thing> init(long id) {
    this.id = id;
    return this;
  }

  @Override
  Thing initialize() {
    ... // code to load the instance
  }
}.init(1234);

foo.get().frobnicate();

This scheme is extremely flexible because it does not assume the existence or type of an ID.


Programs for different language learning - Perl I've created a program for my own purposes in three different languages, Perl, Python, and Ruby. Each program will be posted in with their own tag but I will add links to them through on each post. The program itself is very simple, all it does is ask the user about three different languages which are saved into an array, it then prompts the user to choose a language that runs through an if statement, and if the user chooses the language they're writing in it then takes a random language from the array and outputs it.

See also Python

See also Ruby

use strict;
use warnings;

sub welcome {
    my @choices = qw( Perl Python Ruby );
    my $lang = 3;
    print("Welcome, to the test script, this will test what language you would like to learn.. In order to find out these choices, write this same definition in all three different languages\n");
    print("There are  $lang, languages to chose from please pick one:\n");
    print "@choices\n";
    my $choice = <STDIN>;
    chomp $choice;
    if ($choice eq "Ruby") {
        print("You have chosen Ruby!\n");
    }
    elsif ($choice eq "Python") {
        print("You have chosen Python!\n");
    }
    else {
        print("You're already writing in Perl!! Let me choose for you:\n");
        my $rand_elm = @choices[rand @choices];
        print "$rand_elm\n"
    }
}
welcome();

Perl took me the longest out of all the programs I wrote, this was my first ever Perl script. With all the ; and the random symbols I had to use to make variables and arrays, it was defiantly hard. However I really enjoyed writing this in Perl, it was something different. Perl maxed out as the longest program at a total of 30 lines. At one point I kept getting an NameError that was due to me missing a ; at the chomp function. All in all I think Perl is definitely a great language to learn.

I also have some questions about Perl:

Is there any better syntax that I should be using here in order to shorten the program?
Does Perl have an equivalent to Ruby's puts?
Why is it easier to use a sub instead of a function?

example:

 welcome();
 # instead of
 sub welcome();

What you did well

use strict;, use warnings;, and my scoping
qw(list of words) instead of ('list', 'of', 'words')
Generally straightforward, readable Perl.

Suggestions

There is a hard-coded correspondence between my $lang = 3 and the fact that there are three choices.  My suggestion:

my $choices = @choices;

In Perl, $choices and @choices are different variables: the former is a scalar, and the latter is a list.  The assignment above coerces the list into a scalar, and it does so by taking the number of elements in the list.  (You used this technique in @choices[rand @choices].
Parentheses are optional for subroutine calls, and they are typically omitted for well-known functions such as print.
For maintainability, terminate the last print with a semicolon, even if you aren't required to.
As @glennjackman already told you, say would be better than print, as it also outputs a trailing newline.
Avoid super-long string literals by using a here document.

    print <<"INTRO";
Welcome to the test script…
There are $choices languages to choose from.
@choices
INTRO

Your logic is rather trusting: any input other than "Ruby" or "Python" is treated as if it were "Perl".

A better way:

use feature qw(say);
…

chomp(my $choice = <STDIN>);
if ($choice eq 'Perl') {
    say "You're already writing in Perl!! Let me choose for you:";
    say @choices[rand @choices];
} elsif (grep { $choice eq $_ } @choices) {
    say "You have chosen $choice!";
}

A possible outcome is "You're already writing in Perl!! Let me choose… Perl".  I consider that to be a logic bug.


HTTP scraper efficiency with multiprocessing I built this scraper for work that will take a csv list of firewalls from our network management system and scan a given list of HTTPS ports to see if the firewalls are accepting web requests on the management ports. I originally built this in powershell, but decided to rebuild it in python for the learning experience.

I was able to cut down the scan time substantially using multiprocessing, but I'm wondering if I can further optimize my code to get it faster.

Also, I'm very new to python. So if you have any input on better more efficient ways that I could have used to accomplish these steps would be much appreciated.

import urllib.request
import re
import os
import ssl
import multiprocessing

#imports a csv list of firewalls with both private and public IP addresses
f = open(r'\h.csv',"r")
if f.mode =="r":
    cont = f.read()

#regex to remove private ip addresses and then put the remaining public ip addresses in a list
c = re.sub(r"(172)\.(1[6-9]|2[0-9]|3[0-1])(\.(2[0-4][0-9]|25[0-5]|[1][0-9][0-9]|[1-9][0-9]|[0-9])){2}|(192)\.(168)(\.(2[0-4][0-9]|25[0-5]|[1][0-9][0-9]|[1-9][0-9]|[0-9])){2}|(10)(\.(2[0-4][0-4]|25[0-5]|[1][0-9][0-9]|[1-9][0-9]|[0-9])){3}","",cont)
d = re.findall(r"[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}",c)

#uses HTTP requests to check if any of the 8 management ports on the addresses in the list are accepting web requests
def httpScan(list):
    iplen = len(list)
    ports = [443, 4433, 444, 433, 4343, 4444, 4443, 4434]
    portlen = len(ports)
    for k in range(iplen):
        for i in range(portlen):
            context = ssl._create_unverified_context()
            try:
                fp = urllib.request.urlopen("https://" + str(list[k]) + ":" + str(ports[i]), context=context)
                mybytes = fp.read()
                mystr = mybytes.decode("utf8")
                fp.close()
            except:
                continue
            if "SSLVPN" in mystr:
                print(list[k] + " SSLVPN" + ": " + str(ports[i]) + "  " + str(k) + "  " + str(os.getpid()))
            elif "auth1.html" in mystr:
                print(list[k] + " MGMT" + ": " + str(ports[i]) + "  " + str(k))

#splits the list of IP addresses up based on how many CPU there are and adds each segment to a dictionary
cpu = int(multiprocessing.cpu_count())
sliced = int(len(d)/cpu)
mod = int(len(d))%int(cpu)
num = 1
lists = dict()
for i in range(cpu):
    if i != (cpu - 1):
        lists[i] = d[(num*sliced) - sliced:num*sliced]
        num += 1
    else:
        lists[i] = d[(num*sliced) - sliced:(num*sliced) + mod]

#starts a process for each unique segment created
t = dict()
if __name__ == "__main__":
    for i in range(cpu):
        t[i] = multiprocessing.Process(target=httpScan, args=(lists[i],))
        t[i].start()

```

Reading File

Here is a tip, while reading, the r is optional

f = open(r'\h.csv',"r")

can be written as

f = open(r'\h.csv')

Your whole reading block can use context managers (blocks using the with keyword).

with open(r'\h.csv', encoding='utf8') as f:
    cont = f.read()

If you are dealing with a huge text file, you might do:

with open(r'\h.csv', encoding='utf8') as f:
    for ip in f:
        ip = ip.rstrip('\n')
        .. verify

String

Using string formatting i.e. .format() can give a better idea of what's going on. It also eliminates the use of str() each time.
We can change this

print(list[k] + " MGMT" + ": " + str(ports[i]) + "  " + str(k))

to that

print("{} MGMT: {}  {}".format(list[k], ports[i], k))

and as from 3.6+, adding an f

print(f"{list[k]} MGMT: {ports[i]}  {k}")

Loop Iteration

In many other languages, you need the index while looping to have the element at this index. Python provides a nice and intuitive way to loop over elements

The current implementation:

ports = [443, 4433, 444, 433, 4343, 4444, 4443, 4434]
portlen = len(ports)
for i in range(portlen):
    print(ports[i])

But the pythonic way is:

ports = [443, 4433, 444, 433, 4343, 4444, 4443, 4434]
for port in ports:
    print(port)

port here gives you the element directly.
If ever you still want the index, you do:

for i, port in enumerate(ports):

where i is the index.

Miscellaneous

Here:

cpu = int(multiprocessing.cpu_count())

No need to cast to int as multiprocessing.cpu_count() already returns an integer. You can verify for int by type(multiprocessing.cpu_count())

Normally with .start(), you must include a .join(), as this allows all child processes to terminate before exiting.

for ...:
   ... .start()

for ...:
   ... .join()


Getting data from different services I have a piece of code that tries to get some data from a different services, falling back to the next one if previous fails.

The code look a little bit ugly, though. Any suggestions on how to rewrite it in a more concise way?

(defn get-data [query]
  (let [data-cached (get-data-from-cache query)
        data-service-1 (if (nil? data-cached)
                         (get-data-service-1 query)
                         data-cached)
        data-service-2 (if (nil? data-service-1)
                         (get-data-service-2 query)
                         data-service-1)
        data (if (nil? data-service-2)
                   (get-data-service-3 query)
                   data-service-2)]
    data))

Perhaps or macro could be used here:

(defn get-data [query]
   (or (get-data-from-cache query)
       (get-data-service-1 query)
       (get-data-service-2 query)
       (get-data-service-3 query)))

or short-circuits, so the first match will return and the rest will not be evaluated


Filtering user input to not contain a specific character I need to filter an input from a user to never have the have the Pilcrow Sign (¶) character. The following methods filters the input from the user

public String filterInput(String userInput) {
    StringBuilder sBuilder = new StringBuilder();

    for (char character : userInput.toCharArray()) {
        if (checkCharacter(character)) {
            sBuilder.append(character);
        }
    }
    return sBuilder.toString();
}

public boolean checkCharacter(char charToCheck) {
    return charToCheck != 182;
}

filterInput gets passed a String containing the user input which is taken from Scanner(System.in) No previous filtering has been done on the user input.

My concern is, is the code well written or is there still a way for the user to write the Pilcrow sign in such a way that when the String is broken into characters, it could bypass the checkCharacter method and return true, effectively managing to inject the forbidden character in my app?

Your code looks correct and safe to me.  However, I would simplify it as:

public static String filterInput(String userInput) {
    return userInput.replace("\u00b6" /* Pilcrow sign */, "");
}

Two public functions seems like overkill, when all you need is a static function.  (If you intend for the filter to be configurable by overriding checkCharacter() in a subclass, then I would suggest changing to a more flexible design where the naughty characters are specified as a parameter.)


Singly Linked List Implementation in ES6 This is my Singly Linked List implementation in ES6.

class LinkedList{

    constructor(){
        this.head = null;
        this.size = 0;
    }

    insert(item){
        this.size ++;
        let p = new Node(item);
        if(this.head === null){
            this.head = p;
        }
        else{
           p.next = this.head;
           this.head = p;
        }
    }
    insertAt(item, index){
        if(index > this.size){
            throw "Wrong index";
        }
        let p = new Node(item);
        if(index === 0){
            p.next = this.head;
            this.head = p;
            return;
        }
        let curr = this.head;
        while(curr !== null){
            if(index === 0){
                if(curr.next === null){
                    curr.next = p;
                    return;
                }
                else if(curr.next.next === null){
                    curr.next = p;
                }else{
                    curr.next.next = p.next;
                    curr.next = p;

                }
                return;
            }
            curr = curr.next;
            index --;
        }

    }
    remove(item){
        let curr, prev;
        prev = curr = this.head;
        if(item === curr.val){
            this.head = this.head.next;
            this.size --;
            return;
        }
        while(curr !== null){
            if (curr.val === item){
                prev.next = curr.next;
                this.size --;
                return;
            }
            prev = curr;
            curr = curr.next;

        }
        throw "Item doesn't exist in list."
    }
    length(){
        return this.size;
    }

    is_empty(){
        return this.size === 0;
    }
    printList(){
        let curr = this.head;
        let out = [];
        while(curr !== null){
            if(curr.next === null){
                out.push(curr.val);
            }else{
                out.push(curr.val);
                out.push("->")
            }
            curr = curr.next;
        }
        return out.join("");
    }
}
class Node{
    constructor(val){
        this.val = val;
        this.next = null;
    }
}

const list = new LinkedList();
list.insert(12);
list.insert(9);
list.insert(13);
list.insert(17);
console.log(list.printList());
list.remove(12);
console.log(list.printList());
console.log(list.length());
console.log(list.is_empty());
list.insertAt(21, 2);
console.log(list.printList());

Invite reviews please on all fronts, improvements, modern usage of JS.
Thanks.

TL;DR

Style could better adhere to established standards.
insertAt has bugs to resolve.
This class is missing basic add/remove functions which prevent it from being useful.
Writing a linked list class in JS only makes sense from an educational standpoint; primitive arrays are efficient, universal and offer more functionality with less code.

Style

Spacing

Use a space between )s, {s and keywords, e.g.

if(this.head === null){

is cleaner as

if (this.head === null) {

Add vertical spacing between lines to group logical code blocks together and separate loop, conditional and function logic from declarations. For example,

let curr = this.head;
while(curr !== null){

is easier to read as

let curr = this.head;

while (curr !== null) {

this.size ++ is clearer as this.size++.
Be consistent--sometimes you use }else{, and sometimes you use

}
else{

Your whitespace fixes can be done by putting the code into SE's Code Snippet and pressing "Tidy", then adding blank lines around blocks by hand. Generally, I don't see reason to deviate from this prescribed style.

Variable and function naming

Function names switch between snake_case and camelCase. All JS naming should be camelCase, except class names, which are UpperCamelCase (as you use).
Avoid single-letter variable names like p unless the meaning is obvious. In this case, something like newNode might be clearer.

Functionality

Errors

throw "Wrong index"; is an unclear error message. What exactly is wrong with the index? Consider throw "Index out of list bounds"; which more accurately describes the problem.
You may wish to reconsider using errors at all. I find throwing errors in JS generally less appropriate than return values because the calling code can stick to normal conditionals to handle control flow.

Also, not throwing errors is in keeping with JS's builtin library functions, which generally don't complain about invalid or missing input. For insertAt, for example, you could return true if the insertion was successful and false otherwise. If the user provides something silly as an index that causes a crash, they'll get an appropriate stack trace that likely explains the problem better than a hand-written error string.

If you do wish to stick with error throwing, ensure it is comprehensive. For example, if (index > this.size) doesn't handle the case when index < 0 which could result in difficult-to-track-down bugs for the client who has to make design assumptions based on your throw.

Then, once you've covered that scenario, it begs the question whether you should now validate that the provided input is an integer number and throw an error message for that as well.

The point is, adding errors gives the client the illusion of a comprehensive and robust set of argument and function state restrictions, which is problematic if they aren't actually robust. Throwing no errors, assuming the client is behaving, and reporting true/false as to whether some function failed or not seems preferential.

Variable keywords

Use const when appropriate in place of let. This should apply to almost everything except for loop counters, accumulator variables and runner nodes. For example, let out = []; should be const out = [];. Even if the risk of bugs caused by accidental reassignment is low, this has semantic benefits.

Misleading function names

printList() is a misleading name; it stringifies the list rather than printing. I recommend overriding toString() here.
insert() usually refers to insertion at some specific element, which is the behavior your insertAt() function offers. With insert(), It's not obvious where the insertion is happening; one of addFront, unshift or push seem clearer, depending on which end of your list you decide the front to be.

Internal logic

insertAt is not working correctly, placing items incorrectly, not at all, and neglecting to increment the size.
Consider adjusting your insertAt code to avoid using returns and curr.next.next, which is difficult to reason about and causing bugs.
In printList, pains are taken to conditionally add the -> arrow only for non-last elements when you can simply walk the list and use out.join("->").
Since your internal code relies only on Node objects, you can make your code cleaner by testing while (curr) instead of while (curr === null). This is debatable, because it restricts your internal logic from distinguishing between null and undefined or other falsey values, but if you trust yourself to be consistent about it, I prefer the cleaner look.

Interface

As written, I find your provided function interface insufficient for typical linked list needs. It's not obvious what functionality this class offers over, say, a primitive array.

Consider remove(item). This sort of function that takes an element and searches the structure for it is best used for hashes with random access. The whole point of linked lists is to offer fast insertion and removal of front and back elements, regardless of what those elements might be. Anything in the middle is of no concern, and libraries that offer access to these elements, such as Java's LinkedList collection are generally considered to be flawed because clients may make false assumptions about the time complexity of provided operations.

Without constant time front and back adds, removals and peeks, I can't foresee a reason to use this class instead of a primitive array. insert is the only useful linked list function your interface offers (disregarding isEmpty, printList, etc as useful but trivial).

Rewrites

Revision #1 (same functionality)

class LinkedList {
  constructor() {
    this.head;
    this.size = 0;
  }

  addFront(value) {
    const newNode = new Node(value);

    if (this.head) {
      newNode.next = this.head;
    }

    this.head = newNode;
    this.size++;
  }

  insertAt(value, idx) {
    let curr = this.head;
    let prev;

    while (curr && idx > 0) {
      prev = curr;
      curr = curr.next;
      idx--;
    }

    if (prev) {
      prev.next = new Node(value);
      prev.next.next = curr;
      this.size++;
    }
    else {
      this.addFront(value);
    }
  }

  remove(value) {
    let curr = this.head;
    let prev;

    while (curr) {
      if (curr.val === value) {
        if (prev) {
          prev.next = curr.next;
        }
        else {
          this.head = undefined;
        }

        this.size--;
        return true;
      }

      prev = curr;
      curr = curr.next;
    }

    return false;
  }

  length() {
    return this.size;
  }

  empty() {
    return !this.size;
  }

  toString() {
    const result = [];
    let curr = this.head;

    while (curr) {
      result.push(curr.val);
      curr = curr.next;
    }

    return result.join("->");
  }
}

class Node {
  constructor(val, nextNode=null) {
    this.val = val;
    this.next = nextNode;
  }
}

const list = new LinkedList();
list.remove(1123);
list.insertAt(21, 33);
list.remove(21);
list.addFront(12);
list.addFront(9);
list.addFront(13);
list.addFront(17);
list.remove(1123);
console.log(list.toString());
list.remove(12);
console.log(list.toString());
console.log(list.length(), list.empty());
list.insertAt(21, 2);
console.log(list.toString(), list.length());
list.insertAt(11, 0);
console.log(list.toString(), list.length());

Revision #2 (added front/back operations)

class LinkedList {
  constructor() {
    this.head;
    this.tail;
  }

  addFront(value) {
    const newHead = new Node(value);

    if (this.head) {
      newHead.next = this.head;
    }
    else {
      this.tail = newHead;
    }

    this.head = newHead;
  }

  addBack(value) {
    if (this.tail) {
      this.tail.next = new Node(value);
      this.tail = this.tail.next;
    }
    else {
      this.head = this.tail = new Node(value);
    }
  }

  peekFront() {
    return this.head ? this.head.val : null;
  }

  peekBack() {
    return this.tail ? this.tail.val : null;
  }

  popFront() {
    if (this.head) {
      const value = this.head.val;
      this.head = this.head.next;

      if (!this.head) {
        this.tail = null;
      }

      return value;
    }
  }

  empty() {
    return !this.head;
  }

  toString() {
    const result = [];
    let curr = this.head;

    while (curr) {
      result.push(curr.val);
      curr = curr.next;
    }

    return result.join("->");
  }
}

class Node {
  constructor(val, nextNode=null) {
    this.val = val;
    this.next = nextNode;
  }
}

const list = new LinkedList();
list.popFront();
console.log(list.toString(), list.empty());
list.addBack(1);
console.log(list.toString(), list.empty());
list.popFront();
console.log(list.toString(), list.empty());
list.addFront(2);
console.log(list.toString());
list.addBack(3);
console.log(list.toString());
list.popFront();
console.log(list.toString());
list.addFront(4);
list.addBack(5);
console.log(list.toString());

Note that there is no popBack() because this operation would be linear without doubly linked nodes. However, the class is sufficient to support both stacks and queues with all operations in constant time. Without the tail pointer, only a stack could be supported. Adding popBack() and double links would give you a deque class.

Revision #3 (supported a queue)

class Queue {
  constructor() {
    this.list = new LinkedList();
  }

  offer(value) {
    this.list.addBack(value);
  }

  poll() {
    return this.list.popFront();
  }

  peek() {
    return this.list.peekFront();
  }

  empty() {
    return this.list.empty();
  }

  toString() {
    return this.list.toString();
  }
}

class LinkedList {
  constructor() {
    this.head;
    this.tail;
  }

  addFront(value) {
    const newHead = new Node(value);

    if (this.head) {
      newHead.next = this.head;
    }
    else {
      this.tail = newHead;
    }

    this.head = newHead;
  }

  addBack(value) {
    if (this.tail) {
      this.tail.next = new Node(value);
      this.tail = this.tail.next;
    }
    else {
      this.head = this.tail = new Node(value);
    }
  }

  peekFront() {
    return this.head ? this.head.val : null;
  }

  peekBack() {
    return this.tail ? this.tail.val : null;
  }

  popFront() {
    if (this.head) {
      const value = this.head.val;
      this.head = this.head.next;

      if (!this.head) {
        this.tail = null;
      }

      return value;
    }
  }

  empty() {
    return !this.head;
  }

  toString() {
    const result = [];
    let curr = this.head;

    while (curr) {
      result.push(curr.val);
      curr = curr.next;
    }

    return result.join("->");
  }
}

class Node {
  constructor(val, nextNode=null) {
    this.val = val;
    this.next = nextNode;
  }
}

const q = new Queue();
q.offer(1);
console.log(q.poll());
console.log(q.poll());

for (let i = 0; i < 5; i++) {
  q.offer(i);
}

while (!q.empty()) {
  console.log(q.poll());
}

After all that work, a benchmark shows that it's no faster than a primitive array. This is likely due to overhead from object creation, function calls and garbage collection, which counteracts the shifting necessary on the primitive array.


Adding a link on page with current URL (including hash) in a querystring I'm trying to add a link to a page. The link should contain the current URL in a querystring (to be used as a ReturnUrl) and it can contain a hash.

Middle-clicking, right-clicking and ctrl+clicking should be supported. For example, middle-clicking will open the link in a new tab with (or without) the querystring.

Is this completely off-track, or how can I improve it?

<a href="@Url.Action(MVC.Links.ThereAndBackAgain())?ReturnUrl=" onclick="this.href += encodeURIComponent(location.href)">

I don't think this can possibly be improved.
The location hash may be changed by the client after the page is loaded,
by clicking on local anchors.
This is not visible by the asp.net,
and fully visible by the client-side javascript,
in the value of location.href.
And since you require the complete URL to be included as part of the query string,
it must be encoded with encodeURIComponent,
which you correctly did.


Serialization/Unserialization of a list of strings - C++ Are there any issues with below implementation?
Input: from STDIN, a list of strings
Output: serialize to a file called "out.txt" and then unserialize into a list of strings, and output to STDOUT

Goal is not here too much about code organization, but I would like to know if this code can fail for certain types of inputs

#include <iostream>
#include <fstream>
#include <sstream>
#include <vector>
#include <cstdlib>

using namespace std;

int main()
{
    std::vector<std::string> input;

    std::string tmp;
    while (std::cin >> tmp)
    {
        input.push_back(tmp);
    }

    std::ofstream out("out.txt", std::ios::out);
    for (std::vector<std::string>::const_iterator iter = input.begin();
         iter != input.end();
         ++iter)
    {
        out << (*iter).size() << "|" << *iter;
    }
    out.close();

    std::vector<std::string> output;
    std::ifstream in("out.txt", std::ios::in);
    if (in.is_open())
    {
        std::string tmp;
        if (!in.eof())
        {
            getline(in, tmp);
            size_t pos;
            int length = 0;
            std::string str;

            while (!tmp.empty())
            {
                pos = tmp.find('|');
                if (pos != std::string::npos)
                {
                    length = atoi(tmp.substr(0, pos).c_str());
                    str = tmp.substr(pos+1, length);
                    tmp = tmp.substr(pos+str.size() + 1);

                    output.push_back(str);
                }
            }
        }

        in.close();
    }

    for (std::vector<std::string>::const_iterator iter = output.begin();
         iter != output.end();
         ++iter)
    {
        std::cout << *iter << "\n";
    }

    return 0;
}

Looks like it will work fine.
You preserve the string integrity in the file by maintaining its length as a separate entity that you can check independently. So it should work fine.

Not sure why you copy it to intermediate arrays.
Why not copy directly from input to file then from file to output?

You implement most of the other loops so precisely. But the main loop to read from the file is a bit a bit bulky and messey. With an extra class you can make it look just like the others.

std::vector<std::string> output;
std::ifstream in("out.txt", std::ios::in);
MyStringReader    reader;

while(in >> reader)
{
    output.push_back(reader);
}
in.close();

So now you just need to define the MyClassReader

struct MyStringReader
{
    std::string   data;
    operator std::string const&() {return data;}  // This is used in the line
};                                                // output.push_back(reader) and converts
                                                  // the reader object into a string before
                                                  // it is pushed

std::istream& operator>>(std::istream& stream, MyStringReader& value)
{
    size_t length;
    char   sep    = 'X';

    // Use the stream operators to get the size (and separator)
    // Much nicer than using tha atoi() function.
    //
    if ((stream >> length >> sep) && (sep == '|'))
    {
        // Resize the string to the correct size and put the word into it.
        value.data.resize(length);
        stream.read(&value.data[0], length);
    }
    return stream;
}


Transforming a dict of lists into a list of dicts I would like to transform a dict of one format into a dict of another format.

The raw_input dict will have the shape:

'key': ['list of strings']

I would like to reformat it into a list of dicts with the following shape:

[
    {'key': 'string'},
    {'key': 'string'},
    ...etc for each item in the list of strings
]

My implementation is functional but naive, using a doubly nested for loop:

raw_input = {
  'error': ['string 1', 'string2'],
  'error2': ['string 3']
}

def return_dict_as_list(raw_input):
    mylist = []
    for foo in raw_input:
        for bar in raw_input[foo]:
            mylist.append( { 'mykey': foo, 'myvalue': bar } )
    return mylist

The output is as expected but I can't help but think there's a better way.

I did try a couple of list comprehensions, but the output was not what was desired:

for example this:

elist = [[{'field': item, 'message': msg} for msg in raw[item]] for item in raw]

returns nested lists within a list, which I could unpack but doesn't seem very zen.

here's an online repl with the code:
https://repl.it/repls/UnpleasantBiodegradableSystems

You can at least turn the inner for loop into a list/generator comprehension and use list.extend ("Flat is better than nested"):

def return_dict_as_list(raw_input):
    mylist = []
    for key, values in raw_input.items():
        my_list.extend({'mykey': key, 'myvalue': value} for value in values)
    return mylist

But you can even turn it into one list comprehension with two for loops in it (and no nested lists):

def return_dict_as_list(raw_input):
    return [{'mykey': key, 'myvalue': value}
            for key in raw_input
            for value in raw_input[key]]

This will be a bit faster, too, since list comprehensions loop at C speed.

Another approach is to make it a generator:

def to_list_generator(d):
    for key, values in d.items():
        for value in values:
            yield {'mykey': key, 'myvalue': value}

This makes it a lot easier to see what is happening (depending on your taste). It also means that you don't need to hold the new list in memory but can process one item at a time in some other function.


Vector with move constructor and move assignment operator Please verify whether the code for a move constructor and a move assignment operator is correct.

#include <iostream>
using namespace  std;

class MyVector {
    size_t m_Size;
    int* m_pInt;
public:`enter code here`
    ~MyVector()
    {
        delete[] m_pInt;
    }

    MyVector():m_Size(0), m_pInt(nullptr)
    {   }

    MyVector(int x)
    {
        m_Size = x;
        m_pInt = new int[m_Size];
        for (int i = 0; i < m_Size; ++i)
        {
            m_pInt[i] = i;
        }
    }

    void print() const
    {
        for (int i = 0; i < m_Size; ++i)
        {
            cout << m_pInt[i] << endl;
        }

    }

    MyVector(const MyVector& rhs)
    {
        m_Size = rhs.m_Size;
        m_pInt = new int[m_Size];
        for (int i = 0; i < m_Size; ++i)
        {
            m_pInt[i] = rhs.m_pInt[i];
        }
    }

    MyVector(MyVector&& rhs):m_Size(move(rhs.m_Size)), m_pInt(move(rhs.m_pInt))
    {
        rhs.m_Size = 0;
        rhs.m_pInt = nullptr;
    }

    MyVector& operator=(const MyVector& rhs)
    {
        if (this != &rhs)
        {
            if (m_pInt != nullptr)
            {
                delete m_pInt;
            }
            m_Size = rhs.m_Size;
            m_pInt = new int[m_Size];
            for (int i = 0; i < m_Size; ++i)
            {
                m_pInt[i] = rhs.m_pInt[i];
            }
        }
        return *this;
    }

    MyVector& operator=(MyVector&& rhs)
    {
        if (this != &rhs)
        {
            if (m_pInt != nullptr)
            {
                delete m_pInt;
            }
            m_Size = move(rhs.m_Size);
            m_pInt = move(rhs.m_pInt);
            rhs.m_Size = 0;
            rhs.m_pInt = nullptr;
        }
        return *this;
    }
};

int main()
{
    MyVector ob1(10);
    // Copy
    MyVector ob2;
    ob2 = ob1; // Asssignment operator
    MyVector ob3 = ob1; // Copy Ctr

    ob1.print();
    // Move
    MyVector ob4(move(ob1));
    MyVector ob5;
    ob5 = move(ob1);

    ob1.print();

    return 0;

}

Default constructor

    MyVector():m_Size(0), m_pInt(nullptr)
    {   }

Stop trying to save space like that. THe point of good coding is to make it readable by humans. The computer can read any style so try and make it more maintainable by making it readable.

I would write it like this:

    MyVector()
        : m_Size(0)
        , m_pInt(nullptr)
    {}

Sure you can use nullptr for the zero sized array. But this seems like a premature optimization. You can make the rest of your code less complicated by never having a null m_pInt. If there is never a null then your code does not need to check for it.

Also currently you have different behavior for:

MyVector  x;
MyVector  x(0);

Constructor

    MyVector(int x)
       // Use the initializer list to initialize the elements.
    {
        m_Size = x;
        m_pInt = new int[m_Size];
        for (int i = 0; i < m_Size; ++i)
        {
            m_pInt[i] = i;
        }
    }

Print

Sure this works. But std::cout is not the only stream you way want to print too

    void print() const
    {
        for (int i = 0; i < m_Size; ++i)
        {
            cout << m_pInt[i] << endl;
        }
    }

So I would pass a stream to print(). It can default to std::cout. Then you should add an operator<<. As the normal way of printing something is via <<.

  MyVector  d(15);
  std::cout << d;

Note: prefer "\n" to std::endl. The difference is a stream flush. The stream will already flush itself when required. So you adding extra flushes is only going to cause the flushes to be less optimal.

Copy Constructor

    MyVector(const MyVector& rhs)
        // Use the initializer list
    {
        m_Size = rhs.m_Size;
        m_pInt = new int[m_Size];
        for (int i = 0; i < m_Size; ++i)
        {
            m_pInt[i] = rhs.m_pInt[i];
        }
    }

Move constructor

    MyVector(MyVector&& rhs):m_Size(move(rhs.m_Size)), m_pInt(move(rhs.m_pInt))
    {
        rhs.m_Size = 0;
        rhs.m_pInt = nullptr;
    }

Yes that works. But you should also mark the move constructor as noexcept. The standard containers have optimizations that can be applied if they know tour class will not throw when being moved. Otherwise they have to fallback to copying to provide the strong exception guarantee.

The standard technique is to use swap though. It makes it look and behave just like move assignment. See below.

Copy Assignment

Yes the test for self assignment looks like a good optimization.

    MyVector& operator=(const MyVector& rhs)
    {
        if (this != &rhs)
        {

BUT its not. Self assignment happens so rarely (in fact basically never in real code) that what you are doing is pesimizing the normal flow and as a result will make your code slower. You do need to cope with self assignment but because it is so rare you don't need to worry that it is not the optimal path.

The second issue I have here is that you destroy the local data before you have a copy of the new data ready.

            if (m_pInt != nullptr)
            {
                delete m_pInt;
            }

If something goes wrong in the rest of your code then you will be unable to roll back the state and thus can not provide the strong exception guarantee. When copying an object it should happen in three distict phases.

1. Copy the source into a temporary object.                  This is dangerous as exceptions can happen. So you must do this without changing the state of the current object.
2. Swap the state of the current object with the temporary.  Swapping is supposed to be exception safe. So you can safely change the state of your object.
3. Now you can release the old resources.                    This is unlikely to go wrong; But even if something goes wrong your object is in a good state.

So your Copy assignment should look like this:

MyVector& operator=(const MyVector& rhs)
{
    // Make a copy to temp
    std::size_t tSize = rhs.m_Size;
    int*        tInt  = new int[tSize];
    std::copy(rhs.m_pInt, rhs.m_pInt + rhs.m_Size, tInt);

    // Now swap with your current object state.
    std::swap(tSize, m_Size);
    std::swap(tInt,  m_pInt);

    // now destroy the old data;
    delete [] tInt;

    // Done
    return *this;
}

If you look carefully at those three stages. Stage 1 looks like the constructor and stage 3 looks like the destructor and stage 2 looks like a standard swap function so we can simplify the above to exactly that:

MyVector& operator=(const MyVector& rhs)
{
    MyVector  temp(rhs);        // 1. Copy
    temp.swap(*this);           // 2. Swap

    // Done
    return *this;
}                               // 3. delete done by destructor.

This is called the copy and swap idiom.

Move Assignment

Again the pesimizing test for self assignment.

    MyVector& operator=(MyVector&& rhs)
    {
        if (this != &rhs)

The standard move assignment is to swap the source and the destination. This has a couple of benefits.

You don't call delete (and thus don't invoke the destructor). Thus it is potentially faster.
Because you did not delete the data there is an opportunity for it to be reused.
If the source is going out of scope it will invoke its destructor and destroy your data but it will be done after the completion of your object thus giving you strong exception guarantee. Thus allowing you to make your assignment operator noexcept.

Standard Move Assignment

MyVector& operator=(MyVector&& rhs) noexcept
{
    rhs.swap(*this);
    return *this;
}

I wrote a series of posts about all this.

Vector - Resource Management Allocation
Vector - Resource Management Copy Swap
Vector - Resize
Vector - Simple Optimizations
Vector - the Other Stuff

Re-Write

class MyVector {
    size_t m_Size;
    int*   m_pInt;
public:
    ~MyVector()
    {
        delete[] m_pInt;
    }

    MyVector(int x = 0)
        : m_Size(x)
        , m_pInt(new int[m_Size])
    {
        for (int i = 0; i < m_Size; ++i)
        {
            m_pInt[i] = i;
        }
    }

    void print(std::ostream& str = std::cout) const
    {
        for (int i = 0; i < m_Size; ++i)
        {
            str << m_pInt[i] << "\n";
        }
    }
    friend std::ostream& operator<<(std::ostream& str, MyVector const& data)
    {
        data.print(str);
        return str;
    }

    MyVector(const MyVector& rhs)
        : m_Size(rhs.m_Size)
        , m_pInt(new int[m_Size])

    {
        std::copy(rhs.m_pInt, rhs.m_pInt + rhs.m_Size, m_pInt);
    }

    MyVector(MyVector&& rhs) noexcept
        : m_Size(0)
        , m_pInt(nullptr)
    {
        rhs.swap(*this);
    }

    MyVector& operator=(MyVector const& rhs)
    {
       MyVector  t(rhs);
       t.swap(*this);
       return *this;
    }

    MyVector& operator=(MyVector&& rhs) noexcept
    {
        rhs.swap(*this);
        return *this;
    }

    void swap(MyVector& other) noexcept
    {
        using std::swap;
        swap(m_Size, other.m_Size);
        swap(m_pInt, other.m_pInt);
    }
    friend void swap(MyVector& lhs, MyVector& rhs)
    {
        lhs.swap(rhs);
    }
};


Dynamic Object Property in C# I have the following code:

if (isMandatory)
{
    x.MandatoryServices.ForEach(y =>
    {
        if (!y.IsSelectedByUser)
        {
            containsRemovedSrvcs = true;
            return;
        }
    });
}
else
{
    x.OptionalServices.ForEach(y =>
    {
        if (!y.IsSelectedByUser)
        {
            containsRemovedSrvcs = true;
            return;
        }
    });
}

I would like to replace it with something more elegant like:

x.{isMandatory ? MandatoryServices : OptionalServices}.ForEach(y =>
{
    if (!y.IsSelectedByUser)
    {
        containsRemovedSrvcs = true;
        return;
    }
});

Of course, the compiler doesn't like that.  I have seen the GetType().GetProperty().GetValue() approach but I don't really know if that will work since my property is a List.  Does anyone know of any better solutions?

A possible solution that is very close to your example could be

var services = isMandatory ? x.MandatoryServices
                           : x.OptionalServices;
services.ForEach(y =>
{
    if (!y.IsSelectedByUser)
    {
        containsRemovedSrvcs = true;
        return;
    }
});

This require that MandatoryServices and OptionalServices can be assigned to the same variable (i.e. same type, shared interface, ..)

I don't understand why you explicitly return return from your lambda expression, since it does not change anything. The "body" of the lambda will be executed for each element in your collection, regardless of your return.

So you could just remove the return:

if (!y.IsSelectedByUser)
{
    containsRemovedSrvcs = true;
}

And since it looks like you just want to see if any element in the collection is not selected by the user, you can use the Any extension method

containsRemovedSrvcs = services.Any(y => !y.IsSelectedByUser);

Applying all this will give you:

var services = isMandatory ? x.MandatoryServices
                           : x.OptionalServices;
containsRemovedSrvcs = services.Any(y => !y.IsSelectedByUser);