Wrapping tuples to be used as keys in a Dictionary I'm currently working on a project where I have to map some objects to several properties.

Basically I need a Dictionary with Tuples as keys and instances of MyClass as values.

As using tuples can quickly lead to an obscure (because of item1, item2... properties) and verbose code I decided to make a class to wrap the tuples.

class Key
{
    private Tuple<string, int, int, int> _impl;

    public Key(string unitID, int address, int comPort, int id)
    {
        _impl = new Tuple<string, int, int, int>(unitID, address, comPort, id);
    }

    public override int GetHashCode()
    {
        return _impl.GetHashCode();
    }

    public override bool Equals(object obj)
    {
        return _impl.Equals(obj);
    }

    public override string ToString()
    {
        return _impl.ToString();
    }
}

That way I should take advantage of the Tuple class and avoid its inconvenients.

So I can write:

Dictionary<Key, MyClass> mapping = new Dictionary<Key, MyClass>();

Instead of:

Dictionary<Tuple<string, int, int, int>, MyClass> mapping = new Dictionary<Tuple<string, int, int, int>, MyClass>();

And look up the dictionary like this:

Key key = new Key("abc", 1, 5, 5464);
MyClass myInstance = mapping[key];

Is this implementation correct ? Is there a more proper way to achieve this ?

public Key(string unitID, int address, int comPort, int id)

Clearly this class represents something much more specialized than an all-purpose "key". Name the type for what it stands for!

private Tuple<string, int, int, int> _impl;

The private field should be readonly. But why don't you have this instead?

private readonly string _unitId;
private readonly int _address;
private readonly int _comPort;
private readonly int _id;

public override int GetHashCode()
{
    return _impl.GetHashCode();
}

Ideally your GetHashCode implementation should rely on immutable fields. Your _impl field isn't marked readonly, which doesn't quite communicate that intent. Using actual fields and dropping the Tuple altogether, you can come up with a more standard GetHashCode implementation.

  That way I should take advantage of the Tuple class and avoid its inconvenients.

You're not getting anything out the Tuple. Get rid of it, it's only obfuscating the class' implementation.

Also, you're not instantiating your Tuple the idiomatic way. This:

_impl = new Tuple<string, int, int, int>(unitID, address, comPort, id);

Should be written like this and leverage generic type parameter inference:

_impl = Tuple.Create(unitID, address, comPort, id);


How can I optimize this code that finds the GCD? int count;
int gcdCount;
int testCase = 5;

while (testCase > 0)
{
    int n = 5;
    count = 0;
    gcdCount = 0;
    // to get two  random numbers a and b a<=n and b<=n
    //get probablity of gcd(a,b) ==b
    for (int i = 1; i <= n; i++)
    {
        for (int j = 1; j <= n; j++)
        {
            count++;
              // check if gcd(i,j) is equal to second number j
            if (findgcd(i, j) == j)
            {
                gcdCount++;
            }
        }
    }
     //return probability of gcdcount
    System.out.println(gcdCount + "/" + count);
    testCase--;
}

Assuming that findgcd is correctly implemented and that i and j are guaranteed to both be non-negative, findgcd(i, j) == j is equivalent to j != 0 && i % j == 0. That allows a further optimisation to a single loop, because the number of values in 1..n which are divisible by j is floor(n/j):

for (int j = 1; j <= n; j++)
{
    count += n;
    gcdCount += n / j;
}


Thread safe object pool, is this object pool thread safe? If many threads Borrow() and Return() an instance of Packet from/to PacketPool could the Exception in Packet.Init() ever be thrown? Assuming only PacketPool ever called the Init() and UnInit() methods on a Packet.

class PacketPool
{
    private Stack<Packet> pool;

    public PacketPool(int initalNum)
    {
        pool = new Stack<Packet>(initalNum);

        for (int i = 0; i < initalNum; i++)
        {
            Packet p = new Packet();
            pool.Push(p);
        }
    }

    public Packet Borrow()
    {
        Packet p;

        lock (pool)
        {
            if (pool.Count == 0)
            {
                p = new Packet();
            }
            else
            {
                p = pool.Pop();
            }
        }

        p.Init();

        return p;
    }

    public void Return(Packet p)
    {
        p.UnInit();

        lock (pool)
        {
            pool.Push(p);
        }
    }
}

class Packet
{
    bool isInitialized;

    public void Init()
    {
        if(isInitialized)
            throw new InvalidOperationException("Already initialized!");
        isInitialized = true;
    }

    public void UnInit()
    {
        isInitialized = false;
    }
}

Assuming only PacketPool ever called the Init() and UnInit() methods on a Packet.

I think that's not an assumption you should make. If the user of your code shouldn't call some methods, then don't let him do it, don't just assume that he won't.

What you could do is to return a facade to Packet that doesn't contain the two methods, instead of returning Packet directly.


Guess a random number between 1 and 100 I want some recommendations or something I could add to the game. Please tell me what I could do better or what I did wrong. I made this during the time I learned Python.

import random
num = random.randint(1, 100)
while True:
    print('Guess a number between 1 and 100')
    guess = input()
    i = int(guess)
    if i == num:
        print('You won!!!')
        break
    elif i < num:
               print('Try Higher')
    elif i > num:
               print('Try Lower')
#any recommendations for the game end
print('if you gussed less than 6 times you won')

Proper integer conversion

Right now, as it stands, you're just converting any user input to a integer, using the int function. What do you suppose happens if the user enters something like "abc"?

What you need to do is set up a try-except block, like this:

try:
    user_integer = input("Enter an integer: ")
    user_integer = int(user_integer)
except ValueError:
    print("You must enter a valid integer!")

To set up something like this in your code, you'd change your code to something like this:

...

while True:
    print("Guess a number between 1 and 100.")
    guess = input()

    try:
        integer_guess = int(guess):

        ...
    except ValueError:
        ...

Tracking the number of "rounds"/"tries"

Rather than printing a message saying that if the user got below a certain amount of tries, they win, you can implement it into the code. The easiest way to do this would be to use a for ... in range( ... ) loop, like this:

rounds = ...
for _ in range(rounds):
    ...

(This has been implemented below, for reference.)

Design

This is not a very extensible design, again, I'd recommend creating a function that allows you to create custom games, like this:

def number_guessing_game(low, high, rounds):
    print("Guess a number between {low} and {high}. You have {rounds} rounds to try and guess correctly.".format(low=low, high=high, rounds=rounds))
    number = random.randint(low, high)

    for _ in range(rounds):
        guess = input("Enter an integer: ")

        try:
            integer = int(guess)
            if integer == number:
                print('You won!!!')
                return
            elif integer < number:
                print('Try Higher')
            elif integer > number:
                print('Try Lower')

        except ValueError:
            print("You must enter a valid integer.")

    print("You didn't guess correctly in {rounds} rounds. You lost.".format(rounds=rounds))

An example function call might look like this:

number_guessing_game(1, 100, 6)

In short, all of your code becomes the following:

import random

def number_guessing_game(low, high, rounds):
    print("Guess a number between {low} and {high}. You have {rounds} rounds to try and guess correctly.".format(low=low, high=high, rounds=rounds))
    number = random.randint(low, high)

    for _ in range(rounds):
        guess = input("Enter an integer: ")

        try:
            integer = int(guess)
            if integer == number:
                print('You won!!!')
                return
            elif integer < number:
                print('Try Higher')
            elif integer > number:
                print('Try Lower')

        except ValueError:
            print("You must enter a valid integer.")

    print("You didn't guess correctly in {rounds} rounds. You lost.".format(rounds=rounds))

number_guessing_game(1, 100, 6)

Hope this helps!


Is this a qualified OOP script in PHP? class Youtube
{
    private $url = null;
    private $id;

    public function __construct($url)
    {
        $this->url = $url;
        $this->parseUrl($url);
    }

    protected function parseUrl($url)
    {

        if (preg_match('%(?:youtube(?:-nocookie)?\.com/(?:[^/]+/.+/|(?:v|e(?:mbed)?)/|.*[?&]v=)|youtu\.be/)([^"&?/ ]{11})%i', $url, $match)) {
            $this->id = $match[1];
        }
        else
        {
            throw new exception('Cannot find an ID!');
        }

    }

    public function getVideo($width='560', $height='315')
    {
        $video = <<<EOF
        <iframe width="$width" height="$height" src="//www.youtube-nocookie.com/embed/$this->id" frameborder="0" allowfullscreen></iframe>
EOF;

        return $video;
    }

}

$sistar = new Youtube('http://www.youtube.com/watch?v=JtVhwsACgTw');
print $sistar->getVideo();

$twoneone = new Youtube('http://www.youtube.com/watch?v=LUrUPzLm5SI');
print $twoneone->getVideo();

This is my first object-oriented script in PHP (I hope it's not too bad in general). I know I should do more validation and commenting within the code. Do you have any suggestions for improving the quality of the code?

Pinoniq has the right idea, which can be taken a bit further:

VideoFactory $vf = new VideoFactory();
Video $video = $vf->createVideo( "http://youtube.com/..." );
$video->setDimensions( 640, 480 );
$video->embed();

Here, the VideoFactory can parse a URL. The createVideo method would return a subclass of Video, which is a YouTubeVideo. However, the implementation need not be dependent on YouTube. This allows you to also write:

VideoFactory $vf = new VideoFactory();
Video $video = $vf->createVideo( "http://vimeo.com/..." );
$video->setDimensions( 640, 480 );
$video->embed();

You'll note that the only item that changed is the URL, but both video formats can be embedded.


Tame this Beast: TSQL Unpivot Okay... here's the beast:

SELECT
     SUBSTRING(DischDate, 7, 4) + SUBSTRING(DischDate, 1, 2) as YYYYMM
    ,Type
    ,SubType
    ,Diags
    ,Count(*)        as Count
    ,SUM(Charges)    as Charges
    ,SUM(Payments)    as Payments
FROM
    (
        SELECT DISTINCT
             ID
            ,Diags
        FROM
           (SELECT VisitID as ID
                    ,DX01 ,DX11 ,DX21, DX31
                    ,DX02 ,DX12 ,DX22, DX32
                    ,DX03 ,DX13 ,DX23, DX33
                    ,DX04 ,DX14 ,DX24, DX34
                    ,DX05 ,DX15 ,DX25, DX35
                    ,DX06 ,DX16 ,DX26, DX36
                    ,DX07 ,DX17 ,DX27, DX37
                    ,DX08 ,DX18 ,DX28, DX38
                    ,DX09 ,DX19 ,DX29, DX39
                    ,DX10 ,DX20 ,DX30, DX40
               FROM [AGH00]...[20110128 - AGH00#TXT]) p
            UNPIVOT
               (Diags FOR DX IN
                    (DX01 ,DX11  ,DX21, DX31
                    ,DX02 ,DX12  ,DX22, DX32
                    ,DX03 ,DX13  ,DX23, DX33
                    ,DX04 ,DX14  ,DX24, DX34
                    ,DX05 ,DX15  ,DX25, DX35
                    ,DX06 ,DX16  ,DX26, DX36
                    ,DX07 ,DX17  ,DX27, DX37
                    ,DX08 ,DX18  ,DX28, DX38
                    ,DX09 ,DX19  ,DX29, DX39
                    ,DX10 ,DX20  ,DX30, DX40)
            )AS unpvt
        ) as DIAGS
    LEFT JOIN [AGH00]...[20110128 - AGH00#TXT] as A0 on DIAGS.ID = A0.VisitID
    LEFT JOIN ( SELECT VisitID, Sum(ChargsAmt) as Charges
                FROM [AGH00]...[20110128 - AGH00Chg#TXT]
                Group By VisitID ) as AC on A0.VisitID = AC.VisitID
    LEFT JOIN ( SELECT VisitID, Sum(Pmt) as Payments
                FROM [AGH00]...[20110128 - AGH00Pmt#TXT]
                Group By VisitID ) as AP on A0.VisitID = AP.VisitID
Group By
     SUBSTRING(DischDate, 7, 4) + SUBSTRING(DischDate, 1, 2)
    ,Type
    ,SubType
    ,Diags
Order By
     SUBSTRING(DischDate, 7, 4) + SUBSTRING(DischDate, 1, 2)
    ,Type
    ,SubType
    ,Diags

Sample output:

YYYYMM Type SubType Diags # Charges    Payments
201004 I    SPEC    5185  3 224086.60  222986.60
200910 I    MONT    73390 2 7573.42    6984.13
200910 O    AHCW    463   4 578.00     578.00
200910 I    MEDI    2766  2 67444.39   67444.39

Notes:

SQL2005 LinkedServer to a TextFile with a backing ini for field descriptions.
DischDate is "mm/dd/yyyy" text field. I want YYYYMM for grouping (as well as other fields)

First "table" is a list generated via Unpivot that basically generates a list of VisitID's and unique Diags
Left Join main customer data by "VisitID"
Left Join Charges Data (Grouped to get individual charges grouped by "customer" aka VisitID)
Left Join Payment Data (same as last)
Group by/Order By/Select Date YYYYMMDD, Type, Subtype, Diags... and grouped Count/Sums

More information is in this SO question where someone pointed me to the unpivot option. Including a semi-confused explanation of what I want to accomplish (basically grouping information for a couple similar reports).

Just wondering if there is a more graceful way, and if anyone see potential errors.

This looks like a good use for UNPIVOT. And you can use common table expressions to avoid repeating yourself. Like this:

WITH DIAGS AS
(
    SELECT DISTINCT VisitID as ID, Diags
    FROM
    (
        SELECT *
        FROM [AGH00]...[20110128 - AGH00#TXT]
    ) p
    UNPIVOT
    (
        Diags FOR DX IN
        (
            DX01, DX11, DX21, DX31
            , DX02, DX12, DX22, DX32
            , DX03, DX13, DX23, DX33
            , DX04, DX14, DX24, DX34
            , DX05, DX15, DX25, DX35
            , DX06, DX16, DX26, DX36
            , DX07, DX17, DX27, DX37
            , DX08, DX18, DX28, DX38
            , DX09, DX19, DX29, DX39
            , DX10, DX20, DX30, DX40
        )
    ) AS unpvt
),
A0 AS
(
    SELECT VisitID, DischDate, [Type], SubType
    FROM [AGH00]...[20110128 - AGH00#TXT]
),
AC AS
(
    SELECT VisitID, SUM(ChargsAmt) AS Charges
    FROM [AGH00]...[20110128 - AGH00Chg#TXT]
    GROUP BY VisitID
),
AP AS
(
    SELECT VisitID, SUM(Pmt) AS Payments
    FROM [AGH00]...[20110128 - AGH00Pmt#TXT]
    GROUP BY VisitID
)
VISITS AS
(
    SELECT SUBSTRING(A0.DischDate, 7, 4) + SUBSTRING(A0.DischDate, 1, 2) AS YYYYMM
        , A0.[Type]
        , A0.SubType
        , DIAGS.Diags
        , AC.Charges
        , AP.Payments
    FROM DIAGS
        LEFT JOIN A0 ON DIAGS.ID = A0.VisitID
        LEFT JOIN AC ON A0.VisitID = AC.VisitID
        LEFT JOIN AP ON A0.VisitID = AP.VisitID
)
SELECT YYYYMM, [Type], SubType, Diags
    , COUNT(*) AS [Count]
    , SUM(Charges) AS Charges
    , SUM(Payments) AS Payments
FROM VISITS
GROUP BY YYYYMM, [Type], SubType, Diags
ORDER BY YYYYMM, [Type], SubType, Diags


Find the fastest run of each year from a list of running results Problem Explained:

Imagine there is a list of running results of 1000m, like following:

[
    { id: 1, date: '2017-01-01 00:00:00', duration: 195 },
    { id: 2, date: '2017-01-10 00:00:00', duration: 270 },
    { id: 3, date: '2017-03-12 00:00:00', duration: 220 },
    { id: 4, date: '2018-01-10 00:00:00', duration: 218 },
    { id: 5, date: '2018-02-23 00:00:00', duration: 220 },
    { id: 6, date: '2018-05-18 00:00:00', duration: 215 }
]

I'd like to find out the fastest run of each year, which will be

[
    { id: 1, date: '2017-01-01 00:00:00', duration: 195 },
    { id: 6, date: '2018-05-18 00:00:00', duration: 215 }
]

Here's my ugly solution (TypeScript):

const items = JSON.parse('Use the JSON above.');
const results: any[] = [];
const fastestOfEachYear: object = {};
const idsOfFastestEachYear: object = {};

// Go through the list to find the fastest of each year.
// Save the IDs in an object.
items.forEach((item) => {
    const runId = item['id'];
    const date = item['date'];
    const dateParts = date.split('-');
    const year = new Date(dateParts[0], dateParts[1] - 1, dateParts[2]).getFullYear();
    const duration = item['duration'];

    // Found a faster one, update our objects with the new item.
    if (fastestOfEachYear[year] == null || fastestOfEachYear[year] > duration) {
        fastestOfEachYear[year] = duration;
        idsOfFastestEachYear[year] = runId;
    }
});

// Retrieve the IDs of the runs that are the fastest of a year.
const idsOfFastest = Object.keys(idsOfFastestEachYear).map((key) => idsOfFastestEachYear[key]);

// Loop through the list again to find those items matching the IDs
// Save them to results.
items.forEach((item) => {
    const runId = item['id'];
    if ($.inArray(runId, idsOfFastest) !== -1) {
        results.push(item);
    }
});

Surely there are nicer and better ways of achieving this? (Reviews in TypeScript, ES6 or even just Vanilla JS are ok.)

First solution using .forEach, for...in and .sort

Here what I did to achieve this:

First sort your array of objects, i.e convert:

data = [ {...}, {...}, {...}, {...}, {...}, {...} ]

into an object sorted by year:

sortedY = { 2017: [{...}, {...}, {...}], 2018: [{...}, {...}, {...}] }

You can achieve such result by using a forEach loop:

const sortedY = {};
data.forEach(e => {
    const year = e.date.split('-')[0];
    sortedY[year] = sortedY[year] || [];
    sortedY[year].push(e);
});

Then we will need to sort each object [{...}, {...}, {...}] per year by duration. sorted is not an array. We can't use map unfortunately: use a for...in loop instead. However to sort each key/value (<=> year/array of results for this year) we can use one of Array's methods: sort, the sorting criteria is the following:

(a,b) => a.duration > b.duration

Where a and b are two different races from the same year.

So with:

sortedY[year].sort((a,b) => a.duration > b.duration)

You get an array of sorted races for the year year.

The last thing to do is saving the first of the sorted array (the first is the one with the shortest duration, so select it with [0] and add it to result:

result[year] = sortedY[year].sort((a,b) => a.duration > b.duration)[0]

The final code is:

const sortedY = {};
data.forEach(e => {
    const year = e.date.split('-')[0];
    sortedY[year] = sortedY[year] || [];
    sortedY[year].push(e);
});

const result = {};
for(year in sortedY) {
   result[year] = sortedY[year].sort((a,b) => a.duration > b.duration)[0];
}

console.log(result);
<script>
const data = [ { id: 1, date: '2017-01-01 00:00:00', duration: 195 }, { id: 2, date: '2017-01-10 00:00:00', duration: 270 }, { id: 3, date: '2017-03-12 00:00:00', duration: 220 }, { id: 4, date: '2018-01-10 00:00:00', duration: 218 }, { id: 5, date: '2018-02-23 00:00:00', duration: 220 }, { id: 6, date: '2018-05-18 00:00:00', duration: 215 } ]
</script>

Second solution using only .forEach

I realised there was a quicker way of doing this: we don't need to loop over the array data twice! One loop will suffice. Indeed, we'll fill result as we navigate through data only replacing a yearly race if a race is shorter in duration.

So a race will be added to result[year] only if:

!result[year] || (result[year] && e.duration < result[year].duration)

is true, i.e. if either:

there is no race registered for the year year
there is one but its duration is longer than the one that we're checking

So the code is shorter, simpler and I bet it will be quicker: its complexity is \$O(n)\$. The previous code was cool but not so much efficient. I was looping twice the data: first time for the sorting, second on the sorted array. Not even mentioning that I was using sort...

let result = {};
data.forEach(e => {
  const year = e.date.split('-')[0];
  if( !result[year] || (result[year] && e.duration < result[year].duration) ) {
    result[year] = e;
  }
})

console.log(result);
<script>
const data = [ { id: 1, date: '2017-01-01 00:00:00', duration: 195 }, { id: 2, date: '2017-01-10 00:00:00', duration: 270 }, { id: 3, date: '2017-03-12 00:00:00', duration: 220 }, { id: 4, date: '2018-01-10 00:00:00', duration: 218 }, { id: 5, date: '2018-02-23 00:00:00', duration: 220 }, { id: 6, date: '2018-05-18 00:00:00', duration: 215 } ]
</script>


Golang visitor pattern for marshaling shapes as JSON, and XML I was trying to implement visitor pattern in Golang, and came across an idea of using a generic func type instead if interface as visitor. Here is my code:

package main

import (
    "encoding/json"
    "encoding/xml"
    "fmt"
)

type Visitor func(shape Shape)

type Shape interface {
    accept(Visitor)
}

type Circle struct {
    Rad int
}

func (c Circle) accept(v Visitor) {
    v(c)
}

type Line struct {
    Length int
}

func (l Line) accept(v Visitor) {
    v(l)
}

func JsonVisitor(shape Shape) {
    bytes, err := json.Marshal(shape)
    if err != nil {
        panic(err)
    }
    fmt.Println(string(bytes))
}

func XmlVisitor(shape Shape) {
    bytes, err := xml.Marshal(shape)
    if err != nil {
        panic(err)
    }
    fmt.Println(string(bytes))
}

func main() {
    circle := Circle{12}
    line := Line{42}
    circle.accept(JsonVisitor)
    line.accept(JsonVisitor)

    circle.accept(XmlVisitor)
    line.accept(XmlVisitor)
}

Besides the code review I have a question: is it still considered a Visitor pattern if using one generic function for all the Shapes?

General things:

The visitor pattern is more suited for object-oriented languages. Whether Go is object-oriented is up for debate, but fact is that many OOP patterns are implemented differently in it (or don't make sense at all).
Specifically, I don't see an advantage to XmlVisitor and JsonVisitor over xml.Marshal and json.Marshal. That said, design is beyond the scope of codereview SE, so I'll assume you have a good reason.
Create a separate package rather than adding to main. I understand this is something you'd do eventually, but it's good practise to do it from the very start.

type Visitor func(shape Shape)

Do you actually need this to be a type? I see little advantage to it (slightly shorter lines perhaps) and this effectively hides from the user what Visitor really is. func (c Circle) accept(v func(Shape)) is self-documenting and easier to read. If you must have your own type, name it VisitorFunc at least, to make it clear what it is.

type Shape interface {
    accept(Visitor)
}

Probably not the best name - it can be anything, doesn't neccessarily need to be shape. I'd go with e.g. "Visitable", but the proper name per Effective Go would be "Accepter" since it contains a single method (use your best judgement to decide which name is better).

More importantly, accept shouldn't be private - you won't be able to call private methods outside of the package where the type is defined.

func (c Circle) accept(v Visitor) {
    v(c)
}

You probably want to pass by reference (pointer): func (c *Circle) accept.... The way it is now, your visitor is read-only and also more costly, as the whole object is copied.

func JsonVisitor(shape Shape) {
    bytes, err := json.Marshal(shape)
    if err != nil {
        panic(err)
    }
    fmt.Println(string(bytes))
}

func XmlVisitor(shape Shape) {
    bytes, err := xml.Marshal(shape)
    if err != nil {
        panic(err)
    }
    fmt.Println(string(bytes))
}

Don't panic(...). panic is used for unrecoverable errors - you'll only use it if you can answer "yes" to "am I confident that, whenever this errors happens, it's so severe that the best thing to do is just crash". Instead, have your accept method, your Visitor type and these functions all return error.


Pokemon search engine I am making a simple function to find and match any possibilities what Pokémon the user wants. This is accomplished by taking in the input (what the user thinks the Pokémon is named) and compare its spelling with preset Pokémon names (letter by letter searching). With each successful match, the Pokémon's score (determined by its value in the dictionary) will go up by one. Only the top seven Pokémon are returned.

The function:

import re
import operator

# Dictionary of Pokemon
list_of_chars = {"Bulbasaur": 0, "Ivysaur": 0, "Venusaur": 0, "Squirtle": 0, "Wartotle": 0, "Blastoise": 0,
                 "Charmander": 0, "Charmeleon": 0, "Charizard": 0, "Zapdos": 0, "Articuno": 0, "Moltres": 0, "Mew": 0,
                 "Mewtwo": 0}
list_of_keys = []

# Allow the list to actually contain the keys
for key in list_of_chars:
    list_of_keys.append(str(key))

def indiv_search(entry):
    certain_list = []
    # Loop through the possible Pokemon
    for i in range(0, len(list_of_keys)):
        # Loop through each *letter* of the entry
        for b in range(0, len(entry)):
            try:
                # Match each letter of entry with the letter of the Pokemon
                m = re.search(str(entry[b]), list_of_keys[i][b], re.IGNORECASE)
            except IndexError:
                continue
            if m is not None:
                # Match is successful
                list_of_chars[list_of_keys[i]] += 1
            elif m is None:
                # Match is not successful
                continue
    # "Sort" dictionary by score values
    sorted_x = sorted(list_of_chars.items(), key=operator.itemgetter(1))
    # Take only top seven choices
    while len(sorted_x) > 7:
        del sorted_x[0]
    for i in range(6, -1, -1):
        certain_list.append(sorted_x[i][0])
    # Return results
    return "Possible Choices: " + ", ".join(certain_list)

while True:
    # Take input, keep in string format
    poke_name = str(raw_input("Pokemon's Name: "))
    if poke_name == "QUIT":
        break
    else:
        # Print results
        print indiv_search(poke_name)
        # Reset all values in dictionary
        for i in range(0, len(list_of_chars)):
            list_of_chars[list_of_keys[i]] = 0

Example output:

Pokemon's Name: Char
Possible Choices: Charmander, Charizard, Charmeleon, Blastoise, Articuno, Venusaur, Zapdos

Note that the first line contains the raw_input() prompt and the input (separated by a space due to the prompt purposely having a space at the end to improve readability). The second line has the results of the function.

I have a few questions:

Is the code Pythonic?
What can be improved to uphold PEP rules?
What can be done to improve the efficiency and if possible, accuracy of the code?

There are much simpler ways to do this.    The easy way is to use difflib.get_close_matches() which is very similar to what you are creating.  If you insist on making your own, there are still simpler ways.

First of all, why regex?  You don't have patterns to match.  You have simple equality tests.  The only thing that makes it different from a in b is re. IGNORE_CASE.  Simply fixed, do a.lower() in b.lower().  In this case, however, you actually mean a.lower() == b.lower().

Iterating through two sequences in parallel is a common enough task that Python made it easy with zip().  Instead of

for i in range(len(a))):

with a[i] and b[i], you can use

for a_item, b_item in zip(a, b):

Conveniently, it stops at the end of the shortest iterable (not in Python 3).

Your method of limiting the number of matches returned is not ideal. Instead of deleting the ones you don't want, keep only the ones that you do. That is done simply with a slice.

In Python, the bool class (for True and False) inherits from the int class. Therefore, you can do arithmetic with booleans.  True is 1, and False is 0. With that knowledge and the earlier improvements, we get something much simpler:

def diff_score(iter_a, iter_b):
    return sum(a == b for a, b in zip(iter_a.lower(), iter_b.lower()))

def best_matches(guess, names, num_matches):
    matches = sorted(names, key=lambda x: diff_score(x, guess), reverse=True)
    return  matches[:num_matches]


Fade-in and fade-out in pure JavaScript I decided to build my own fade-in fade-out function, since that is all I need on my page.

Please comment on things I can make better.

<!DOCTYPE html>

<title></title>
<meta charset="utf-8" />

        Fade In |
        Fade Out

Fading Box

Just some generic notes about the JavaScript code: I'd extract out a setOpacity function and create a fadeOut and a fadeIn function too.

function setOpacity(opacity) {
    fading_div.style.opacity = opacity / 100;
    fading_div.style.filter = 'alpha(opacity=' + opacity + ')';
}

function fadeOut(opacity) {
    setOpacity(opacity);
    if (opacity == 1) {
        fading_div.style.display = 'none';
        done = true;
    }
}

function fadeIn(opacity) {
    setOpacity(opacity);
    if (opacity == 1) {
        fading_div.style.display = 'block';
    }
    if (opacity == 100) {
        done = true;
    }
}

...
setTimeout("fadeIn(" + i + ")", i * 5);
...

It eliminates the in and out magic constants and lots of conditions which checks their values.

Using variable name suffixes like opacity_value looks a little bit redundant (since variables stores values), so I've renamed them. The same is true for the function_opacity function.

I've changed the second for loop too to use fewer arithmetic operations, I think the following is easier to read:

for (var i = 1; i <= 100; i++) {
    setTimeout("fadeOut(" + (100 - i) + ")", i * 5);
}


Producer-Consumer in C++ This is my first experimentation with the producer-consumer problem (and thread manipulation in general), so any feedback on taboos and fixes is greatly appreciated!

#include <condition_variable>
std::condition_variable cv;

#include <iostream>
#include <random>

#include <mutex>
std::mutex mtx;

#include <thread>

// standard in all of my programs
void panic(const std::string &msg) {
    throw std::runtime_error("Error: " + msg + "\n");
}

int count = 0, buff_size = 0;
char* buff;

int random_int(int lower_bound) {
    std::random_device seed;
    std::mt19937 generator(seed());
    std::uniform_int_distribution<int> dist(lower_bound, std::nextafter(26, DBL_MAX));

    return dist(generator);
}

char random_char(int lower_bound) {
    return 'A' + (random_int(lower_bound) % 26);
}

/* Consumer

Reads characters from the buffer and prints them.

*/
void consume(int job) {
    std::unique_lock<std::mutex> lck(mtx);

    while (count == 0) {
        cv.wait(lck);
    }

    /*
    job + 1 = Running
    job = Ready
    */
    for (int i = 0; i < buff_size; i++) {
        std::cout << buff[i] << std::endl;
    }

    count--;
}

/* Producer

Randomly generates letters at (pos > buff_size & pos <= 26),
inserts them at the next available position in the buffer,
and then prints out the lowercase form of the inputted letter.

*/
void produce(int job) {
    std::unique_lock<std::mutex> lck(mtx);

    for (int i = 0; i < buff_size; i++) {
        buff[i] = random_char(buff_size);
        std::cout << static_cast<char>(tolower(buff[i])) << std::endl;
    }

    count++;
    cv.notify_one();
}

int main() {
    int buf_size = 0;

    std::cout << "The Producer-Consumer Problem (in C++11!)" << std::endl << "Enter the buffer size: ";
    std::cin >> buf_size;

    if (buf_size > 0 && buf_size <= 26) {
        // set the buffer size
        buff_size = buf_size;
        buff = new char[buff_size];
    }
    else {
        // rage quit
        panic("The range must be > 0 and <= 26.");
    }

    std::thread production[10], processed[10];

    /* Initialize the arrays */
    for (int order = 0; order < buff_size; order++) {
        production[order] = std::thread(produce, order);
        processed[order] = std::thread(consume, order);
    }

    /* Join the threads to the main threads */
    for (int order = 0; order < buff_size; order++) {
        processed[order].join();
        production[order].join();
    }
}

#include <condition_variable>
std::condition_variable cv;

#include <iostream>
#include <random>

#include <mutex>
std::mutex mtx;

#include <thread>

Don't mix includes and code, makes it easy to miss those globals you're defining in the middle. Keep the headers by themselves sorted in some order or grouped logically/by function as you prefer.

throw std::runtime_error(...);

You're missing <stdexcept>, your code could fail to compile.

int count = 0, buff_size = 0;
char* buff;

What are these? Why are they global? Why initialize two explicitly and not the third?

int random_int(int lower_bound) {
    std::random_device seed;
    std::mt19937 generator(seed());
    std::uniform_int_distribution<int> dist(lower_bound, std::nextafter(26, DBL_MAX));

    return dist(generator);
}

This isn't good. You're seeding a generator each time you generate a new random number - that's not how you're supposed to do it. You're supposed to seed a generator once, then use it repeatedly.

std::nextafter(26, DBL_MAX)

You're missing <cfloat> for DBL_MAX, and I really don't understand what this is for, or why 26 appears magically here with no warning (no indication in the function name, no comment). nextafter/before are really meant for floating point numbers (or template code where you don't know if you'll get ints or floats). If it's a plain int constant, just code it in. Actually, don't put that magic constant at all and allow the user to pass the lower and upper bound.

char random_char(int lower_bound) {
    return 'A' + (random_int(lower_bound) % 26);
}

(This only works for encodings where the letters are consecutive. This isn't guaranteed, but chances that your code will run on something with e.g. EBCDIC are small.)

Why %26 if your RNG already gives you values in the right range? The % trick is likely to ruin your distribution's statistical properties. So again, just pass the min and max to your random generating function (e.g. return myrand('A','Z');).

Here's one way to fix this:

// bounds are inclusive
char rand_char(char lower_bound, char upper_bound)
{
  thread_local std::random_device rd;
  thread_local std::mt19937 gen(rd());
  thread_local std::uniform_int_distribution<int> dist(lower_bound,
                                                        upper_bound);
  return static_cast<char>(dist(gen));
}

Then you can call:

rand_char('A', 'Z');

thread_local gives each thread its own generator and distribution, initialized only once per thread on first call of the function.

/* Producer

Randomly generates letters at (pos > buff_size & pos <= 26),
inserts them at the next available position in the buffer,
and then prints out the lowercase form of the inputted letter.

*/

This comment does not match the code, the code doesn't contain a pos at all. There's a job parameter, but it is not used. There does not appear to be any "next available" concept in the code. There is no "inputted letter" either. And if pos > buff_size, generating something at buff[pos] would be an overflow. So fix that comment (and fix the code to match it) or remove it. Bad comments are worse than no comment at all.

int buff_size = 0;
// ...
int main() {
  int buf_size = 0;

That's just evil! Are you running in an underhanded C++ contest? :-) Really that's a terrible choice of variable names.

std::cout << "The Producer-Consumer Problem (in C++11!)" << std::endl << "Enter the buffer size: ";
std::cin >> buf_size;
if (buf_size > 0 && buf_size <= 26) {
  // ...
} else {
  // rage quit
  panic("The range must be > 0 and <= 26.");
}

Bad user experience: inform about the limits before asking for the input.

buff = new char[buff_size];

Where's the delete[]? If your comment above produce was what you intended, you'd need to initialize too, otherwise your producers could read uninitialized data (which leads to undefined behavior). Use a std::vector instead to avoid the leaks and initialize all values to a sane default (e.g. a space).

As for the whole producer/consumer thing: your code is completely serial. There can only be one active thread, whether it is a producer or a consumer. Multiple produce threads could run one after the other without a consumer running in between, so you're "losing products" and it is likely that you'll see the same "product" output multiple times.

You should change your code so that the producers produce something predictable (e.g. "produce" and print out their job number) so that you can actually visually verify whether things are working. Then test your code with starting all the consumers first (and waiting a tiny bit), then the consumers. Then do the opposite. Run it several times, always checking that the output is correct. Increase the concurrency. Use sanitizers if you can (-fsanitize option for recent-enough GCC and clang for instance).


More elegant way to get timestamp from day/hour? (Perl) http://coolwx.com/buoydata/data/curr/all.html provides weather data
from ships (or maybe buoys) at sea, but the date is given as "25/18" (meaning the 18th
hour of the 25th day of the month, all times GMT).

I need to convert this to a full timestamp ("2013-09-25
18:00:00"). All reports are in the near past. One glitch: if today is the
1st, and the report is "31/18" for example, it's referring to the 31st
of last month.

Here's my solution, which I feel is kludgey. Can it be improved?

sub ship2time {
  my(@now) = gmtime(time());
  # assumed given as "foo/bar";
  my($day,$hour) = split(/\//, $_[0]);
  # today's date (GMT)
  my($today) = strftime("%d", @now);
  # if the given day is in the past (or very near future) it's this month
  if ($day <= $today+1) {
    return strftime("%Y-%m-$day $hour:00:00", @now);
  }

  # otherwise, we're referring to last month (eg, today is the 1st,
  # report says 31st)
  my($year, $month) = split(/\-/, strftime("%Y-%m", @now));
  $month--;
  if ($month < 0) {$year--; $month=12;}

  return "$year-$month-$day $hour:00:00";
}

There's a timestamp file available here: http://coolwx.com/buoydata/data/curr/timestamp, so I think it'll be easier if you download and parse both data and timestamp files.

You can parse the timestamp with this with %Y%m%d%H pattern, for example using core Time::Piece module and its strptime function:

use Time::Piece;
my $time = Time::Piece->strptime("2013092616", "%Y%m%d%H"); # Time::Piece object, see docs
say $time->datetime; # outputs: 2013-09-26T16:00:00

See documentation for more formatting methods of Time::Piece object.


Kth to last element of a singly linked list Find the kth to last element of a singly linked list

Any comments?

import org.junit.Test;

public class Solution {
  // Find kth to last element of a singly linked list
  // Solution proposed: Use a runner that is k steps ahead
  // when runner hits end of linked list, slow on kth to end element

  public Node n3 = new Node(3);
  public Node n2 = new Node(2,n3);
  public Node n1 = new Node(1,n2);
  public Node head = new Node(0,n1);

  public Node findKthToLast(Node head, int k){
    // place the runner k steps ahead
    if (head == null){
      return null;
    }

    Node runner = new Node(head);
    Node slow = new Node(head);

    for (int i = 0; i < k; i++){
      if (runner.getNext() == null){
        return null;
      }
      else{
        runner = runner.getNext();
      }
    }

    while (runner != null){
      runner = runner.getNext();
      slow = slow.getNext();
    }

    return slow;
  }

  @Test
  void test1th(){
    System.out.println(findKthToLast(head, 1).getValue());
  }

  @Test
  void testLargerThanList(){
    System.out.println(findKthToLast(head, 15));
  }

  @Test
  void testNonDegenerate(){
    System.out.println(findKthToLast(head, 2).getValue());
  }

  public static void main(String[] args) {
    Solution e = new Solution();
    e.test1th();
    e.testLargerThanList();
    e.testNonDegenerate();
  }

}

class Node {
  private int value;

  private Node next;

  public Node(int v) {
    value = v;
  }

  public Node(int v, Node n) {
    value = v;
    next = n;
  }

  public Node(Node n){
    this.value = n.value;
    this.next = n.next;
  }

  public Node getNext(){
    return next;
  }
  public int getValue(){
    return value;
  }
  public void setNext(Node n){
    this.next = n;
  }
}

Your code is neat, and, within the confines of the "Solution", you have solved things well.

The algorithm you use is the right one, it's implemented well, and it's all clear. I like it.

There's only one thing that concerns me. The interface of the Node class:

The value of the node should be final (not just private).
The Node class itself should be private, and should not be passed in to the findKthToLast method (it should be accessed as the private field it is)
the return value of the findKthToLast should be the node's value, not the node itself.

There are all concerns that are about the implementation environment, not just the implementation itself. The Solution concept may be the issue.

So, taking that all in to consideration, your solution, given the constraints, is a good one, but the Solution environment and requirements themselves may be the worst of the problems.


Flood fill algorithm I am implementing a flood fill algorithm using Python and NumPy.  I have written the following fill function which works fine:

def fill(self, data, xsize, ysize, x_start, y_start):

    stack = [(x_start, y_start)]

    while stack:
        x, y, stack = stack[0][0], stack[0][1], stack[1:]

        if data[x, y] == 1:
            data[x, y] = 2
            if x > 0:
                stack.append((x - 1, y))
            if x < (xsize - 1):
                stack.append((x + 1, y))
            if y > 0:
                stack.append((x, y - 1))
            if y < (ysize - 1):
                stack.append((x, y + 1))

I wanted to implement a way that would eliminate the need to re-check a point if it had already been checked. By creating a new list of checked points and then comparing to see if the current point is already in that list, but this ended up making it slower and I wanted to understand if there is a more efficient way to improve upon my existing implementation.

Instead of using a list to store your points (which has an \$O(n)\$ look-up time), you can use a set object which has an \$O(1)\$ look-up time. For more information regarding Pythonic time complexity, you can look here.

I have a couple comments on your style:

Instead of x and y, I would use column/col and row. Even though x and y are mathematical coordinate names, I still prefer to be more explicit.
Group declarations are OK in the right circumstances, like unpacking an iterator or defining groups of like variables. The way you do this doesn't quite fit into the 'right circumstances' category: you are assigning and then slicing.

To get a better implementation, there are a couple of options:

Split the statement into its components (assigning x, y and slicing the list)
Calling list.pop() to do both at the same time

These two options, in terms of efficiency, are probably identical. So, its up to you to choose which one you want to use. Here is their syntax:

# 2 statements.
col, row = stack[0]
stack = stack[1:]

# Using `pop`.
col, row = stack.pop(0)

In Python 3, use the * operator to unpack the list

This option is the least applicable (due to your comments below). However, for reference here is that syntax:

(col, row), *stack = stack

This assigns the first element of the stack to (col, row) and the rest to itself, effectively popping off the first element.

P.S. Congratulations. This is probably my shortest review on this site.


7-Day Agenda in Python This is a personal project of mine.  I made a simple seven day agenda program in Python that stores tasks as a string, saves it into a .dat file, and returns that task when you input a day of the week.  This works on a terminal.  I'm new to programming in general.
This is Python 3.8.2, 32 bit, and in a virtual environment.
# Pickle module allows you to save data after program closes
import pickle

week = {"Sunday": [], "Monday": [], "Tuesday": [],
        "Wednesday": [], "Thursday": [], "Friday": [], "Saturday": []}

def agenda():
    option = input(
        "What would you like to do?  (A - look at agenda, B - create task, C - clear list, or D - quit) ").upper()
    if option == "A":
        look_at_agenda()
    elif option == "B":
        create_task()
    elif option == "C":
        clear_list()
    elif option == "D":
        print("Have a nice day!")
    else:
        print("Invalid option.")
        agenda()

# global allows day variable in other functions to work even without declaring it

def valid_days():
    global day
    day = input("Which day? ").capitalize()
    week_list = [day_list for day_list in week.keys()]
    if day not in week_list:
        print("Invalid day.")
        valid_days()

# Loads whatever string from week.dat file
def look_at_agenda():
    see_all = input("See entire week? Y - yes, N - no. ").upper()
    if see_all == "Y":
        week = pickle.load(open("week.dat", "rb"))
        print(week)
    elif see_all == "N":
        valid_days()
        week = pickle.load(open("week.dat", "rb"))
        print(week[day])
    else:
        print("Invalid option.")
        look_at_agenda()
    agenda()

def create_task():
    valid_days()
    task = input("Describe your task. ")
    # Loads string from week.dat file; when program restarts, it allows you to create a task without deleting the saved ones
    week = pickle.load(open("week.dat", "rb"))
    week[day].append(task)
    # Saves string into week.dat file so it remembers when you open the program again
    pickle.dump(week, open("week.dat", "wb"))
    agenda()

def clear_list():
    clear_all = input("Clear all lists? Y - yes, N - no ").upper()
    if clear_all == "N":
        valid_days()
        week[day].clear()
        print(f"List cleared for {day}!")
        # saves the new list into a week.dat file
        pickle.dump(week, open("week.dat", "wb"))
    elif clear_all == "Y":
        [week[day].clear() for day in week]
        # saves the now empty list into a week.dat file
        pickle.dump(week, open("week.dat", "wb"))
    else:
        print("Invalid option.")
        clear_list()
    agenda()

agenda()

Welcome to the code review community. The code looks clean, and follows the PEP8 style guide. Here are a few suggestions/modifications you can make though:

Do not use global variables.

Instead of calling agenda() at the end, put it inside the if __name__ block.

Since you're working with python 3.8+, you may also add type hinting for function arguments.

For user input/validation, you can make use of a while loop:
 from typing import List

 def read_user_input_from_options(message: str, valid_inputs: List = None):
     """
     Show `message` in the console while asking for user input.

     If `valid_inputs` is not provided, returns the value from user as is.
     """
     if valid_inputs:
         valid_inputs = map(lambda x: x.upper(), valid_inputs)
     while True:
         user_input = input(message)
         if not valid_inputs:
             return user_input
         user_input = user_input.upper()
         if user_input in valid_inputs:
             return user_input
         print("Invalid option chosen. Try again.")

The above is an example suited for your case, where all the input option you need are capitalised alphabets. It can be extended to support other validators as per requirements.

Use multiline strings. Makes it more readable. It is more a personal preference than rules to live by.

You can map the option to a function call (see below):
 def quit_program():
     print("Have a nice day!")
     return

 def main():
     OPTION_TO_FUNCTION = {
         "A": look_at_agenda,
         "B": create_task,
         "C": clear_list,
         "D": quit_program,
     }
     option = read_user_input_from_options(
         """
         What would you like to do?
         A - look at agenda
         B - create task
         C - clear list
         D - quit
         """,
         valid_inputs=["a", "b", "C", "d"],
     )
     return OPTION_TO_FUNCTION[option]()

where main is the function called from inside your if __name__ == "__main__" block.

Make use of a class to keep the state of current execution. This would accompany my point about avoiding globals as well.


Calculating population standard deviation This is a script I have written to calculate the population standard deviation. I feel that this can be simplified and also be made more pythonic.

from math import sqrt

def mean(lst):
    """calculates mean"""
    sum = 0
    for i in range(len(lst)):
        sum += lst[i]
    return (sum / len(lst))

def stddev(lst):
    """calculates standard deviation"""
    sum = 0
    mn = mean(lst)
    for i in range(len(lst)):
        sum += pow((lst[i]-mn),2)
    return sqrt(sum/len(lst)-1)

numbers = [120,112,131,211,312,90]

print stddev(numbers)

The easiest way to make mean() more pythonic is to use the sum() built-in function.

def mean(lst):
    return sum(lst) / len(lst)

Concerning your loops on lists, you don't need to use range(). This is enough:

for e in lst:
   sum += e

Other comments:

You don't need parentheses around the return value (check out PEP 8 when you have a doubt about this).
Your docstrings are useless: it's obvious from the name that it calculates the mean. At least make them more informative ("returns the mean of lst").
Why do you use "-1" in the return for stddev? Is that a bug?
You are computing the standard deviation using the variance: call that "variance", not sum!
You should type pow(e-mn,2), not pow((e-mn),2). Using parentheses inside a function call could make the reader think he's reading a tuple (eg. pow((e,mn),2) is valid syntax)
You shouldn't use pow() anyway, ** is enough.

This would give:

def stddev(lst):
    """returns the standard deviation of lst"""
    variance = 0
    mn = mean(lst)
    for e in lst:
        variance += (e-mn)**2
    variance /= len(lst)

    return sqrt(variance)

It's still way too verbose! Since we're handling lists, why not using list comprehensions?

def stddev(lst):
    """returns the standard deviation of lst"""
    mn = mean(lst)
    variance = sum([(e-mn)**2 for e in lst]) / len(lst)
    return sqrt(variance)

This is not perfect. You could add tests using doctest. Obviously, you should not code those functions yourself, except in a small project. Consider using Numpy for a bigger project.


Reading SQL file using injection and utility reader I've come up with a way to externalize and read in my .sql files instead of having them in-code as strings.  Note that I'm keeping the .sql file in the same package as the DAO (NOT separated into a separate resources folder).

I'm curious what others think about this methodology.

The sqlStatement is inject on class load (which because this is managed by Spring, on application start).  It will call the static method SqlReader.getStatement(), passing in the file name.

Data Access

@Repository
public class TestDAO {

    @Value("#{sqlReader.getStatement('com/mycompany/selectStatement.sql')}")
    private String selectStatement;

The SqlReader is a Utility class located in another package.

public class SqlReader {

    public static String getStatement(final String file) {

        try {
            return FileUtils.readFileToString(new ClassPathResource(file).getFile(), StandardCharsets.UTF_8);
        } catch (IOException exception) {
            Logger.error("IOException reading file [" + file + "].", exception);
            throw new IllegalStateException("Error reading for file [" + file + "]", exception);
        }
    }
}

Updated: updated the error handling. also, made SqlReader a Spring component, which cleans up the @Value expression by removing the necessity to lookup the SqlReader by type (Spring can find it by it's bean name now).

The idea to extract SQL queries from the code into in a separate resource is good in general. Its main advantage is that when there is a non-substantial change at the database side, it might be enough to change the resource file, without recompiling the code.

However, it slightly reduces the readability: the query is in a separate file, you do not have it immediately visible near the instructions that process the ResultSet. That can become confusing if the query is passed to a PreparedStatement with a number of parameters, but everything finally depends on each case. This discussion can easily end up with arguing whether inline SQL is better than using an ORM :)

For the solution that you submitted, I can observe the following:

each *.sql file can contain only one query. If there are a lot of queries used by objects within a package, this can easily pollute the folder and you'll need to be careful when choosing a naming convention for the files, otherwise one will be easily lost among them. selectStatement.sql is just an example and real resources should have names like selectBookByAuthor.sql etc.
a single string with plain and directly usable SQL is replaced with two values which are not obvious to change and maintain if necessary: the class name (com.mycompany.SqlReader) and the file name (com/mycompany/selectStatement.sql), because both are packed in the same string expression. Would you rely on your IDE or make all changes manually for each occurrence when these two values are changing?
the UnsupportedOperationException is not the best choice for this case. Logically, the operation is supported, but something gone wrong here and the expected predefined statement could not be read. The application is thus in an invalid state: the .sql was not included in the build sequence or a typo in the file name... I think that IllegalStateException is much better here. It also should be moved inside the catch block and chained with the IOE (throw new IllegalStateException(ioexception);). You should probably consider adding to the catch block other exception types, if any, that can result from calling FileUtils.readFileToString.

Another Approach

I can suggest another approach that has more or less the same result of externalizing SQL queries, but can be achieved without the dedicated SqlReader class, only by using the features of Spring.

The fields representing SQL queries can be initialized from property file sources:

@Value("${sql.select.products}")
private String selectProductsQuery;

The properties file must contain the key sql.select.products.

The annotation @PropertySource is used to point to the properties file to look in:

@PropertySource("classpath:/queries.properties")
public class TestDAO { ... }

In queries.properties you just specify the queries to use:

sql.select.products=SELECT p.id, p.name ... FROM product p...

The advantages:

it's all managed by Spring, once the configuration is set up properly.
You have all the queries in the same file.


PHP Authentication API I created small simple PHP Authentication API. I have a couple of scripts that I use for session, authentication and registration. Since I'm not an experienced backend and PHP developer, I wanted someone more experienced to review my scripts and tell me what I did wrong and what I can improve.

I did not use any framework; this is plain PHP.

User registration:

<?php
require_once '../dbConnect.php';

$object = json_decode(file_get_contents("php://input"), true);

if (isset($object['email']) && isset($object['password']) && isset($object['firstName']) && isset($object['lastName'])) {
    $email = $object['email'];

    $validationQuery="select * from members where email='$email'";
    $result = $mysqli->query($validationQuery) or die($mysqli->error.__LINE__);
    $member = mysqli_fetch_assoc($result);

    if($member) {
        $message = array('message' => 'Member with provided email address already exist, please use other email.');
        http_response_code(406);
        echo json_encode($message);
    } else {
        session_start();
        $firstName = $object['firstName'];
        $lastName = $object['lastName'];
        $password = password_hash($object['password'], PASSWORD_DEFAULT);

        $registrationQuery = "INSERT INTO members
                (firstName, lastName, email, password)
                VALUES
                ('$firstName', '$lastName', '$email', '$password')";

        if ($mysqli->query($registrationQuery) === TRUE) {
            $message = array(
                'message' => 'Registration Successful, you can use your credentials to log in.',
                'memberId' => mysqli_insert_id($mysqli));
            $_SESSION["id"] = $message['memberId'];
            echo json_encode($message);
        }
    }

    $mysqli->close();
} else {
    http_response_code(400);
}
?>

Getting authenticated member from session:

<?php
require_once '../dbConnect.php';
session_start();

$object = json_decode(file_get_contents("php://input"), true);

if (isset($object['email']) && isset($object['password'])) {

    $email = $object['email'];
    $password = $object['password'];
    $query="select * from members where email='$email'";
    $result = $mysqli->query($query) or die($mysqli->error.__LINE__);
    $member = mysqli_fetch_assoc($result);

    if($member) {
        if (password_verify($object['password'], $member['password'])) {
            $message = array('message' => 'Authentication Successful!');
            $_SESSION["id"] = $member['id'];
            echo json_encode($message);
        } else {
            $message = array('message' => 'Wrong Credentials, Authentication failed!');
            session_destroy();
            http_response_code(400);
            echo json_encode($message);
        }
    } else {
        session_destroy();
        http_response_code(406);
    }

    $mysqli->close();
} else {
    session_destroy();
    http_response_code(400);
}
?>

Getting authenticated member from PHP session cookie

<?php
require_once '../dbConnect.php';
session_start();

if (isset($_SESSION["id"])) {
    $memberId = $_SESSION["id"];
    $query="select id, firstName, lastName, email, profileImage from members where id='$memberId'";

    $result = $mysqli->query($query) or die($mysqli->error.__LINE__);
    $member = mysqli_fetch_assoc($result);

    echo $json_response = json_encode($member);

    $mysqli->close();
} else {
    http_response_code(401);
}

?>

Simple logout script:

<?php

session_start();

if (isset($_SESSION["id"])) {
    $message = array('message' => 'Successful log out!');
    session_destroy();
    echo json_encode($message);
} else {
    echo 'You are not logged in!';
    http_response_code(403);
}

?>

Ehh, let's look at the biggest issue here: the SQL-Injection vulnerability.

$object = json_decode(file_get_contents("php://input"), true);

if (isset($object['email']) && isset($object['password']) && isset($object['firstName']) && isset($object['lastName'])) {
    $email = $object['email'];

    $validationQuery="select * from members where email='$email'";

All I have to do is provide a bad string in that JSON for email and now I can destroy your database easy.

Solution: prepared statements.


SalesTax problem (C# version) One year ago I published an F# solution of the same task and there is an old C# solution. But I think it's a simple task and require a simple solution.

What do you think?

using System;
using System.Collections.Generic;

namespace SalesTaxes
{
    class Program
    {
        static void Main(string[] args)
        {
            List<ShoppingCartItem> itemList = getItemsList();

            decimal salestaxes = 0.00m;
            decimal totalprice = 0.00m;

            foreach (ShoppingCartItem item in itemList)
            {
                salestaxes += item.Taxes * item.Quantity;
                totalprice += item.Item.Price * item.Quantity;
                Console.WriteLine(string.Format("{0} {1} : {2}", item.Quantity, item.Item.Name, (item.Item.Price + item.Taxes) * item.Quantity));
            }
            totalprice += salestaxes;
            Console.WriteLine("Sales Taxes : " + salestaxes);
            Console.WriteLine("Total : " + totalprice);
            Console.ReadLine();
        }

        private static List<ShoppingCartItem> getItemsList()
        {
            List<ShoppingCartItem> lstItems = new List<ShoppingCartItem>();
            //input 1

            lstItems.Add(new ShoppingCartItem { Item = new Product { Name = "Book", Price = 12.49m, Type = Product.ProductType.book, IsImport = false }, Quantity = 1 });
            lstItems.Add(new ShoppingCartItem { Item = new Product { Name = "music CD", Price = 14.99m, Type = Product.ProductType.other, IsImport = false }, Quantity = 1 });
            lstItems.Add(new ShoppingCartItem { Item = new Product { Name = "chocolate bar", Price = 0.85m, Type = Product.ProductType.food, IsImport = false }, Quantity = 1 });

            return lstItems;
        }
    }

    public class Product
    {
        public enum ProductType
        {
            food = 1,
            book = 2,
            medical = 3,
            other = 4
        };

        public string Name { get; set; }
        public decimal Price { get; set; }
        public ProductType Type { get; set; }
        public bool IsImport { get; set; }
        public bool IsExempt
        {
            get
            {
                return (int)Type < 4;
            }
        }
    }

    public class ShoppingCartItem
    {
        const decimal TaxRate = 0.1m;
        const decimal ImpTaxRate = 0.05m;
        public Product Item { get; set; }
        public int Quantity { get; set; }
        public decimal Taxes
        {
            get
            {
                return decimal.Ceiling(Item.Price * ((Item.IsExempt ? 0 : TaxRate) + (Item.IsImport ? ImpTaxRate : 0)) * 20) / 20;
            }
        }
    }
}

A couple of small suggestions.

Var:
You spend a lot of time redeclaring variable types when they are already well established by your code. For example:

decimal totalprice = 0.00m;
foreach (ShoppingCartItem item in itemList){...}
List<ShoppingCartItem> lstItems = new List<ShoppingCartItem>();

I know there can be some debate about whether var is preferred or not, but I can say I work in a shop that currently uses it heavily and it makes refactoring a dream. Just changing the Foreach to:

foreach(var item in itemList)

would make refactoring much easier later. This way it will figure out the type for you at compile time, meaning itemList can be an ienumerable of anything as would still be valid in that case.

Console.WriteLine:
There is already a string format overload for Console.Writeline. The following lines are equivalent:

Console.WriteLine(string.Format("{0} ", x));
Console.WriteLine("{0}",x);

Collection Construction:
You can use simpler syntax to construct your collection in getItemsList() making your entire signature for the function something closer to this:

    private static IEnumerable<ShoppingCartItem> getItemsList()
    {
        return new List<ShoppingCartItem>
        {
            new ShoppingCartItem
            {
                Item = new Product {Name = "Book", Price = 12.49m, Type = Product.ProductType.book, IsImport = false},
                Quantity = 1
            },
            new ShoppingCartItem
            {
                Item = new Product{Name = "music CD",Price = 14.99m,Type = Product.ProductType.other,IsImport = false},
                Quantity = 1
            },
            new ShoppingCartItem
            {
                Item = new Product{Name = "chocolate bar",Price = 0.85m,Type = Product.ProductType.food,IsImport = false},
                Quantity = 1
            }
        };
    }

Return Type List:
For what you use the return value for, in getItemsList, it doesn't need to be returned as a List. It could be an IList or IEnumerable without any ill effects. It's not a huge deal in your case as it's private, but around where I work we generally try to return collections under there interfaces. I know resharper will flag this as well.

Naming:
The enumeration ProductTypes values and the getItemsList method do not follow C# standards for naming. It really should be GetItemsList and Food/Book/Medical/Other.

IsExempt
Why is it:

return (int)Type < 4;

instead of:

return Type != ProductType.Other;

They mean the same thing, but one is a bit more obvious as to it's intentions.

Taxes Getter:
This is more of a soft suggestion, but this getter is a bit convoluted to read. I had to dig through and add some spacing to figure out what it was doing and even then I misplaced a paren the first time and got the wrong solution. I understand what you are doing with this type of code but it's honestly something that will be more of a headache than a help later as it is not immediately obvious what it is doing unless you dig in. Maybe simplifying out the ternary statements into getters for those values, or methods if you prefer, would be a better solution as it would vastly simplify the amount of parenthesis and make the statement a bit easier on the eyes. Example:

public decimal Taxes
    {
        get
        {
            return decimal.Ceiling( Item.Price *( CalculateTaxRate() + CalculateImportRate()) * 20) / 20;
        }
    }

    private decimal CalculateTaxRate()
    {
        return Item.IsExempt
            ? 0
            : TaxRate;
    }

    private decimal CalculateImportRate()
    {
        return Item.IsImport
            ? ImpTaxRate
            : 0;
    }

This seems much easier to read at a glance.

Writelines
Also worth noting that you swap between the string.format method of writing variables out to the string concatenation method in a couple of places:

Console.WriteLine(string.Format("{0} {1} : {2}", item.Quantity, item.Item.Name, (item.Item.Price + item.Taxes) * item.Quantity));
Console.WriteLine("Sales Taxes : " + salestaxes);

Best to stick with a style when you start. I prefer the string format approach, using the appropriate Console.WriteLine overload, as it hedges away from doing string + string. In this case it's really not more efficient, but it's a bad to get into the habit of string + string, as in non-trivial usages of concatenation it's inefficient. When I find yourself using string + string it's usually a tip off that there is a better way to be doing it (StringBuilder, String.Format, etc). Once again, no actual gain from this in your instance, just a good habit to build upon.

Overall:
The solution itself looks pretty good, though I'll admit I didn't run it through too much in the way of testing. I would just suggest those small style changes. Hope that helps.


Optimize vector rotation I have a trivial function that rotates 2d vectors, and a method in a class representing a polygon that rotates every point in the polygon around an origin. The code is fairly optimized as it is, but I was wondering if there is any faster way of doing it, since the function is called a HUGE amount of times and I need it to be as fast as it can possibly be.

Here is the code for the rotation function (in a file called geo.py):

def rotate_vector(v, angle, anchor):
    """Rotate a vector `v` by the given angle, relative to the anchor point."""
    x, y = v

    x = x - anchor[0]
    y = y - anchor[1]
    # Here is a compiler optimization; inplace operators are slower than
    # non-inplace operators like above. This function gets used a lot, so
    # performance is critical.

    cos_theta = math.cos(angle)
    sin_theta = math.sin(angle)

    nx = x*cos_theta - y*sin_theta
    ny = x*sin_theta + y*cos_theta

    nx = nx + anchor[0]
    ny = ny + anchor[1]
    return [nx, ny]

And here is the code for the polygon object:

import geo

class ConvexFrame(object):
    """A basic convex polygon object."""

    def __init__(self, *coordinates, origin=None):
        self._origin = origin
        # The coordinates in this object are stored as offset values, that is,
        # coordinates that represent a certain displacement from the given origin.
        # We will see later that if the origin is None, then it is set to the
        # centroid of all the points.

        self._offsets = []

        if not self._origin:
            # Calculate the centroid of the points if no origin given.
            self._origin = geo.centroid(*coordinates)
        orx, ory = self._origin
        append_to_offsets = self._offsets.append
        for vertex in coordinates:
            # Calculate the offset values for the given coordinates
            x, y = vertex
            offx = x - orx
            offy = y - ory
            append_to_offsets([offx, offy])

        offsets = self._offsets
        left = geo.to_the_left
        # geo.to_the_left takes three vectors (v0, v1 and v2) and tests if vector v2
        # lies to the left of the line between v0 and v1. The offset values are input
        # in counter-clockwise order, so all points v(i) should lie to the left of the
        # the line v(i-2)v(i-1).
        n = len(offsets)
        for i in range(n):
            v0 = offsets[i-1]
            v1 = offsets[i]
            v2 = offsets[(i+1)%n]
            if not left(v0, v1, v2):
                raise ValueError("""All vertices of the polygon must be convex.""")

    def rotate(self, angle, anchor=(0, 0)):
        # Avg runtime for 4 vertices: 7.2e-06s
        orx, ory = self._origin
        x, y = anchor
        if x or y:
            # Default values of x and y (0, 0) indicate
            # for the method to use the frame origin as
            # the anchor. Since we are rotating the offset
            # values and not actually the coordinates, we
            # have to adjust the anchor relative to the origin.
            x = x - orx
            y = y - ory
        _rot = geo.rotate_vector
        self._offsets = [_rot(v, angle, (x, y)) for v in self._offsets]

If I can get this below 3e-06s for 4 vertices that would be phenomenally helpful.

UPDATE 1:

Just found an optimization; in the list comprehension I say (x, y) every iteration, meaning I have to rebuild the tuple every single iteration. Removing that shaves the time down to between 7e-06 and 6.9e-06s for 4 vertices.

def rotate2(self, angle, anchor=(0, 0)):
    # Avg runtime for 4 vertices: 7.0e-06s
    # Best time of 50 tests: 6.92e-06s
    orx, ory = self._origin
    x, y = anchor
    if x or y:
        # Default values of x and y (0, 0) indicate
        # for the method to use the frame origin as
        # the anchor.
        x = x - orx
        y = y - ory
        anchor = x, y
    _rot = geo.rotate_vector
    self._offsets = [_rot(v, angle, anchor) for v in self._offsets]

You will likely be rotating many vectors by the same angle.  Therefore, it would be wasteful to compute \$\cos \theta\$ and \$\sin \theta\$ repeatedly.

The typical way to think of linear transformations is as matrix multiplication:

$$
\left[ \begin{array}{c} x' \\ y' \end{array} \right] =
\left[ \begin{array}{rr}
  \cos \theta  & -\sin \theta \\
  \sin \theta & \cos \theta
\end{array}\ \right]
\left[ \begin{array}{c} x \\ y \end{array} \right]
$$

So, define a make_rotation_transformation(angle, origin) function that returns a closure that holds the transformation matrix and origin vector.

from math import cos, sin

def make_rotation_transformation(angle, origin=(0, 0)):
    cos_theta, sin_theta = cos(angle), sin(angle)
    x0, y0 = origin
    def xform(point):
        x, y = point[0] - x0, point[1] - y0
        return (x * cos_theta - y * sin_theta + x0,
                x * sin_theta + y * cos_theta + y0)
    return xform

def rotate(self, angle, anchor=(0, 0)):
    xform = make_rotation_transformation(angle, anchor)
    self._offsets = [xform(v) for v in self._offsets]


Using a class to create a custom wrapper for including files I am using PHP in order to create a custom wrapper for including files, for this I have written this class:

<?php
class Refrences
{
    #region properties
    private $_basePath;
    #endregion

    #region public members
    public function __construct() { $this->_basePath = $this->cleanDirName(dirname(__FILE__)); }

    public function includeFile($f) { $this->ior(1, $f); }

    public function requireFile($f) { $this->ior(2, $f); }

    public function includeFileOnce($f) { $this->ior(3, $f); }

    public function requireFileOnce($f) { $this->ior(4, $f); }

    public function includeArray(array $files)
    {
        foreach ($files as $f) { $this->ior(1, $f); }
    }

    public function requireArray(array $files)
    {
        foreach ($files as $f) { $this->ior(2, $f); }
    }

    public function includeArrayOnce(array $files)
    {
        foreach ($files as $f) { $this->ior(3, $f); }
    }

    public function requireArrayOnce(array $files)
    {
        foreach ($files as $f) { $this->ior(4, $f); }
    }
    #endregion

    #region private members
    private function cleanDirName($f) { return rtrim(str_replace("\\", "/", dirname($f)), "/") . "/"; }

    private function fileExists($f) { return (!file_exists($f)) ? false : true; }

    private function isFile($f) { return (is_dir($f) || !is_file($f)) ? false: true; }

    private function ior($switch, $file)
    {
        switch ($switch)
        {
            case 1:
                if ($this->checkIsFile($file)) include ($file);
                else die("Cannot load file.");
                break;
            case 2:
                if ($this->checkIsFile($file)) require ($file);
                else die("Cannot load file.");
                break;
            case 3:
                if ($this->checkIsFile($file)) include_once ($file);
                else die("Cannot load file.");
                break;
            case 4:
                if ($this->checkIsFile($file)) require_once ($file);
                else die("Cannot load file.");
                break;
        }
    }

    private function checkIsFile($f) { return ($this->fileExists($f) && $this->isFile($f)) ? true : false; }
    #endregion
}

And all functionality etc. works, but is this a good thing to do? Does it allow me greater control over the files that are included? Is there a better way to do this?

don't shorten method or variable names. It's really hard to see what ior means. I'm guessing includeOrRequire?
You should definitely check for directory traversal. You have the base name and the file name, so you can check if the file is actually in the base dir. You could also additionally do some sanitation on base name (eg replace ..).
Your cleanDirName replaces \ with /. Why not replace / and \ with  DIRECTORY_SEPARATOR, so it works independent of the OS?
You shouldn't die in functions, as it makes it impossible for the calling code to recover (or even to display a custom error page). It also makes the distinction between include and require meaningless, as the only difference is that one holds execution while the other doesn't
return cond ? true : false can be written as return cond.
Don't put the method content in the same line as the method signature, it makes your code less readable. Same for foreach, etc.
Having the ior function doesn't actually provide any benefits. It doesn't remove any duplication, and it introduces extra complexity (the cases are also rather random, just looking at $this->ior(2, $f);, I would have no idea what it does).
fileExists is unnecessary, its just a rename of file_exists. It's also written confusingly, because of the unnecessary negation.
I don't see why you need to check if is_dir and if is_file. Either it's one or the other. Just is_file would be enough. And then your isFile method is also unnecessary, as it's just a rename of is_file.
You don't use basepath, so why have it in the constructor?

Taking all this (without the validation, you would need to add that), you would get:

<?php
class Refrences
{
    public function __construct() { }

    public function includeFile($file) {
        if ($this->isExistingFile($file)) {
            include($file);
        } else {
            // throw exception or return false.
        }
    }
    // [...]

    public function includeArray(array $files)
    {
        foreach($files as $file) {
            includeFile($file);
        }
    }
    // [...]

    private function cleanDirName($file) {
        return rtrim(str_replace(["\\", "/"], DIRECTORY_SEPARATOR, dirname($file)), "/") . "/";
    }

    private function isExistingFile($file) {
        return file_exists($file) && is_file($file);
    }
}

Without the validation for directory traversal, this is basically just a renaming of include, etc to includeFile. I'm not sure it's worth it. You do save the file exists and is file checks, but you have to check the return of the include call anyways.

It does however give you one central place where all includes take place, so if you do want to do additional stuff when including (like checking for directory traversal or normalizing paths for different OSes), it wouldn't be bad to have code like this.


Using enums in a card/deck class I recognize that this is a common question area with lots of answers:
Here and here for example.
Specifically I'm trying to better understand how to use Enums in Python. Suits in a deck of cards seems appropriate, but implementing them as a class seems extra, and I'm curious if there's a more pythonic way to do it?
class Suits(Enum):
    Club = 1
    Heart = 2
    Diamond = 3
    Spade = 4

class Card:
    def __init__(self, value, suit):
        self.value = value
        self.suit = suit

class Deck:
    def __init__(self):
        self.cards = self.initDeck()

    def initDeck():
        cards = {}
        for suit in Suits:
            for i in range(14):
                cards.add(Card(i, suit))
        return cards

The whole point of Enums is to prevent you from adding a bad value though, right? (Like, making a new card as Card(0, 'joker')) so maybe this isn't the right application for it? Currently there would be no error if I passed Card(4, 'dimond') and that's exactly what enums are supposed to prevent, right?
Hoping for a bit more insight/education. Thanks!

Your implementation uses Enum for the suits, but not for the values.  Thus it is harder to validate the card values, and would as you noted, allow a joker card to be created, even though you didn't want that ability.

Card Value Enum:

A card value enum provides the ability to give the cards a value and a descriptive name, while limiting possible values to valid ranges:

class CardValue(Enum):
    Ace = 1
    Deuce = 2
    Three = 3
    Four = 4
    Five = 5
    Six = 6
    Seven = 7
    Eight = 8
    Nine = 9
    Ten = 10
    Jack = 11
    Queen = 12
    King = 13

Immutable Types

For something like a card, an immutable type can provide some advantages.  One big one, is that the same value and suit will always key the same in a set or dict. Inheriting from tuple will achieve imuutability.  Note that the class is setup in __new__ not __init__ since it is immutable.

class Card(tuple):

    def __new__(cls, value, suit):
        assert isinstance(value, CardValue)
        assert isinstance(suit, CardSuit)
        return tuple.__new__(cls, (value, suit))

property decorator

The property decorator makes it easy to access the value and suit.

    @property
    def value(self):
        return self[0]

    @property
    def suit(self):
        return self[1]

__str__ method

Since we are using Enum's for all of the values, a fully descriptive __str__ method is quite straight forward:

    def __str__(self):
        return "{} of {}s".format(self.value.name, self.suit.name)

So something like:

print(Card(CardValue.Ace, CardSuit.Club))

gives:

Ace of Clubs

Make sure we stay immutable

Suggest adding some boilerplate to help avoid abusing the class instances.

    def __setattr__(self, *ignored):
        raise NotImplementedError

    def __delattr__(self, *ignored):
        raise NotImplementedError

Deck class

So the Deck class can be made much simpler with the new Value class and a set comprehension like:

class Deck:
    def __init__(self):
        self.cards = {
            Card(value, suit) for value in CardValue for suit in CardSuit
        }

Whole Listing:

from enum import Enum

class CardValue(Enum):
    Ace = 1
    Deuce = 2
    Three = 3
    Four = 4
    Five = 5
    Six = 6
    Seven = 7
    Eight = 8
    Nine = 9
    Ten = 10
    Jack = 11
    Queen = 12
    King = 13

class CardSuit(Enum):
    Club = 1
    Heart = 2
    Diamond = 3
    Spade = 4

class Card(tuple):

    def __new__(cls, value, suit):
        assert isinstance(value, CardValue)
        assert isinstance(suit, CardSuit)
        return tuple.__new__(cls, (value, suit))

    @property
    def value(self):
        return self[0]

    @property
    def suit(self):
        return self[1]

    def __str__(self):
        return "{} of {}s".format(self.value.name, self.suit.name)

    def __setattr__(self, *ignored):
        raise NotImplementedError

    def __delattr__(self, *ignored):
        raise NotImplementedError

class Deck:
    def __init__(self):
        self.cards = {
            Card(value, suit) for value in CardValue for suit in CardSuit
        }


UITableView allowing some kinds of requests to be deleted with swipe gestures I have a tableview which shows a list of objects called Requests. It has 3 segments. Namely Accepted, Received and Sent. And the objects for each segment are in 3 arrays.

I want to enable deleting rows (in turn the objects) only for Accepted and Sent segments.

This is my current implementation.

func tableView(_ tableView: UITableView, trailingSwipeActionsConfigurationForRowAt indexPath: IndexPath) -> UISwipeActionsConfiguration? {

    var deleteAction: UIContextualAction?

    switch currentShowingStatus {
    case .accepted:
        deleteAction = UIContextualAction(style: .destructive, title: "Delete") { action, view, completionHandler in
            let request = self.acceptedRequests[indexPath.row]
            completionHandler(true)
        }
    case .sent:
        deleteAction = UIContextualAction(style: .destructive, title: "Delete") { action, view, completionHandler in
            let request = self.sentRequests[indexPath.row]
            completionHandler(true)
        }
    default:
        break
    }

    if let deleteAction = deleteAction {
        let configuration = UISwipeActionsConfiguration(actions: [deleteAction])
        configuration.performsFirstActionWithFullSwipe = false
        return configuration
    } else {
        return UISwipeActionsConfiguration(actions: [])
    }
}

As you can see there is quite a bit of duplicating code. And I can't extract away the UIContextualAction declaration part because of it's completion handler. Selecting the object to be deleted happens in there.

I also can't define UISwipeActionsConfiguration as a local variable and reduce its duplicating due to that class must be initialized with UIContextualAction instances.

Note: You have to pass an empty actions array to UISwipeActionsConfiguration in order to not show the swipe actions in cell. Returning nil doesn't do it.

So all this has produced an ugly piece of code. I'm wondering if there's a better way to refactor this.

Alternative Approach

Instead of checking for segment first and then adding multiple delete actions, I put the checking for segment part inside one completion handler like this.

func tableView(_ tableView: UITableView, trailingSwipeActionsConfigurationForRowAt indexPath: IndexPath) -> UISwipeActionsConfiguration? {
    var actions = [UIContextualAction]()

    let deleteAction = UIContextualAction(style: .destructive, title: "Delete") { action, view, completionHandler in
        switch self.currentShowingStatus {
        case .accepted:
            let request = self.acceptedRequests[indexPath.row]
            self.deleteClientRequest(request)
        case .sent:
            let request = self.sentRequests[indexPath.row]
            self.deleteClientRequest(request)
        default:
            break
        }
        completionHandler(true)
    }

    switch currentShowingStatus {
    case .accepted, .sent:
        actions.append(deleteAction)
    case .received:
        actions.removeAll()
    }

    let configuration = UISwipeActionsConfiguration(actions: actions)
    configuration.performsFirstActionWithFullSwipe = false
    return configuration
}

The downside is I still have to check for segment again in order to show the swipe action for just the Accepted and Sent segments.

In your first implementation you can make deleteAction a  constant if you initialize it in all cases of the switch-statement:

    let deleteAction: UIContextualAction?
    switch currentShowingStatus {
    case .accepted:
        deleteAction = UIContextualAction(...)
    case .sent:
        deleteAction = UIContextualAction(...)
    case .received:
        deleteAction = nil
    }

That makes it clear that the variable is initialized exactly once before used. I would also replace default: by the explicit case received:. That makes it obvious to reader in which case no swipe action is configured (without looking up the enum definition) and forces you to update the code if more cases are added.

The double definition of UISwipeActionsConfiguration could be avoided by initializing the array of actions instead:

    let actions: [UIContextualAction]
    switch currentShowingStatus {
    case .accepted:
        actions = [UIContextualAction(...)]
    case .sent:
        actions = [UIContextualAction(...)]
    default:
        actions = []
    }

    let configuration = UISwipeActionsConfiguration(actions: actions)
    configuration.performsFirstActionWithFullSwipe = false
    return configuration

It remains the code duplication for UIContextualAction though.

In your alternative approach you append or remove elements from the actions array. But that array is initially empty. Similarly as above, you can simplify it to

    let deleteAction = ...

    let actions: [UIContextualAction]
    switch currentShowingStatus {
    case .accepted, .sent:
        actions = [deleteAction]
    case .received:
        actions = []
    }

But – as you noticed – you have to evaluate self.currentShowingStatus inside the closure again. I would avoid that for an additional reason: You are relying on the fact that the status has not changed when the closure is executed.

The code duplication for UIContextualAction can be avoided if you initialize the request  first:

   let request: Request?
   switch currentShowingStatus {
   case .accepted:
       request = self.acceptedRequests[indexPath.row]
   case .sent:
       request = self.sentRequests[indexPath.row]
   case .received:
       request = nil
   }

   if let request = request {
       let deleteAction = UIContextualAction(style: .destructive, title: "Delete") {
           _, _, completionHandler in
           self.deleteClientRequest(request)
           completionHandler(true)
       }
       let configuration = UISwipeActionsConfiguration(actions: [deleteAction])
       configuration.performsFirstActionWithFullSwipe = false
       return configuration
   } else {
       return UISwipeActionsConfiguration(actions: [])
   }

Note also that unused closure parameters can be replaced by the wildcard parameter _.

I would probably leave it like that, but of course you can combine the techniques:

    let request: Request?
    switch currentShowingStatus {
    case .accepted:
        request = self.acceptedRequests[indexPath.row]
    case .sent:
        request = self.sentRequests[indexPath.row]
    case .received:
        request = nil
    }

    let actions: [UIContextualAction]
    if let request = request {
        actions = [UIContextualAction(style: .destructive, title: "Delete") {
            _, _, completionHandler in
            self.deleteClientRequest(request)
            completionHandler(true)
            }]
    } else {
        actions = []
    }

    let configuration = UISwipeActionsConfiguration(actions: actions)
    configuration.performsFirstActionWithFullSwipe = false
    return configuration


jQuery Ajax response This is the way I handle an Ajax response. I am sure it can be improved.

success: function (json) {
                $.each(json, function(index,item){
                    if (item.field == "article_id") {
                        $("#article_id").val(item.value);
                    } else if (item.field == "naslov") {
                        $("#naslov").val(stripslashes(item.value));
                    }else if (item.field == "slug_naslov") {
                        $("#slug_naslov").val(item.value);
                    }else if (item.field == "datum") {
                        $("#datum").val(item.value);
                    }else if (item.field == "url") {
                        $("#url").val(item.value);
                    }else if (item.field == "tekst") {
                        $("#tekst").val(stripslashes(item.value));
                    }else if  (item.field == "tag_title") { //handle tags - tokeninput
                        var tagoviArray = item.value;
                        var tagoviInput = $("#txtTags");
                            $(tagoviInput).tokenInput("clear");
                            $.each(tagoviArray, function (index, value) {
                                var arr = value.split(',');
                                    $.each(arr, function (i, v) {
                                        if (!(v=="")) {
                                            $(tagoviInput).tokenInput("add", {id: "", name: v}); tag
                                        }
                                    });
                            });

                    }else if (item.field == "love") {
                        $("#love").val(item.value);
                    }
                });
                return false;
            }

Maybe something like this?

success: function (json) {
  var decorators = {},
      handlers = {};

  // decorators prepare content for insertion
  decorators.naslov = stripslashes;
  decorators.tekst  = stripslashes;

  // specialized handlers
  handlers.tag_title = function (value) {
    // do the tokenization-stuff here
  };

  // Go through the response
  $.each(json, function (index, item) {
    var value = item.value;
    // if the field has its own handler, use that
    if( typeof handlers[item.field] === "function" ) {
      handlers[item.field](value);
    } else {
      // Otherwise, send the value through the decorator
      // (if there is one for the given field), and
      // insert the value in the corresponding input
      if( typeof decorators[item.field] === 'function' ) {
        value = decorators[item.field](value);
      }
      $("#" + item.field).val(value);
    }
  });
}


Hierarchy with `list` type I want to establish a hierarchy, where at each level of the hierarchy, each type can contain a list<its own type>.  So Node (root of object class hierarchy) contains a list<Node*>, SpecialNode : Node contains a list of SpecialNode.  SpecialNode should not "accidentally" put new SpecialNode children into the list<Node*> in the base class, so the list<> are private.

I personally hate the concept of namehiding, because it is extremely prone to mistakes, uninitialized values, etc.  So here I have made it impossible (via private) for SpecialNode to "accidentally" insert into the list<Node*> instead of its own list<SpecialNode*>.

struct Node
{
private:
  list<Node*> children ;

protected:
  int v ;

public:
  Node( int iv ) : v(iv) { }

  virtual void print( const char* msg )
  {
    printf( "REGULAR CHILD %d in %s\n", v, msg ) ;
    for( Node *sn : Node::children )
      sn->print( "regular" ) ;

  }
} ;

struct SpecialNode : public Node
{
private:
  list<SpecialNode*> children ;

public:
  SpecialNode( int iv ) : Node(iv) { }
  void add( SpecialNode* child ) {
    children.push_back( child ) ;
  }

  void print( const char* msg )
  {
    printf( "SPECIAL CHILD %d in %s\n", v, msg ) ;
    for( SpecialNode *sn : children )
      sn->print( "special" ) ;

    //for( Node *sn : Node::children )
    //  sn->print( "regular" ) ;
  }

} ;

int main(int argc, const char * argv[])
{
  SpecialNode *sn = new SpecialNode( 1 ) ;

  // SpecialNodes must contain SpecialNode children.
  sn->add( new SpecialNode( 2 ) ) ;
  //sn->children.push_back( new Node( 3 ) ) ; // fails b/c Node is too basic to fit in SpecialNode container.

  sn->print( "root" ) ;

}

Is this a good way to do this, or is there a better way?

You might prefer to not use polymorphism at all if you want to make sure that your types remain unique. Consider something more like:

class Node
{
private:
    list<Node*> children ;
    int v ;

public:
    Node( int iv ) : v(iv) { }

    void add( Node* child )
    {
        children.push_back( child );
    }

    void print( const char* msg )
    {
        printf( "REGULAR CHILD %d in %s\n", v, msg );
        for( Node *sn : Node::children )
        {
            sn->print( "regular" );
        }
    }
};

class SpecialNode // no inheritance
{
private:
    list<SpecialNode*> children;
    int v;

public:
    SpecialNode( int iv ) : v(iv) {}
    void add( SpecialNode* child )
    {
        children.push_back( child ) ;
    }

    void print( const char* msg )
    {
        printf( "SPECIAL CHILD %d in %s\n", v, msg ) ;
        for( SpecialNode *sn : children )
            sn->print( "special" ) ;
    }
};

class Composite // no inheritance
{
private:
    Node nodes;
    SpecialNode specialNodes;

public:
    Composite( int iv ) : nodes(iv), specialNodes(iv)
    void addSpecial( SpecialNode* child )
    {
        specialNodes.add( child ) ;
    }
    void add( Node * child)
    {
        nodes.add(child);
    }

    void print( const char* msg )
    {
        nodes.print(msg);
        specialNodes.print(msg);
    }
};


Collecting a list of documents from a factory for an AngularJS controller With this controller I'm injecting myFactory and using Array.push() to add the array response.documents to the this.documents array. I can't help feeling there's a better way without having to use _this.

function myController($route, myFactory) {
  'ngInject';

  this.$onInit = function() {
    const _this = this;
    const id = $route.current.params.someId;

    this.documents = [];

    myFactory.getData(id).then((response) => {

      console.log(response); // { "something": { "Id":1234 }, "documents":[{ "DocumentId":"2345", "DocumentName":"doc.docx" }] }
      console.log(response.documents); // [{ "DocumentId":"2345", "DocumentName":"doc.docx" }]

      response.documents.forEach((object) => {
        _this.documents.push(object);
      });
    });
  };
}

In addition to what Blindman said, I would recommend avoiding using this in nested scopes , which is always confusing and error prone, by assigning documents to a local variable. i.e.

this.$onInit = function() {
  const id = $route.current.params.someId;
  let documents =this.documents = [];
  ...
  response.documents.forEach((object) => {
    documents.push(object);


Fixed-sized memory pool in C I'm learning C and I am currently implementing a memory pool.

I'm writing it in steps, first I implemented a fixed-sized memory pool then I will try to implement a memory pool with a known size of elements but an unknown number of elements and ultimately try to write a generic one that is just a block of memory that can allocate any size.

The pool is actually a block of memory that is initialized with values that are pointers to the next memory like so:

 ______________
|0x8 |0x10|NULL|
|____|____|____|
 ^    ^    ^
 0x0  0x8  0x10

I want to know if I'm heading in the right direction.
There is the code

allocator.h:

#include <stdlib.h>

typedef struct FixedPoolAllocator FixedPoolAllocator;

FixedPoolAllocator* FixedPoolAllocator_new(size_t nmemb, size_t size);

void* FixedPoolAllocator_alloc(FixedPoolAllocator *allocator);

void FixedPoolAllocator_free(FixedPoolAllocator *allocator, void *ptr);

void FixedPoolAllocator_destroy(FixedPoolAllocator *allocator);

allocator.c:

#include "allocator.h"

#define MAX(a, b) (((a) > (b)) ? (a) : (b))

typedef struct FixedPoolAllocator {

    // The currently free space in which a value can be stored
    size_t *currentFree;
    size_t numOfMembers;
    size_t memberSize;
    // The actual memory block, It is of type char* so that pointer arithmetic will be easier
    char *mem;

} FixedPoolAllocator;

FixedPoolAllocator* FixedPoolAllocator_new(size_t nmemb, size_t size)
{
    if (0 == size || 0 == nmemb) return NULL;

    FixedPoolAllocator *ret = malloc(sizeof *ret);
    if (NULL == ret) return NULL;

    // Make sure that the size of each member is atleast size_t so that the members can store a memory address
    size_t member_size = MAX(sizeof(size_t), size);

    ret->mem = malloc(nmemb * member_size);
    if (NULL == ret->mem) return NULL;
    // The initial free member
    ret->currentFree = (size_t*)ret->mem;

    ret->numOfMembers = nmemb;
    ret->memberSize = size;

    size_t *temp = ret->currentFree;

    // Assign each member with the address of the next member
    size_t i;
    for (i = 0; i < nmemb - 1; i++) {
        *temp = (size_t) (ret->mem + (i+1) * member_size);
        temp = (size_t*) ((char*)temp + member_size);
    }

    // The last member points to NULL
    *temp = (size_t)NULL;

    return ret;
}

void* FixedPoolAllocator_alloc(FixedPoolAllocator *allocator)
{
    if (NULL == allocator || NULL == allocator->currentFree) return NULL;

    // Return the currently free member and update currentFree to the next member in the list
    void *ret = allocator->currentFree;
    allocator->currentFree = (size_t*)*allocator->currentFree;

    return ret;
}

void FixedPoolAllocator_free(FixedPoolAllocator *allocator, void *ptr)
{
    size_t ptr_as_value = (size_t)ptr;

    // Assign the pointed value with the current free member
    *(size_t*)ptr = (size_t)allocator->currentFree;
    // Update the currenty free member to be the one that was just freed
    allocator->currentFree = ptr;
}

void FixedPoolAllocator_destroy(FixedPoolAllocator *allocator)
{
    free(allocator->mem);

    free(allocator);
}

Only a small review

Alignment not guaranteed

temp = (size_t*) ((char*)temp + member_size); attempts to convert a char* to size_t *, yet (char*)temp + member_size is not known to be properly aligned for a size_t*.  Insufficient alignment is undefined behavior.

Detect overflow

As the true product of nmemb * member_size may exceed SIZE_MAX:

if (SIZE_MAX/nmemb > member_size) {
  Handle_Overflow();  // clean-up, return NULL;
}
... =  malloc(nmemb * member_size);

Strange uses of size_t as pointer

// The last member points to NULL *temp = (size_t)NULL; implies something wrong.  If codes needs a pointer, use a pointer, not size_t.  If code needs a size, doesn't use a pointer.  Note that size_t and pointers do not necessarily covert well back and forth.  They might not be the same memory width either.

This use of size_t as a place to encode a pointer is simply broken.  Code needs a new approach.