Bowling Kata in Java I am sharing my Bowling Game Score Kata made in Java, it is available for whoever wants to code review it.
I appreciate any comments.

class BowlingGameScore {

    private int[] pinsKnockedDown = new int[21];
    private int currentRoll = 0;

    void roll(int pinsKnockedDown) {
        this.pinsKnockedDown[currentRoll++] = pinsKnockedDown;
    }

    int score() {
        int totalScore = 0;
        int rollNumber = 0;

        for (int frameNumber = 0; frameNumber < 10; frameNumber++) {
            int frameScore;

            if (isSpare(rollNumber)) {
                frameScore = 10 + getSpareBonus(rollNumber);
                rollNumber += 2;

            } else if (isStrike(pinsKnockedDown[rollNumber])) {
                frameScore = 10 + getStrikeBonus(rollNumber);
                rollNumber++;

            } else {
                frameScore = pinsKnockedDown[rollNumber] + pinsKnockedDown[rollNumber + 1];
                rollNumber += 2;
            }

            totalScore += frameScore;
        }

        return totalScore;
    }

    private int getStrikeBonus(int roll) {
        return pinsKnockedDown[roll + 1] + pinsKnockedDown[roll + 2];
    }

    private boolean isStrike(int roll) {
        return roll == 10;
    }

    private int getSpareBonus(int roll) {
        return pinsKnockedDown[roll + 2];
    }

    private boolean isSpare(int roll) {
        return pinsKnockedDown[roll] + pinsKnockedDown[roll + 1] == 10;
    }
}

And its tests:

class BowlingGameScoreTest {
    private BowlingGameScore game;

    @BeforeEach
    void setUp() {
        game = new BowlingGameScore();
    }

    @Test
    void
    game_no_pins_get_hit() {
        rollMany(0, 20);

        Assertions.assertEquals(0, game.score());
    }

    @Test
    void
    game_just_1_pin_get_hit() {
        rollMany(1, 20);

        Assertions.assertEquals(20, game.score());
    }

    @Test
    void
    game_with_1_spare() {
        rollSpare();
        game.roll(3);
        rollMany(0, 17);

        Assertions.assertEquals(16, game.score());
    }

    @Test
    void
    game_with_1_strike() {
        game.roll(10);
        game.roll(3);
        game.roll(4);
        rollMany(0, 16);

        Assertions.assertEquals(24, game.score());
    }

    @Test
    void
    game_of_strikes() {
        rollMany(10, 12);

        Assertions.assertEquals(300, game.score());
    }

    @Test
    void
    knockdown_nine_and_miss_ten_times() {

        int firstRollPinsKnockedDown = 9;
        int secondRollPinsKnockedDown = 0;
        int repeatTimes = 10;
        rollPair(repeatTimes, firstRollPinsKnockedDown, secondRollPinsKnockedDown);

        Assertions.assertEquals(90, game.score());
    }

    @Test
    void
    knockdown_five_and_spare_ten_times_and_fice() {
        rollPair(10, 5, 5);
        game.roll(5);

        Assertions.assertEquals(150, game.score());
    }

    private void rollPair(int repeatTimes, int firstRollPinsKnockedDown, int secondRollPinsKnockedDown) {
        for (int i = 0; i < repeatTimes; i++) {
            game.roll(firstRollPinsKnockedDown);
            game.roll(secondRollPinsKnockedDown);
        }
    }

    private void rollSpare() {
        game.roll(5);
        game.roll(5);
    }

    private void rollMany(int pins, int times) {
        for (int i = 0; i < times; i++) {
            game.roll(pins);
        }
    }
}

As far as BowlingGameScore goes, LGTM. I have no remarks. (And usually I have lots.)

In the tests, after each void (e.g. before game_no_pins_get_hit),
it's a bit odd to have a newline.

Also, a static import to turn Assertions.assertEquals into
a concise assertEquals would be convenient.


Controling Document and Window events via array I have recently taken on a task to rewrite some very old javascript / jquery and many of the issues are dozens of JS files with onready, onload, onscroll events everywhere and in no logical order! It truly looks like dozens of different developers over many years just sticking whatever code in whatever file.

Moving forward in my re-writes I will get away from this, but I have so many different JavaScript files that for right now the task of cleaning them up and unifying a common functions, $.fn's , prototypes and plugins structure will take me months to organize.

So, knowing I have a long road ahead of me here is what I implemented so incremental deployments would still be possible and to help me organize the code into logical groups for the eventual mass cleanup ahead. The code below is very stable and surprisingly fast, but am I headed in a strange direction?

Define an object I can organize the events into.

myObj = {
    doc: {
        ready:[],
        custom: []
    },
    win: {
        load: [],
        scroll: [],
        resize: [],
        resizestop: [],
    }
}

Push all the code into the logical array that organizes the events.

//for dom ready events
myObj.doc.ready.push(function(){
    alert("document ready")
})

//for window load events
myObj.win.load.push(function(){
    alert("window loaded")
})

//for custom events
myObj.doc.custom.push({
    name:"myCustomName",
    func: function(){
        alert("custom event")
    }
})

Loop through the arrays of the object and execute the stored function.

$(document).on({
    ready:function(){for(var i = 0; i < myObj.doc.ready.length; i++){ myObj.doc.ready[i]() }}
});

$(window).on({
    load:function(){for(var i = 0; i < myObj.win.load.length; i++){ myObj.win.load[i]() }},
    scroll:function(){for(var i = 0; i < myObj.win.scroll.length; i++){ myObj.win.scroll[i]() }},
    resize:function(){for(var i = 0; i < myObj.win.resize.length; i++){ myObj.win.resize[i]() }},
    resizestop:function(){for(var i = 0; i < myObj.win.resizestop.length; i++){ myObj.win.resizestop[i]() }}
})

//bind custom document events
for(var i = 0; i < myObj.doc.custom.length; i++){
    var c = myObj.doc.custom[i];
    $(document).on(c.name, c.func);
}

The code below is very stable and surprisingly fast, but am I headed in a strange direction?

I would say, it's a good first step to bring the code base into some kind of controlled state. It's also a good way of understanding the application and going through all the source files.

  It truly looks like dozens of different developers over many years just sticking whatever code in whatever file.

Actually most of old applications out there looks like this, some CMS driven backend, where people are trying to fix the DOM after its rendered.

However, I would make a HOF to generate the functions:

function createStackCaller(array) {
  return function() {
    for (var i = 0; i < array.length; i++) {
      if (typeof array[i] === 'function')
        array[i]();
    }
  }
}

$(window).on({
    load: createStackCaller(myObj.win.load),
    scroll: createStackCaller(myObj.win.scroll),
    resize: createStackCaller(myObj.win.resize),
    resizestop: createStackCaller(myObj.win.resizestop),
});


Optimize parsing of number for currency conversion I have written this working block, but is it optimized? I am getting currency in this format: 1.234,25.

public static String convertCurrency(String value, String exchangeCurrencyId)
{
    java.text.NumberFormat format = null;
    value = value.replaceAll("\\.", ",");
    char[] chars = value.toCharArray();
    chars[value.lastIndexOf(',')] = '.';
    value = new String(chars);
    value = value.replaceAll(",", "");
    Double price = Double.valueOf(value);
    price = price * (50); //exchange rate

    return price.toString();
}

This code has various issues. Here is what I understand your code does:

  The code takes a formatted number which consists of digits, separators (commas or periods). The last separator is used as the decimal separator, all other separators are deleted.

  Then it multiplies the number with some exchange rate, and returns that as a string.

The issues with your code are:

format is not used.
exchangeCurrencyID is not used.
The exchange rate is hard-coded
You use regexes for simple transliterations
Your code obfuscates the intent.
Your method does multiple unrelated things:

Sanitizing and parsing the number string
Applying exchange rate conversion
These separate responsibilities should be performed by two separate methods.

If we wish to avoid using regexes for a task that does not need them, we could write something like this:

StringBuilder sanitized = new StringBuilder();
boolean decimalSeparatorFound = false;
char[] chars = value.toCharArray();
for (int i = chars.length-1; i >= 0; i--) {
    if (chars[i] == ',' || chars[i] == '.') {
        if (decimalSeparatorFound) continue;  // skip this char
        decimalSeparatorFound = true;
        sanitized.append('.');
    } else {
        sanitized.append(chars[i]);
    }
}
double price = Double.valueOf(sanitized.reverse().toString());

...


Testing boto connection I am writing a unit test to be sure the code can connect to aws via boto, since I will be putting the credentials in place via some other process than code deployment.  So far, I've only figured out how to check if I have a valid connection by looking at an S3 bucket, but am hoping someone has a more direct way to check.

import unittest
import boto

class TestTest(unittest.TestCase):
    def setUp(self):
        self.s3 = boto.connect_s3()

    def tearDown(self):
        pass

    def test_connection(self):
        assert self.s3 is not None

        # must be a better way to ensure connection works
        bucket = boto.s3.bucket.Bucket(connection=self.s3, name='test_this_bucket')
        all_keys = bucket.get_all_keys()
        assert len(all_keys) == 0

If you want just to test connection, checking boto.connect_s3() is good enough. According to the docs it raises an exception if something goes wrong.

In case if you want to do more advanced scenario, you can try to make another test with bucket creation and few keys inside.

I will structure the tests the following way:

import unittest
from time import time, sleep

import boto

class BasicConnectionTest(unittest.TestCase):
    def test_connection_works(self):
        # Please add some credentials if necessary
        connection = boto.connect_s3()
        # when using unittest, I'm trying to stick to helper methods
        # because they are more verbose
        self.assertIsNotNone(connection)

class AdvancedConnectionTest(unittest.TestCase):
    def setUp(self):
        self.connection = boto.connect_s3()

    def test_bucket_can_be_created(self):
        bucket = self.create_bucket()
        self.assertIsNotNone(bucket)
        self.assertEqual(len(bucket.get_all_keys()), 0)

    def test_keys_can_be_created(self):
        bucket = self.create_bucket()
        key = self.create_key(bucket, "mykey")
        key.set_contents_from_string("Hello World!")
        sleep(2)
        contents = key.get_contents_as_string()
        self.assertEqual(contents, "Hello World!")

    def create_bucket(self):
        bucket = self.connection.create_bucket('boto-demo-%s' % int(time()))
        self.addCleanup(bucket.delete)
        return bucket

    def create_key(self, bucket, name):
        key = bucket.new_key('mykey')
        self.addCleanup(key.delete)
        return key

In this case you will be safe that if something will go wrong in your tests, addCleanup will take care of removing bucket and key.

It is also possible to test some configs, e.g. http://boto.readthedocs.org/en/latest/ref/s3.html#boto.s3.bucket.Bucket.get_lifecycle_config

Just experiment :)


USACO Arithmetic Progressions The problem I'm solving is:

given a length of an arithmetic progression, and a limit to the
terms (see below), find all progressions that work.
All terms of the progressions must be of the form a²+b², and
0 ≤ a²+b² ≤ limit.

I have the following code:
from math import ceil
with open('ariprog.in') as fin:
    ariLen = int(fin.readline().strip())
    ariLim = int(fin.readline().strip())
def generate(bound):
    max_len=((bound**2)*2)+1
    parity = [0]*max_len
    for i in range(bound+1):
        for j in range(bound+1):
            parity[i**2+j**2] = 1
    return parity

parity = generate(ariLim)
lenpar = len(parity)
big_mama_ar = []
# print(lenpar)
for a in range(lenpar-1):
    if parity[a] == 1:
        for d in range(1, ceil((lenpar-a)/(ariLen-1))):
            for n in range(1, ariLen):
                # print('a:', a)
                # print('d:', d)
                # print('n:', n)
                if parity[a+n*d] != 1:
                    break
            else:
                big_mama_ar.append((a,d))
                pass
big_mama_ar.sort(key=lambda x: x[1])
with open('ariprog.out', 'w') as fout:
    if big_mama_ar == []:
        fout.write('NONE\n')
    else:
        for i in big_mama_ar:
            fout.write(str(i[0])+' '+str(i[1])+'\n')

This code times out on my grader when ariLen is 21 and ariLim is 200. The time limit is 5 seconds, and on my computer, it takes 22 seconds.
ariprog.in is

21
200

Welcome to CodeReview!

Whitespace

The PEP8 standard, and consequently most Python linting tools, will recommend that you add another linebreak before your function definitions, plus some whitespace around your operators, etc. I won't detail this exhaustively; you're best to use the IDE of your choice - PyCharm is a reasonable one that is helpful for this.

Type hinting

bound is probably an integer, so add : int after it. It probably returns an int as well.

Subroutines

Put your global-scoped code into subroutines for ease of maintenance, legibility and testing.

Redundant pass

That pass isn't needed.

Use format strings

This:

str(i[0])+' '+str(i[1])+'\n'

can be

f'{i[0]} {i[1]}\n'

Simplify some math

This:

((bound**2)*2)+1

can be

2 * bound**2 + 1

due to operator precedence.

Truthiness

This:

if parity[a+n*d] != 1:

can be

if not parity[a + n*d]:

because 0 is falsey.

camel_case

ariLen is more commonly written ari_len in Python.


Sort a list for a GridView I've written the following piece of code to sort my list, which is the data source to my GridView, depending on the column to be sorted on. It seems to me that there is quite a bit of redundancy, and the below code may be optimized. Also, I wanted to apply the strategy pattern (note that I'm just starting out with design patterns), but it seems to me like it would be overkill. Please suggest how the following may be better written.

User is my class with several properties, including but not limited to Role, Policy, and Name. The other properties may also be included in this method, so it might grow. SortingOrder is an enum with 2 values, Ascending and Descending. SortExpression contains the value based on which the data is to be sorted.

    private void SortUsers(ref List<User> userList)
    {
        if (SortOrder == SortingOrder.Ascending)
        {
            switch (SortExpression)
            {
                case "Role":
                    userList = userList.OrderBy(user => user.Role).ToList();
                    break;
                case "Policy":
                    userList = userList.OrderBy(user => user.Policy).ToList();
                    break;
                default:
                    userList = userList.OrderBy(user => user.Name).ToList();
                    break;
            }
        }
        else
        {
            switch (SortExpression)
            {
                case "Role":
                    userList = userList.OrderByDescending(user => user.Role).ToList();
                    break;
                case "Policy":
                    userList = userList.OrderByDescending(user => user.Policy).ToList();
                    break;
                default:
                    userList = userList.OrderByDescending(user => user.Name).ToList();
                    break;
            }
        }
    }

Repeated Switches

I'm not sure what you mean by Strategy Pattern (never seen the term before), but judging by Wikipedia, I imagine you are referring to the idea of replacing the pair of switch statements, which are liable to fall out of sync and create hard-to-find bugs, with a method to look up the key (e.g. a mapping from "Role" to user => user.Role and such) and then just switch on the SortingOrder, or a method to perform the sort itself (I might be inclined to do both). This is a great idea!

This can be as complex or simple as you like, because it is abstracted away. It should pay off to simply move the existing switch into a method:

delegate IComparable UserKey(User user);

private static UserKey TranslateSortExpression(string sortExpression)
{
    switch (sortExpression)
    {
        case "Role":
            return user => user.Role;
        case "Policy":
            return user => user.Policy;
        default:
            return user => user.Name;
    }
}

private void SortUsers(ref List<User> userList)
{
    UserKey sortingKey = TranslateSortExpression(SortExpression);

    if (SortOrder == SortingOrder.Ascending)
    {
        userList = userList.OrderBy(sortingKey).ToList();
    }
    else
    {
        userList = userList.OrderByDescending(sortingKey).ToList();
    }
}

A non-obvious benefit of pulling out the expression translator is that by having it return values, the compiler will ensure that it always returns a result. Currently, if you'd added an incomplete code-path by accident you may not notice, as your code would silently fail.

Anyhow, the code remains simple, has definitely become more maintainable, and now you have an obvious place to modify the code (e.g. to if you wanted to look up the Key delegate from a dictionary (which could allow you to have 'user keys' or all manner of extensibility)).

ref List<User>

It is clear why you have done this, but it is a tad scary. My first assumption was that this code was performing an in-place sort, and why it is ref isn't necessary clear to the caller (though thankfully C# makes you acknowledge this at the call site).

Unless you have good reason to duplicate the list, you should consider an inplace sort (e.g. using List.Sort(...), though this requires writing a comparer). If not, I would consider changing the method signature so that it takes in one list and returns another. As lovely as ref variables are, explicitly returning the new list will encourage cleaner calling code (i.e. not reusing a variable name for the unsorted and then sorted list) and make the method behaviour more obvious.

Either way, you should considering adding inline documentation to this method (\\\ <summary> etc.) to remind people of the chosen behaviour.

As before, returning also gives the compiler a chance to check you haven't omitted anything. Imagine the misery that could stem from no replacing the list, and finding that some backing list you are expecting to be cloned is being mysteriously modified by some code that is operating on the sorted list (I'm not saying I expect your code will do this, but it's the sort of misery inducing error that can take all day to debug because the code doesn't fail quickly (I'm a big proponent of fail-fast: the best time to fail is at compile time)).

Furthermore, if you return a different list, it can be of a different type. If your signature looked like

private void IList<User> SortUsers(IReadOnlyList<User> unsortedUserList);

then the behaviour becomes apparent (Is it an inplace sort? No, I pass it a readonly list! Where is the output? It's a return value!)

I'll throw in a comment here about using interfaces instead of concrete implementations (e.g. IList<T> instead of List<T>) where possible. It allows people to use their own implementations (which keeps code clean and potentially more efficient) and to exploit type-variant generics (interfaces can be variant, but types cannot (IList<T> is invariant, so it doesn't apply here (though IReadOnlyList<T> is covariant))

A word about enums

You should be made aware (if you are not already) that C# enums are not type safe (they are pretty dreadful). You might consider adding an explicit check for SortingOrder.Descending and throwing if any other value is passed in (e.g. (SortingOrder)12)

Instance method

Is there a good reason this is an instance method and not a static method? If you can't think of one, then it probably should be static, otherwise another programmer may start to wonder why. Static methods just 'look' honest, because they can't change the state of instances unexpectedly (atleast, not in sane systems).

I don't know enough about the layout of your code to say whether this should or should not be static, but it's worth pondering if you haven't though about it yet.

(I note that SortExpression isn't a parameter, so it is presumably an instance member, which may be a good reason to keep this virtual, but again, not enough info in the question for us really to comment on this)


hand optimising javascript selector function I personally want a small selector function that covers the three common cases

getElementById
getElementsByClassName
getElementsByTagName

It should support contexts and should not support querySelectorAll since <opinionated> QSA is slow as hell and should be avoided </opinionated>

I have an implementation of such a function

function select(selector, context) {
    var c = selector.charAt(0),
        method;

    if (c === "#") {
        method = "getElementById";
        selector = selector.substring(1);
    } else if (c === ".") {
        method = "getElementsByClassName";
        selector = selector.substring(1);
    } else {
        method = "getElementsByTagName";
    }

    return (context || document)[method](selector);
}

And I have a benchmark.

As can be seen it's still a factor of 4 away from native support.

Can I hand optimise this further? And if so, how?

This is twice as slow as native (the original being 4x as slow)

function select (selector, context) {
    var c = selector.charAt(0),
        method,
        context = context || document;

    if (c === '#') {
        selector = selector.substring(1);
        method = context.getElementById(selector);
    }

    else if (c === '.') {
        selector = selector.substring(1);
        method = context.getElementsByClassName(selector);
    }

    else {
        method = context.getElementsByTagName(selector);
    }

    return method
}


Chess Tournament Simulator I wrote a simulator that simulates chess games. The game uses probability to determine the player that is most likely to win. though they might be upsets(a low rated player winning the game). I would love insights and suggestions on how to improve on the algorithm, general review is also welcomed.
NOTE: The names and rating were gotten from chess.com application and the code were written in jupyter-notebook
"""Chess Game simulator"""

import random
import time

NAMES = ['Jimmy', 'Samie', 'Elani', 'Aron', 'Emir', 'Sven', 'Nelson',
         'Antonio', 'Isabel', 'Wally', 'Li', 'Noam', 'Francis', 'Danya',
         'Danny', 'Engine']

RATINGS = [600, 900, 400, 700, 1000, 1100,
           1300, 1500, 1600, 1800, 2000,
           2200, 2300, 2650, 2500, 1000]

class Player:
    """This class holds the player information"""

    def __init__(self, name, rating):
        self.name = name
        self.rating = rating
        self.opponent = None

    def __repr__(self):
        return 'Player({}, {})'.format(self.name, self.rating)

class ChessGame():

    def __init__(self, players):
        self.players = players
        self.tournament_rounds = {1: 'Round 1', 2: 'Round 2', 3: 'Semi-final', 4: 'Final'}
        self.game_round = 0

    def next_round(self):
        """Go the next round of the tournament"""

        self.game_round += 1

        try:
            print(self.tournament_rounds[self.game_round])
        except KeyError:
            return

    def make_pair(self):
        """Combine two players randomly to play a chess game"""

        players = self.players[:]
        new_players = []

        while players:
            player1 = random.choice(players)
            players.remove(player1)
            player2 = random.choice(players)
            players.remove(player2)

            player1.opponent = player2
            player2.opponent = player1
            new_players.append(player1)

        self.players = new_players

    def game(self):
        """run the simulation"""

        if len(self.players) < 2:
            print('Exception: Not enough players to play a game')
            return

        self.next_round()
        print('--------------------------------------------')
        self.make_pair()
        for player in self.players:
            print(f'{player.name}({player.rating}) vs {player.opponent.name}({player.opponent.rating})')
        time.sleep(3)
        print('--------------------------------------------')
        self.players = self.score_game()

    def score_game(self):
        """Score the game using probabilities"""

        winners = []
        for player in self.players:
            total_rating = player.rating + player.opponent.rating
            player_chance = player.rating / total_rating

            maximum_range = 1000
            player_range = [i for i in range(0, int(player_chance * maximum_range))]
            choice = random.randint(0, maximum_range)
            if choice in player_range:
                print(f'{player.name} wins this round by checkmate')
                player.rating += self.game_round * 200
                winners.append(player)
            else:
                print(f'{player.opponent.name} wins this round by checkmate')
                player.opponent.rating += self.game_round * 200
                winners.append(player.opponent)

        return winners

CHESS_PLAYERS = [Player(name, rating) for name, rating in zip(NAMES, RATINGS)]

CHESS_SIM = ChessGame(CHESS_PLAYERS)
CHESS_SIM.game()
print()
CHESS_SIM.game()
print()
CHESS_SIM.game()
print()
CHESS_SIM.game()

Use immutable sequences
Turn
NAMES = ['Jimmy', 'Samie', 'Elani', 'Aron', 'Emir', 'Sven', 'Nelson',
         'Antonio', 'Isabel', 'Wally', 'Li', 'Noam', 'Francis', 'Danya',
         'Danny', 'Engine']

RATINGS = [600, 900, 400, 700, 1000, 1100,
           1300, 1500, 1600, 1800, 2000,
           2200, 2300, 2650, 2500, 1000]

into tuples instead of lists, because they're constants.
Use data classes
class Player:
    """This class holds the player information"""

    def __init__(self, name, rating):
        self.name = name
        self.rating = rating
        self.opponent = None

    def __repr__(self):
        return 'Player({}, {})'.format(self.name, self.rating)

does not need an explicit __init__ if you make it a @dataclass:
@dataclass
class Player:
    """This class holds the player information"""
    name: str
    rating: int
    opponent: Optional['Player']

though I don't think that it's appropriate to represent opponent as an attribute of a player. That belongs to the game state.
Sequence representation
Consider changing
    self.tournament_rounds = {1: 'Round 1', 2: 'Round 2', 3: 'Semi-final', 4: 'Final'}

into a global (or class-static) tuple of strings:
TOURNAMENT_ROUNDS = ('Round 1', 'Round 2', 'Semi-final', 'Final')

The indices as keys are redundant.
Iterators and responsibilities
def next_round(self):
    """Go the next round of the tournament"""

    self.game_round += 1

    try:
        print(self.tournament_rounds[self.game_round])
    except KeyError:
        return

and
            print(f'{player.name} wins this round by checkmate')
            player.rating += self.game_round * 200

seem dicey to me in terms of OOP representation. You'd be better-off having the game class yield a generator of rounds. This will make the game instance reusable.
Your score_game has the following responsibilities baked in - game simulation, and game presentation. These need to be separated - your score_game should not be printing. Also, score_game is itself not a great name, since it's not just scoring the game - it's actually running the game simulation.
Random selection
        maximum_range = 1000
        player_range = [i for i in range(0, int(player_chance * maximum_range))]
        choice = random.randint(0, maximum_range)
        if choice in player_range:

needs to be re-thought. Currently you're constructing a list, in memory, of every single number from 0 through (up to) 1000 only to do a membership test and then throw the whole thing away. Instead, just generate a random number and compare it to an upper bound.
Sleepy code
Don't arbitrarily add sleep. If you want to space out your console interface between logical operations, wait for a keypress.
Suggested
"""Chess Game simulator"""

from dataclasses import dataclass
from random import shuffle, randrange
from typing import ClassVar, Tuple, Iterable, Dict

@dataclass
class Player:
    name: str
    rating: int

    def __str__(self):
        return f'{self.name}({self.rating})'

CHESS_PLAYERS = [
    Player(name, rating)
    for rating, name in (
        ( 600, 'Jimmy'),
        ( 900, 'Samie'),
        ( 400, 'Elani'),
        ( 700, 'Aron'),
        (1000, 'Emir'),
        (1100, 'Sven'),
        (1300, 'Nelson'),
        (1500, 'Antonio'),
        (1600, 'Isabel'),
        (1800, 'Wally'),
        (2000, 'Li'),
        (2200, 'Noam'),
        (2300, 'Francis'),
        (2650, 'Danya'),
        (2500, 'Danny'),
        (1000, 'Engine'),
    )
]

class Match:
    def __init__(self, p1: Player, p2: Player):
        self.p1, self.p2 = p1, p2
        total = self.p1.rating + self.p2.rating
        if randrange(total) < self.p1.rating:
            self.winner = self.p1
        else:
            self.winner = self.p2

    def __str__(self):
        return f'{self.p1} vs {self.p2}'

    @property
    def outcome(self) -> str:
        return f'{self.winner} wins this round by checkmate'

class Round:
    def __init__(self, name: str, matches: Iterable[Match]):
        self.name = name
        self.matches = tuple(matches)

    def print(self):
        print(self.name)
        print('-'*45)
        print('\n'.join(str(match) for match in self.matches))
        print('-'*45)
        print('\n'.join(match.outcome for match in self.matches))

class ChessGame:
    ROUND_NAMES: ClassVar[Tuple[str]] = ('Round 1', 'Round 2', 'Semi-final', 'Final')

    def __init__(self, players: Iterable[Player]):
        self.players: Tuple[Player] = tuple(players)

    def __iter__(self) -> Iterable[Round]:
        players = list(self.players)

        for i_round, name in enumerate(self.ROUND_NAMES):
            shuffle(players)
            half = len(players)//2
            round = Round(
                name,
                (
                    Match(p1, p2)
                    for p1, p2 in zip(players[:half], players[half:])
                )
            )
            yield round

            for match in round.matches:
                match.winner.rating += (i_round + 1) * 200

            players = [match.winner for match in round.matches]

    def print_run(self):
        for round in self:
            round.print()
            print()

def main():
    sim = ChessGame(CHESS_PLAYERS)
    sim.print_run()

if __name__ == '__main__':
    main()

The biggest 'gotcha' here is that, whereas the ChessGame is now reusable, player instances (just as in your original code) are mutated, so if you want to do repeated simulations you have to make copies of your player instances.


Create a template class for same algorithm different type of member variable I want a template class that defines the same algorithms, but can work on different type of data stored in a member variable.
The exact type of data ideally depends only on a single template parameter.
I don't want run time polymorphism (like have an argument of a pointer to base class that can accept all derive class).

Below is my current solution using variadic template. Is there a simpler/better way?

#include <vector>
class Data1 {
public:
    Data1(int size, int stride)
            : _v(size*stride), _size(size), _stride(stride) {}
    int& operator[](int id) {return _v[id*_stride];}
    const int _size;
private:
    const int _stride;
    std::vector<int> _v;
};

class Data2 {
public:
    Data2(int size) : _v(size), _size(size) {}
    int& operator[](int id) {return _v[id];}
    const int _size;
private:
    std::vector<int> _v;
};

template <typename Data, typename... Data_arg>
class Base_template {
public:
    Base_template(Data_arg...data_arg): _data(data_arg...) {}
    void set_all() {
        for (int i = 0; i < _data._size; ++i) _data[i] = _val;
    }
protected:
    int _val;
    Data _data;
};

class Thing1 : public Base_template<Data1, int, int> {
public:
    Thing1(const int size)
            : Base_template(size, 3) {
        this->_val = 1;
    }
};
class Thing2 : public Base_template<Data2, int> {
public:
    Thing2(const int size)
            : Base_template(size) {
        this->_val = 10;
    }
};

int main() {
    Thing1 t1(100);
    t1.set_all();
    Thing2 t2(10);
    t2.set_all();
}

Notes:

I use variadic template because the data needs to be initialize.
In the simple example above Data2 actually can be replaced by data1 with a stride of 1. But in the actually code, this is not the case.

I'm not sure I understand exactly what you're going for, but I think your Base_template has way too many parameters. You have:

template <typename Data, typename... Data_arg>
class Base_template {
public:
    Base_template(Data_arg...data_arg): _data(data_arg...) {}
    void set_all() {
        for (int i = 0; i < _data._size; ++i) _data[i] = _val;
    }
protected:
    int _val;
    Data _data;
};

but I think all you want is:

template <typename Data>
class Base_template {
public:
    template<typename... Args>
    Base_template(Args&&... args): _data(std::forward<Args>(args)...) {}
    void set_all() {
        for (int i = 0; i < _data._size; ++i) _data[i] = _val;
    }
protected:
    int _val;
    Data _data;
};

Also, notice in passing that your naming convention of "leading underscore" might lead you into conflict with reserved identifiers if you're not super careful. I prefer to use "trailing underscore" for private member variables, just so that I don't have to think about the complicated reserved-identifier rules at all.


C Makefile boilerplate This is the current Makefile that I use with my C projects.  Here is what I would like to be reviewed:

Reusability - is this Makefile easy to use for multiple separate projects with minimal modification?
Organization - is this Makefile organized in a logical and readable way?
Dynamics - are there Make features (or compiler options/warnings) that I am not taking advantage of that could make my Makefile more powerful?
Miscellaneous - what other things could I improve?

CXX = g++-4.9
CC = gcc-4.9
DOXYGEN = doxygen
CFLAGS = -fdiagnostics-color=always -std=gnu11 -s -c -g3 -O3 -time
WARNINGS = -Werror -Wall -Wextra -pedantic-errors -Wformat=2 -Wno-import -Wimplicit -Wmain -Wchar-subscripts -Wsequence-point -Wmissing-braces -Wparentheses -Winit-self -Wswitch-enum -Wstrict-aliasing=2 -Wundef -Wshadow -Wpointer-arith -Wbad-function-cast -Wcast-qual -Wcast-align -Wwrite-strings -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-declarations -Wredundant-decls -Wnested-externs -Winline -Wdisabled-optimization -Wunused-macros -Wno-unused
LDFLAGS =
LIBRARIES = -lcurl
SOURCES = main.c test.c
OBJECTS = $(SOURCES:.c=.o)
EXECUTABLE = test

all: $(SOURCES) $(EXECUTABLE)

$(EXECUTABLE): $(OBJECTS)
    $(CC) $(LDFLAGS) $(OBJECTS) -o $@ $(LIBRARIES)

debug: CFLAGS += -DDEBUG -g
debug: $(SOURCES) $(EXECUTABLE)

.c.o:
    $(CC) $< -o $@ $(CFLAGS) $(WARNINGS)

.PHONY: doc clean
doc:
    $(DOXYGEN) doxygen.config
clean:
    rm -rf $(EXECUTABLE) $(OBJECTS)

Dependencies: right now your makefile will let changes in .h to remain unnoticed.

DEPS := $(SOURCES:.c=.d)

.c.d:
    $(CC) -o $< -MM $(CFLAGS)

-include $(DEPS)

PS: it is highly recommended to remove -c from CFLAGS and mention it explicitly in the .c.o rule.


Bag data structure (a set that can store multiple copies of items) I am trying to make my class a bit cleaner, especially some of its methods like Count, __add__, remove and __iter__. I feel like I could use cleaner code on the above methods and get the same results.

Bag class represents and defines methods, operators, and an iterator for the Bag class. Bags are similar to sets, and have similar operations but unlike sets they can store multiple copies of items. Store the information in bags as dictionaries whose elements are keys whose associated values are the number of times the key occurs in the bag.

from collections import defaultdict
from goody import type_as_str
import copy
from collections import Counter

class Bag:

     def __init__(self, items = []):
         self.bag_value = defaultdict(int)
         for item in items:
             self.bag_value[item] += 1

     def __repr__(self):
         bag_list = []
         for item, count in self.bag_value.items():
             bag_list.extend(list(item*count))
         return 'Bag(' + str(bag_list) + ')'

     def __str__(self):
         return 'Bag(' + ','.join(str(item) + '[' + str(count) + ']' for item, count in self.bag_value.items()) + ')'

     def __len__(self):
         bag_len = 0
         for value in self.bag_value:
             bag_len += self.bag_value[value]
         return bag_len

     def unique(self):
         return len(self.bag_value)

      def __contains__(self, item):
          return item in self.bag_value

      def count(self, item):
          if item in self.bag_value:
             return self.bag_value[item]
          else:
             return 0

      def add(self, new):
          self.bag_value[new] += 1

      def __add__(self,right):
          mergedbag = Bag()
          mergedbag.bag_value = copy.copy(self.bag_value)
          for item in right.bag_value.keys():
              mergedbag.bag_value[item] += right.bag_value[item]
          return mergedbag

      def remove(self, item):
          if item in self.bag_value:
              if self.bag_value[item] == 1:
                  del self.bag_value[item]
              else:
                  self.bag_value[item] -= 1
          else:
              raise ValueError(str(item) + ' not in bag')

      def __eq__(self, right):
          if type(right) is not Bag:
              raise TypeError('Cannot compare Bag with' + type_as_str(right) + '. Can only compare Bag with Bag')
          else:
              return (Counter(self.bag_value)==Counter(right))

      def __ne__(self, right):
          return not self.__eq__(right)

      def __generate__(self, bag_value):
          for item in self.bag_value:
              for value in range(self.bag_value[item]):
                  yield item

      def __iter__(self):
          return self.__generate__(self.bag_value)

if __name__ == '__main__':
#     bag = Bag(['d','a','b','d','c','b','d'])
#     bag2 = Bag(['d','a','b','d','c','b','d'])
#     bag3 = Bag(['d','a','b','d','c','b'])
#     print(bag == bag2)
#     print(bag == bag3)
#     print(bag != bag2)
#     print(bag != bag3)
    import driver
    driver.driver()

As you're trying to implement a multiset (or bag) I would recommend you to use collections.Counter which already does the same thing:

  The Counter class is similar to bags or multisets in other languages.

So, simply inherit from Counter class and add your additional methods in the class or override the already present methods from Counter class as per your requirements. Counter class also supports &, |, - operators to perform various multiset operations.

Now, coming back to your code:

def __init__(self, items = []):
    self.bag_value = defaultdict(int)
    for item in items:
        self.bag_value[item] += 1

Never use a mutable object as default value of an argument as there value can persist between calls, the recommended way is to use None as default value and then check it inside of the __init__ method. As we are now inheriting from Counter we can directly operate on self instead of using a defaultdict to keep the count.

class Bag(Counter):

     def __init__(self, _items=None, **kwargs):

          _items = [] if _items is None else _items
          super().__init__(_items, **kwargs)

  Why both _items and kwargs?

It depends on how you want to use it ultimately, but with this signature you can use it as:

Bag('aaabbccd')
Bag(a=3, b=5, c=7)
Bag('aabbcc', a=5, c=7, r=9)

I used _items to handle the case when user actually passed a key named items:

Bag(a=5, c=7, r=9, items=9)

def __repr__(self):
    bag_list = []
    for item, count in self.bag_value.items():
         bag_list.extend(list(item*count))
    return 'Bag(' + str(bag_list) + ')'

Counter class already has a method to handle this called elements() which returns an itertools.chain object which is an iterator, we can call list on it and we are done:

def __repr__(self):
    return 'Bag({})'.format(list(self.elements()))

def __str__(self):
    return 'Bag(' + ','.join(str(item) + '[' + str(count) + ']' for item, count in self.bag_value.items()) + ')'

We can take advantage of string formatting here to make the above code a little concise and easier to read:

def __str__(self):
    format_str = '{}[{}]'.format
    return 'Bag({})'.format(', '.join(format_str(k, v) for k, v in self.items()))

def __len__(self):
     bag_len = 0
     for value in self.bag_value:
         bag_len += self.bag_value[value]
     return bag_len

In our class we can obtain the length by simply summing the .values(), sum() returns 0 when the Bag is empty so it saves a lots of line:

def __len__(self):
    return sum(self.values())

We can simply drop __contains__ as Counter can already handle that part, and for unique() we can use len(self.keys()).

def __add__(self,right):
    mergedbag = Bag()
    mergedbag.bag_value = copy.copy(self.bag_value)
    for item in right.bag_value.keys():
        mergedbag.bag_value[item] += right.bag_value[item]
    return mergedbag

Here as we're only dealing with integer values copy.copy is not actually required, and instead of manual loop use the .update() method of dictionary:

def __add__(self, right):
    merged_bag = Bag()
    merged_bag.update(self)
    merged_bag.update(right)
    return merged_bag

In .remove() the only change is that I used string formatting with __repr__ version of item. __repr__ is more useful as with str() you won't be able to differentiate between 1 and '1'.

raise ValueError('{!r} not in bag'.format(item))

def __eq__(self, right):
    if type(right) is not Bag:
        raise TypeError('Cannot compare Bag with' + type_as_str(right) + '. Can only compare Bag with Bag')
    else:
        return (Counter(self.bag_value)==Counter(right))

The recommended way to check the type of an object is to use isinstance rather than calling type. And in else block I simply called the super class's __eq__ to handle the result instead of creating Counters:

def __eq__(self, right):
    if not isinstance(right, Bag):
        raise TypeError(('Cannot compare Bag with object of type {!r}. Can only '
                         'compare Bag with Bag').format(type(right).__name__))
    else:
        return super().__eq__(right)

def __iter__(self):
    return self.__generate__(self.bag_value)

Never define your own dunder method(__generate__), these methods are considered special by Python and if any future version of Python defines a method by the same name then your code can result in some unknown errors. In our __iter__ method we can simply yield from self.elements():

def __iter__(self):
    yield from self.elements()

Note that there should be only be single line of space between method of a class and your indentation is also messed up, always use 4 spaces no tabs(Change your IDE's setting to use 4 spaces for a tab). Read PEP-008 for more information on styling-guide.

Complete code:

from collections import Counter

class Bag(Counter):

     def __init__(self, _items=None, **kwargs):

         _items = [] if _items is None else _items
         super().__init__(_items, **kwargs)

     def __repr__(self):
         return 'Bag({})'.format(list(self.elements()))

     def __str__(self):
         format_str = '{}[{}]'.format
         return 'Bag({})'.format(', '.join(format_str(k, v) for k, v in self.items()))

     def __len__(self):
         return sum(self.values())

     def unique(self):
         return len(self.keys())

     def count(self, item):
         return self[item]

     def add(self, new):
         self[new] += 1

     def __add__(self, right):
         merged_bag = Bag()
         merged_bag.update(self)
         merged_bag.update(right)
         return merged_bag

     def remove(self, item):
         if item in self:
             if self[item] == 1:
                 del self[item]
             else:
                 self[item] -= 1
         else:
             raise ValueError('{!r} not in bag'.format(item))

     def __eq__(self, right):
         if not isinstance(right, Bag):
             raise TypeError(('Cannot compare Bag with object of type {!r}. Can only '
                              'compare Bag with Bag').format(type(right).__name__))
         else:
             return super().__eq__(right)

     def __ne__(self, right):
         return not self.__eq__(right)

     def __iter__(self):
         yield from self.elements()


C function to move and collapse cells in 2048 game I'm implementing a clone of 2048 in C with SDL, and I have the following function for performing a movement on a column vector/array

/**
 * collapseVector: Collapse a vector "2048" style, flush the cells in one direction, joining them if legal.
 * @param _vector 1-D array to be collapsed
 * @param _score Score counter
 * @param invert Whether or not to inverse-flush vector. Useful for Up(false) vs. Down(true), and so on
 */
void collapseVector(int _vector[MAX_BOARD_POS], int **_score, bool invert) {
    int idx;
    int buf[MAX_BOARD_POS] ={0};
    if (invert) {
        idx = MAX_BOARD_POS - 1;
        for (int i = MAX_BOARD_POS - 1; (MAX_BOARD_POS - i) < MAX_BOARD_POS + 1; --i){
            if(!_vector[i]){
                continue;
            }
            if(!buf[idx]){
                buf[idx] = _vector[i];
            }else if(_vector[i] == buf[idx]){
                ++buf[idx];
                **_score += 1 << buf[idx];
            }else{
                buf[--idx] = _vector[i];
            }
        }

    } else {
        idx = 0;
        for (int i = 0; i < MAX_BOARD_POS; ++i) {
            // If it's an empty cell (0) ignore it. The buf vector is zero-filled anyway
            if (!_vector[i]) {
                continue;
            }
            // Special case for filling in the first cell of buffer
            if (!buf[idx]) {
                buf[idx] = _vector[i];
            } else if (_vector[i] == buf[idx]) {
                // If we have identical neighbouring cells join them
                ++buf[idx];
                // Add 2^buf[idx] to our score counter.
                **_score += 1 << buf[idx];
            } else {
                // Flush cells to the end of vector
                buf[++idx] = _vector[i];
            }
        }
    }
    memcpy(_vector, buf, MAX_BOARD_POS * sizeof(*_vector));
}

This code works perfectly, but it's long and repetitive. How can I improve this? The two for loops in here are very similar and I wonder whether I could join them perhaps.

Don't repeat yourself

The two loops can be joined pretty easily. We basically just need to factor out the (few) pieces that are different between the two, and pull those pieces out of the loop. The loops are just short enough, however, that the result may not be a whole lot shorter than the two loops are right now (so if you care more about short code than lack of repetition, you may not want to do this).

Comments

Right now, you've provided comments for what the loop does in one of the loops, but not the other. By collapsing the loops together, we can eliminate this inconsistency. That leaves the question of whether to include the comments or not. In general, I'd rather write the code to be self-documenting instead. See below for at least some of how to do that.

Symbolic names

Instead of writing the code to explicitly state the an empty cell contains the value 0, I'd prefer to give that a name:

#define EMPTY 0

// ...

if (_vector[i] == EMPTY)
   // ...

This (probably) eliminates any requirement for a comment to tell us that we're checking whether that cell is empty.

Unnecessary Indirection

It appears that you can pass _score as a pointer to int rather than a pointer to pointer to int as you're doing now. At least as far as I can see, the extra level of indirection isn't accomplishing anything useful at all.

Bool Parameters

I generally discourage using bools as function parameters. I'd rather use an enumeration that directly states what you want to express:

enum { UP, DOWN };

collapseVector(..., UP);

...seems much more meaningful than:

collapsVector(..., true);

Final Code

One possibility would be something on this general order:

#define EMPTY 0

bool down(int a, int b) {
    return a < b;
}

bool up(int a, int b) {
    return b < a;
}

enum direction { UP, DOWN };

/**
* collapseVector: Collapse a vector "2048" style, flush the cells in one direction, joining them if legal.
* @param _vector 1-D array to be collapsed
* @param _score Score counter
* @param invert Whether or not to inverse-flush vector. Useful for Up(false) vs. Down(true), and so on
*/
void collapseVector(int _vector[MAX_BOARD_POS], int *_score, direction dir) {
    int idx = 0;
    int buf[MAX_BOARD_POS] = { 0 };
    int start = 0;
    int end = MAX_BOARD_POS;
    int inc = 1;
    bool (* cmp)(int, int) = down;

    if (dir == UP) {
        idx = start = MAX_BOARD_POS - 1;
        end = 0; // Not sure if this is quite right, but I think it's at least close
        inc = -1;
        cmp = up;
    }

    for (int i = start; cmp(i, end); i += inc) {
            if (_vector[i] == EMPTY) {
                continue;
            }
            if (buf[idx] == EMPTY) {
                buf[idx] = _vector[i];
            }
            else if (_vector[i] == buf[idx]) {
                ++buf[idx];
                *_score += 1 << buf[idx];
            }
            else {
                buf[idx += inc] = _vector[i];
            }
        }
    }
    memcpy(_vector, buf, MAX_BOARD_POS * sizeof(*_vector));
}


Calculate tax return Please consider this code for a TaxReturnCalculator (and ignore both the suboptimal use of float instead of BigDecimal and non-existing parameter validation):
public class TaxReturnCalculator {

    private static final float FIFTY_PERCENT = 0.5f;
    private static final float THIRTY_PERCENT = 0.3f;
    private static final float MAXIMUM_AMOUNT_TAX_RETURN = 1000f;

    public static float calculateTaxReturn(float income)
    {
        float basicTaxReturn = calculateBasicTaxReturn(income);

        float taxReturnAddition = calculateTaxReturnAddition(basicTaxReturn);

        float taxReturn = basicTaxReturn + taxReturnAddition;

        float taxReturnLimited = calculateTaxReturnLimited(taxReturn);

        return taxReturnLimited;
    }

    private static float calculateBasicTaxReturn(float income)
    {
        return income * FIFTY_PERCENT; // More complex calculation here
    }

    private static float calculateTaxReturnAddition(float basicTaxReturn)
    {
        return basicTaxReturn * THIRTY_PERCENT;  // More complex calculation here
    }

    private static float calculateTaxReturnLimited(float taxReturn)
    {
        float taxReturnLimited;

        if(taxReturn > MAXIMUM_AMOUNT_TAX_RETURN)
        {
            taxReturnLimited = MAXIMUM_AMOUNT_TAX_RETURN;
        }
        else
        {
            taxReturnLimited = taxReturn;
        }

        return taxReturnLimited;
    }
}

I designed the calculateTaxReturn(float income) method to follow the

One Level of Abstractoin per Function

practice recommended in Clean Code by Robert C. Martin: The three functions inside this method are on the same level of abstraction.
However, what doesn't seem well designed to me is that calculateTaxReturnLimited(taxReturn) is always called and thus always produces a taxReturnLimited -- even if the tax return is not to be limited (because it's smaller than the maximum amount)! Isn't this confusing?
Of course I could avoid this by not having a calculateTaxReturnLimited function at all and put it's code directly inside of the calculateTaxReturn, but that would vialote the One Level of Abstractoin per Function principle.
Renaming the function to something like limitTaxReturnIfNecessary doesn't seem good to me either, having multiple ...IfNecessary functions seems like a clutter and code smell to me.
Is this a trade-off? How can it be resolved best to follow best practices?

You don't need to write ...ifNecessary if you just describe the property you want to have in the return value, and then you just know that after calling it, it has that property. You want the return amount to be clamped so it is less than or equal to the maximum amount. It doesn't matter that it might not change it, but it DOES matter to the business logic that it has been applied (i.e. checked). The actual name isn't that important in this case because I think I'd drop it anyway, as per below.
First, I just rewrote your code to use a member variable instead of being static, and without all the intermediate variables:
public class TaxReturn {

    private static final float MAXIMUM_RETURN_AMOUNT= 1000f;

    private final float income;

    public TaxReturn(float income) {
        this.income = income;
    }

    public float returnAmount() {
        return clampReturnAmount(base() + addition());
    }

    private float base() {
        return income * 0.5f; // More complex calculation here
    }

    private float addition() {
        return base() * 0.3f; // More complex calculation here
    }

    private static float clampReturnAmount(float returnAmount) {
        return returnAmount > MAXIMUM_RETURN_AMOUNT ? MAXIMUM_RETURN_AMOUNT
                : returnAmount;
    }

}

But this clampReturnAmount function is just Math.min by a different name. Using Math.min to enforce a limit is common enough to be well understood why we're calling it, especially if we're naming the second argument something like argument MAXIMUM_RETURN_AMOUNT. So just:
public class TaxReturn {

    private static final float MAXIMUM_RETURN_AMOUNT = 1000f;

    private final float income;

    public TaxReturn(float income) {
        this.income = income;
    }

    public float returnAmount() {
        return Math.min(base() + addition(), MAXIMUM_RETURN_AMOUNT);
    }

    private float base() {
        return income * 0.5f; // More complex calculation here
    }

    private float addition() {
        return base() * 0.3f; // More complex calculation here
    }

}


Segementing a recursive tree in haskell Could somebody help me work out how to abstract the recursion from this code for me please?
I'm trying to write something that takes a pandoc structure, and subdivides the tree
whenever it finds a sentence, so:

This sentence. Should split.

becomes:

  This sentence.
   Should split.

I have a working implementation, but the recursive part is a bit repetitive:

segment :: [Inline] -> [Inline]
segment [] = []
segment [el] = [el]
segment els @ (head:tail) =
  case head of
    Emph ch -> map (Emph . toList) (segment ch) ++ segment tail
    Strong ch -> map (Strong . toList) (segment ch) ++ segment tail
    Strikeout ch -> map (Strikeout . toList) (segment ch) ++ segment tail
    Superscript ch -> map (Superscript . toList) (segment ch) ++ segment tail
    Subscript ch -> map (Subscript . toList) (segment ch) ++ segment tail
    SmallCaps ch -> map (SmallCaps. toList) (segment ch) ++ segment tail
    Quoted qt ch -> map (Quoted qt . toList) (segment ch) ++ segment tail
    Cite cs ch -> map (Cite cs . toList) (segment ch) ++ segment tail
    Span attr ch -> map (Span attr . toList) (segment ch) ++ segment tail
    _ ->
      maybe [sentence els] (\i -> (sentence $ take i els) : segment (drop i els))
        $ (1 +) <$> List.findIndex split els
  where
    sentence = Span ("sentence", [], [])

split :: Inline -> Bool
split (Str str) = endsWith "." str || endsWith "?" str || endsWith "!" str
split _         = False

endsWith :: String -> String -> Bool
endsWith suff str = suff `List.isSuffixOf` str

toList :: a -> [a]
toList = (: [])

I'm trying to work out a way to hand in some f that will model the part that splits the structure
(i.e the bottom bit), but I'm struggling as the function has to be [Inline] -> [Inline], it's also
as if it needs to be Inline -> [Inline] -> [Inline] as each of the recursive steps need the original
constructor in order to preserve the structure of the document, ie:

This sentence. Should split.

becomes:

  This sentence.
   Should split.

I'll start with something unrelated to recursion: A first cleanup step might be to reduce the repetition in segment:

segment els @ (head:tail) =
  let constr = case head of
                 Emph ch        -> Just (Emph, ch)
                 Strong ch      -> Just (Strong, ch)
                 Strikeout ch   -> Just (Strikeout, ch)
                 Superscript ch -> Just (Superscript, ch)
                 Subscript ch   -> Just (Subscript, ch)
                 SmallCaps ch   -> Just (SmallCaps, ch)
                 Quoted qt ch   -> Just (Quoted qt, ch)
                 Cite cs ch     -> Just (Cite cs, ch)
                 Span attr ch   -> Just (Span attr, ch)
                 _              -> Nothing
      sentence = Span ("", ["sentence"], [])
  in case constr of
    Just (inlnConstr, ch) -> map inlnConstr (segment ch) ++ segment tail
    _               ->
      maybe [sentence els] (\i -> (sentence $ take i els) : segment (drop i els))
        $ (1 +) <$> List.findIndex split els

Note that we not only cut away the repeating (segment ch) and ++ segment tail, but also the toList, which is just an id here (as the result of segment already is a List.

Next would be the splitting, which could be done using break instead of findIndex, drop, and take:

      case break split els of
        (_, [])    -> [sentence els]
        (xs, y:ys) -> sentence (xs ++ [y]) : segment ys

The last part of your post might better be asked on stackoverflow, as there is no working code for this yet.


Avoiding label in a loop to sort links How can I refactor my code to remove the label and the need for it? I am aware that a loop with label is "forbidden". But I can not find a way to rewrite this without the label.

private List<myList> sortLinks(SegmentType s, Set<myList> LinkSet) {
        List<myList> LinkList = new LinkedList<myList>();

        String dep = s.getDep().toString();
        mainLoop: for (int index = 0; !LinkSet.isEmpty(); index++) {

            for (Iterator<myList> iterator = LinkSet.iterator(); iterator.hasNext(); ) {
                myList link = iterator.next();
                if (link.getLegDep().toString().equals(dep)) {
                    iterator.remove();
                    link.setLine(s.getLineCode());
                    link.setNb(s.getNb());
                    link.setSuff(s.getSuff());
                    link.setIndex(index);
                    linkList.add(link);

                    dep = link.getDest().toString();
                    continue mainLoop;
                }
            }

            return Collections.emptyList();
        }
        return linkList;
    }

In general, labelled code is uncommon, but 'forbidden' is a bit harsh. Break, and Continue have better characteristics than GoTo, and should not be 'painted with the same brush'.

The logic in your code is convoluted though.... you are 'searching' the input set, and ensuring that all members of the input set match a condition. As you search, if the member matches, you remove the member. If one of the members does not match, you immediately return an empty result.

Note, that if a 'middle' member fails to match, you have already removed the first members, and yet you return an empty set.

Your logic could be significantly simplified if you extracted part of your method as a 'helper' function:

private myList searchAndRemove(String dep, Set<myList> candidates) {
    for (Iterator<myList> iterator = candidates.iterator(); iterator.hasNext(); ) {
        myList link = iterator.next();
        if (link.getLegDep().toString().equals(dep)) {
            iterator.remove();
            return link;
        }
    }
    return null;
}

private List<myList> sortLinks(SegmentType s, Set<myList> LinkSet) {

    List<myList> matched = new LinkedList<myList>();

    String dep = s.getDep().toString();
    int index = 0;
    while (!LinkSet.isEmpty()) {
        myList link = searchAndRemove(dep, LinkSet);
        if (link == null) {
            return Collections.emptyList();
        }
        matched.add(link);
        link.setLine(s.getLineCode());
        link.setNb(s.getNb());
        link.setSuff(s.getSuff());
        link.setIndex(index++);

        dep = link.getDest().toString();
    }

    return matched;
}

In addition to the above, note that your class names and variable names are really, really bad....

myList is a class, and should be CapitalCamelCase naming style, and have a different name.
LinkList is a variable name, and should be lowerCamelCase, and have a name like matchedSegments
LinkSet is also a variable name, and should be lowerCamelCase.

In general, with the name-pollution of having a class with 'Link' in the name (probably because it's not a horrible name', but it conflicts with LinkedList, so try to avoid Objects with Link as the name.

Additionally, note that your index variable was not an index in to the source data, but an index of the output segment. Including it as part of the original 'for' loop implied that it was used as an index in to that. The reality is that it is independent. I have edited it to make that clear.


Project Euler 42 - finding triangle number V2 This is my revised solution to Problem 42 from Project Euler, based on suggestions in this original post Project Euler 42 - Finding triangle numbers

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.IO;
using System.Collections;
using System.Diagnostics;

namespace ProjectEuler
{
    class p42trianglenumber
    {
        // p42. tn = ½n(n+1)
        // A=1, B=2, etc. case insensitive.
        public static void NotMain()
        {
            var sw = new Stopwatch();
            sw.Start();
            int numberTriangleWords = 0;
            string[] words = ReturnStringArray("p042_words.txt");
            int[] triangleNumbers = BuildTriangleNumber();

            foreach (string word in words)
            {
                var score = GetScore(word);

                if (triangleNumbers.Contains(score))
                {
                    numberTriangleWords++;
                }
            }
            sw.Stop();
            Console.WriteLine(numberTriangleWords + Environment.NewLine + sw.ElapsedMilliseconds.ToString() + "ms");
            Console.ReadLine();
        }

        private static string[] ReturnStringArray(string fileLocation)
        {
            StreamReader sr = new StreamReader(fileLocation);
            string[] words = sr.ReadLine().Split(',');
            return words;
        }

        private static bool IsWordTriangleNumber(int wordVal, int[] triangleNumbers)
        {
            return (triangleNumbers.Contains(wordVal));
        }

        private static int[] BuildTriangleNumber()
        {
            int triangleNumberValue = 0;
            int[] triangleNumbers = new int[50];

            for (int i = 0; i < 50; i++)
            {
                if (i == 0)
                    triangleNumbers[i] = triangleNumberValue;
                else
                {
                    triangleNumberValue += i;
                    triangleNumbers[i] = triangleNumberValue;
                }
            }
            return triangleNumbers;
        }

        private static int GetScore(string word)
        {
            var total = 0;
            foreach (var letter in word)
            {
                total += GetScore(letter);
            }
            return total;
        }

        private static int GetScore(char letter)
        {
            var start = char.IsLower(letter) ? 'a' : 'A';
            var score = (int)letter - (int)start + 1;
            if (score < 0 || score > 26) return 0;
            return score;
        }
    }
}

Possible problem

private static string[] ReturnStringArray(string fileLocation)
{
    StreamReader sr = new StreamReader(fileLocation);
    string[] words = sr.ReadLine().Split(',');
    return words;
}

With this method you don't read the whole file but only the first line in the textfile. Usually project Euler problem don't involve only one line of anything but rather some 10k of something.

In the first 5 lines of code I would change 3 variables/method names

        int numberTriangleWords = 0;
        string[] words = ReturnStringArray("p042_words.txt");
        int[] triangleNumbers = BuildTriangleNumber();

The problem is about getting the number of triangle words.

int numberOfTriangleWords = 0

You are reading a text file as words.

string[] words = ReadFileAsWords("p042_words.txt");

You are creating / building an array of numbers

int[] triangleNumbers = BuildTriangleNumbers();

This is just nitpicking on the naming, but naming is a very important part of writing good readable and maintainable code.

This method

    private static bool IsWordTriangleNumber(int wordVal, int[] triangleNumbers)
    {
        return (triangleNumbers.Contains(wordVal));
    }

isn't used anywhere. You should delete it.

    private static int[] BuildTriangleNumber()
    {
        int triangleNumberValue = 0;
        int[] triangleNumbers = new int[50];

        for (int i = 0; i < 50; i++)
        {
            if (i == 0)
                triangleNumbers[i] = triangleNumberValue;
            else
            {
                triangleNumberValue += i;
                triangleNumbers[i] = triangleNumberValue;
            }
        }
        return triangleNumbers;
    }

Because triangleNumbers is an array of the int struct, the items are initialized using default(T) which for an int is 0. Therefor if you just let the loop start at 1 you wouldn't need the check if (i == 0) at all which would also remove the braceless single line statement after the if.
I would like to encourage you to always use braces {} to make your code less error prone. Saving a few milliseconds by omitting them can lead to problems later on which patching can take a lot of time.

    private static int[] BuildTriangleNumber()
    {
        int triangleNumberValue = 0;
        int[] triangleNumbers = new int[50];

        for (int i = 1; i < 50; i++)
        {
            triangleNumberValue += i;
            triangleNumbers[i] = triangleNumberValue;
        }
        return triangleNumbers;
    }

I like the overloading of the GetScore() method, but the GetScore(char) method can be improved.

    private static int GetScore(char letter)
    {
        var start = char.IsLower(letter) ? 'a' : 'A';
        var score = (int)letter - (int)start + 1;
        if (score < 0 || score > 26) return 0;
        return score;
    }

There is no need to first assign a or A to start and then later on cast it to an int. The int value of a == 97 and of A == 65. So using these values should speed the whole thing up.

Edit Based on @Caridorc's comment
I have extracted these values to private static reaonly variables using a naming scheme which isn't legit by the meaning of the c# naming guidelines but looks better with the underscore in it.

    private static int ASCIIValue_a = 97;
    private static int ASCIIValue_A = 65;
    private static int GetScore(char letter)
    {
        var start = char.IsLower(letter) ? ASCIIValue_a : ASCIIValue_A;
        var score = (int)letter - start + 1;
        if (score < 0 || score > 26) return 0;
        return score;
    }


Finding word count for a given string I wrote a program to implement to find the count of words for a given string.
I have applied two different approaches.
The test works for these two approaches and I am looking for other approaches to solve this problem.
Thanks for your valuable comments advance.
package test;

import main.algorithms.WordCount;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;

public class WordCountTest {

    WordCount wc;

    @Before
    public void setUp(){
        wc = new WordCount();
    }

    @Test
    public void testWordCount(){

        String str = "nesly amazon google facebook apple";

        Assert.assertEquals(5,wc.countWords(str));

        Assert.assertEquals(5,wc.countWordsTokenizer(str));
    }

}

package main.algorithms;

import java.util.StringTokenizer;

public class WordCount {

    public int countWords(String str)
    {
        int end = 0;
        int in = 1;

        int state = end; int wc = 0; int i = 0;

        while (i < str.length())
        {
            if(str.charAt(i) == ' ' || str.charAt(i) == '\n'
                    || str.charAt(i) == '\t')
                state = end;

            else if (state == end)
            {
                state = in;
                ++wc;
            }
            ++i;
        }
        return wc;
    }

    public int countWordsTokenizer(String str){

        return new StringTokenizer(str).countTokens();
    }

}

Your approach works well but it doesn't consider punctuation, for example:
@Test
public void testWordCount(){
    String str = "Hey — how are you?";

    assertEquals(4, wc.countWords(str)); // Fail
    assertEquals(4, wc.countWordsTokenizer(str)); // Fail
}

Also StringTokenizer is discouraged for new code. From the Javadoc:

StringTokenizer is a legacy class that is retained for compatibility
reasons although its use is discouraged in new code. It is recommended
that anyone seeking this functionality use the split method of
String or the java.util.regex package instead.

The split method accepts a regex as input. This is to split by non-words characters:
public int countWordsSplit(String str) {
    if (str == null || str.isEmpty()) {
        return 0;
    }
    return str.split("\\W+").length;
}

Test:
@Test
public void testWordCount(){
    String str = "Hey — how are you?\t\n";

    assertEquals(4, wc.countWords(str)); // Fail
    assertEquals(4, wc.countWordsTokenizer(str)); // Fail
    assertEquals(4, wc.countWordsSplit(str)); // Pass
}

Edge cases
wc.countWordsSplit("Bill's house"); // returns 3

wc.countWordsSplit("1 2"); // returns 2
wc.countWords("1 2"); // returns 2
wc.countWordsTokenizer("1 2"); // returns 2

// Letters with accents, umlaut etc.
// Other edge cases...

Considerations
It all depends on how you define what a "word" is in the context of your program.
Define what a word is by making some assumptions. Then find a solution that satisfies your requirements. This approach prevents you from having an over-complicated solution that your program doesn't need.
More info here
Many thanks to @RolandIllig for the helpful comments.


Safely trash instead of rm This script is called trash and it goes on the path.

Usage: trash [OPTIONS] [TRASH_FILES]...

How clean/readable do you think this is? Is Python the right tool for the job?

#!/usr/bin/env python3
import os
import sys
import shutil
import itertools
from pathlib import Path
import click

first = next

def candidate_names(path):
    '''unused_names('f.txt') yields f.txt, f.txt_1, f.txt_2, ...'''
    yield path
    for i in itertools.count(1):
        yield path.with_name('{path.name}_{i}'.format(**locals()))

default_trash_dir = '~/.trash'

@click.command()
@click.argument('trash_files', nargs=-1)
@click.option('--trash-dir', default='', metavar='<trash_dir>', help=
'''destination for trash. Defaults to ${{TRASH_DIR}}, and then '{}'.'''
              .format(default_trash_dir))
def trash(trash_files, trash_dir):
    '''Safely trash the given files by moving them into a trash_dir.

Files are stored by their path relative to either ~ or / (preferring ~
if possible)

If an existing item with the same name is already in the trash, I
add _1, _2, ... to the end of the filename.

Metadata is copied; See Python documentation on shutil.copy2 for
details.

    \b
    $ touch a
    $ mkdir b
    $ touch b/c
    $ touch /tmp/d
    $ trash a b/c /tmp/d
    /home/sam/subdir/a -> /home/sam/.trash/subdir/a
    /home/sam/subdir/b/c -> /home/sam/.trash/subdir/b/c
    /tmp/d -> /home/sam/.trash/_root/tmp/d
    $ touch a
    $ trash a
    /home/sam/subdir/a -> /home/sam/.trash/subdir/a_1

    '''

    # get/make trashdir
    if not trash_dir:
        # empty-string and non-existence are treated the same way
        trash_dir = os.environ.get('TRASH_DIR', '')
        if not trash_dir:
            trash_dir = default_trash_dir
    trash_path = Path(trash_dir).expanduser()
    trash_path.mkdir(exist_ok=True, parents=True)

    for trash_file_ in trash_files:
        trash_file = Path(trash_file_)
        if not trash_file.exists():
            print("{}: not found".format(trash_file))
            sys.exit(1)

        # get absolute path
        trash_file = Path(trash_file_).resolve()

        # get a destination which does not already exist
        try:
            trash_dest_candidate = trash_path / trash_file.relative_to(Path.home())
        except ValueError:
            trash_dest_candidate = trash_path / '_root' / trash_file.relative_to('/')

        trash_dest = first(filter(lambda path: not path.exists(),
                                 candidate_names(trash_dest_candidate)))

        trash_dest.parent.mkdir(exist_ok=True, parents=True)
        print('{} -> {}'.format(trash_file, trash_dest))

        try:
            shutil.move(trash_file, trash_dest)
        except Exception as e:
            print(str(e))
            sys.exit(2)

if __name__ == '__main__':
    trash()

first = next

That needlessly pollutes the top-level namespace. Please bury that within def trash. Also, the first() call is odd -- it would be more idiomatic to simply call next() when obtaining first element.

    '''unused_names('f.txt') yields f.txt, f.txt_1, f.txt_2, ...'''

That's a code defect. The comment meant to refer to candidate_names().

You might choose to shift the responsibility, to only yield unused names, but the current code is fine, too.

    yield path.with_name('{path.name}_{i}'.format(**locals()))

Passing in **locals() isn't the most readable way. Please just phrase it as

    yield path.with_name(f'{path.name}_{i}')

Docstring:

Metadata is copied; See Python documentation on shutil.copy2 for details.

Kudos for delegating to official docs rather than copying a sentence from them. But it's probably better to point to move(), which itself points to copy2().

\b

Not sure what you meant by \b.

Maybe you don't need the metavar?

The trash_files can sometimes have long directory paths -- consider using SHA1 to compress the directory down to a short unique string.

if not trash_dir:

I don't understand why the click default wasn't set to os.environ.get('TRASH_DIR', '').

You have three expressions you want to probe here, call them d1, d2, d3. Rather than if / then, consider using or:

trash_dir = d1 or d2 or d3

Consider phrasing this

for trash_file_ in trash_files:

as

for trash_file in map(Path, trash_files):

I like the business of finding trash_dest_candidate relative to $HOME or /, whichever works. This could easily be a list of (more than 2) dirs.

    try:
        shutil.move(trash_file, trash_dest)
    except Exception as e:
        print(str(e))
        sys.exit(2)

Catching all exceptions doesn't seem to help you here -- might be better to not catch anything and live with the stack trace. The str() is superfluous. Consider calling exit() with errno from e, rather than constant 2. It appears to me that Ctrl-C is disabled during cross-filesystem file copies.

Overall, it looks like pretty good code that offers a nice service of "collapsing" filenames while avoiding collisions. Consider appending timestamp + pathname to a log file, and automatically pruning month-old files at end of each run, so trash does not grow without bound.


Checking whether a user is on the donors list I was wondering if I could please get some justification if the PHP code I wrote using Mysqli prepared statements would be able to withstand SQL injection attacks.

I have one field that gets a value from a form. Code works 100% fine, just want to make sure it is secure against SQL injection.

$steaminput = filter_input(INPUT_POST, 'steamid_user', FILTER_SANITIZE_STRING);
$conn = mysqli_connect(host, user, pass, table);
$sql = "SELECT username, activated, expiration_date FROM donors WHERE steam_id=?";

$stmt = mysqli_prepare($conn, $sql);

mysqli_stmt_bind_param($stmt, "s", $steaminput);

mysqli_stmt_execute($stmt);

mysqli_stmt_bind_result($stmt, $username_res, $activated_res, $expiration_res);

  while(mysqli_stmt_fetch($stmt)){
    $data = array('username'=>$username_res, 'activated'=>$activated_res, 'expiration_date'=>$expiration_res);
  }

mysqli_stmt_close($stmt);
mysqli_close($conn);
//I use array information from retreival here

The final parameter here is misnamed:

$conn = mysqli_connect(host, user, pass, table);

It denotes name of MySQL database (e.g. "test") rather than name of a table (e.g. "test.donors" or "donors.donors").

The _res suffix is superfluous and may be safely elided:

mysqli_stmt_bind_result($stmt, $username_res, $activated_res, $expiration_res);

  want to make sure it is secure against SQL injection.

Yes, you are using the API as recommended, in secure fashion.


Counting newlines in a file I've started to go through Node tutorials on nodeschool.io, and the second assignment was to write a program that will count the number of newlines in a file and print it out. My solution looks like this

var fs = require('fs');

var file = process.argv[2];
var buff = fs.readFileSync(file);
var str = buff.toString();
var str_arr = str.split('\n');
var newline_length = str_arr.length-1;

console.log(newline_length);

It's correct, and works. Theirs is

var fs = require('fs')

var contents = fs.readFileSync(process.argv[2])
var lines = contents.toString().split('\n').length - 1
console.log(lines)

// note you can avoid the .toString() by passing 'utf8' as the
// second argument to readFileSync, then you'll get a String!
//
// fs.readFileSync(process.argv[2], 'utf8').split('\n').length - 1

While both are correct, mine is longer (more lines used because I declared variables at every step).

I am wondering, am I wrong in declaring the variables for every step and then working on the code as I go? I mean, it does take more space, but I have the feeling that it's a bit clearer what I'm doing, unlike theirs. Or am I mistaken and haven't really gotten to that level where I can safely write everything in a one line like they did?

Also am I using memory space by declaring too many variables?

This task is rather trivial, and mostly the code I've written so far is for web elements (carousels and whatnot), so I'm used to writing mostly jQuery code instead of pure JavaScript (declaring variables used in events and such). Does this have any impact in real life apps?

The two versions execute exactly the same steps,
the only difference is the number of intermediary local variables.

Which version is more clear can be a matter of opinion.
Beginners may find your verbose version more clear,
more experienced programmers may find it tedious.

The good thing about the second version is that the local variables mark bigger logical steps that are quite intuitive. With one weakness, the meaning of the lines variable is not immediately clear. If you say "lines", it might mean a collection, or the count of lines.
But actually it's neither, it's the number of newline characters.

By contrast, your version marks the steps in too much detail,
for example it's not particularly interesting that readFileSync(file) doesn't return a String and it needs to be converted.
That step could be collapsed, as it is a too specific implementation detail,
not something "logical".

I think the best of both worlds would be more like this:

var fs = require('fs');

var path = process.argv[2];
var text = fs.readFileSync(path).toString();
var lines = text.split('\n');
var newlines_count = lines.length - 1;
console.log(newlines_count);

Each step marks a bigger logical step, with slightly more natural variable names.


Shoping cart code using a loop to print multiple times This code belongs to a shopping cart which I want to print multiple times with the help of a loop.

There are 9 SQL queries and I want all these queries to run at a time with the below table every time the SQL query changes, but the below table remains the same.

          //SQL query define in array
  $query = array(
  "SELECT  * FROM products   WHERE   rowno=1
   and   colno=1 and pageno=1 and  productLine like '$cat'",
   "SELECT  *  FROM   products  WHERE  rowno=1  and  colno=2
   and  pageno=1   and   productLine   like   '$cat'",
   "SELECT *  FROM  products WHERE rowno=1 and colno=3
    and pageno=1 and productLine like '$cat'",
   "SELECT * FROM products  WHERE rowno=2 and colno=1 and pageno=1
   and productLine like '$cat'",
  "SELECT * FROM products WHERE rowno=2 and colno=2
   and pageno=1 and productLine like '$cat'",
   "SELECT * FROM products WHERE rowno=2 and colno=3
   and pageno=1 and productLine like '$cat'",
   "SELECT * FROM products WHERE rowno=3 and colno=1
    and pageno=1 and productLine like '$cat'",
   "SELECT * FROM products WHERE rowno=3 and colno=2
    and pageno=1 and productLine like '$cat'",
    "SELECT * FROM products WHERE rowno=3 and colno=3
    and pageno=1 and productLine like '$cat'");

 foreach($query as  $a)
 {

$cmd=mysql_query($query,$conn);  // con query
$row=mysql_fetch_row($cmd);    // fetch the table in database
echo
  "

  $row[1]	      // table row

  RS $row[7]&nbsp;	         // this table same in  loop but sql change

  $row[8]

   $row[5]

 Details

";
  } // end loop

Security

It's unclear where $cat comes from, but if it can possibly be user supplied (eg when it comes from a GET/POST request, or when it comes from the database where it might have been placed by a user), you should use prepared statements to prevent SQL injection.

You should also not be using mysql_* as it's deprecated.

Also, you should always prevent XSS when echoing data that might be user supplied.

Queries in array

Your approach of using an array to handle the 9 queries whose results should be handled the same is ok, but it would be better if you extracted your foreach loop into a separate function and then called that function with the queries (or better yet their result). Doing it that way would increase readability and reusability of your code.

Reduce amount of queries

You could remove the amount of queries you use by using or, which should increase the performance of your code. So instead of asking

SELECT * FROM products WHERE rowno=1 and colno=1 and pageno=1 and productLine like '$cat'";
SELECT * FROM products WHERE rowno=1 and colno=2 and pageno=1 and productLine like '$cat'";

You could just ask

SELECT * FROM products WHERE ((rowno=1 and colno=2 and pageno=1) or (rowno=1 and colno=2 and pageno=1)) and productLine like '$cat'";

This could be further simplified, and the other queries could be added as well, but honestly, I'm not quite sure if it wouldn't be easier to change your database schema. I'm not quite sure how it currently works though.

Misc

all your SQL keywords should be all uppercase to increase readability, not just some.
your comments all just reflect what the code just told me, so you can just remove them.
use proper indentation to increase readability.
select the concrete fields that you want, don't just select *.
move style information in it's own css file.
you forgot the opening and closing table tags.
why use like? it seem you want equality?

With some of these changes applied, your code might loop something like this:

$dbh = new PDO([...]);
printShoppingCart(getByCat($pdo, $cat, "SELECT var1, var2, var3, var4, var5 FROM products WHERE rowno=1 AND colno=1 AND pageno=1 AND productLine=:line"));
[...]

function getByCat($pdo, $ca, $query) {
    $stmt = $dbh->prepare($query);
    $stmt->bindParam(':line', $cat);
    return $stmt->fetch();
}

function printShoppingCart($vars) {
    echo "

        " . htmlspecialchars($vars[0], ENT_QUOTES, 'UTF-8') . "

            RS " . htmlspecialchars($vars[2], ENT_QUOTES, 'UTF-8') . "&nbsp;

        " . htmlspecialchars($vars[3], ENT_QUOTES, 'UTF-8') . "

            " . htmlspecialchars($vars[4], ENT_QUOTES, 'UTF-8') . "

        Details

";
}

There are more improvements that can be made, but I think this would be a good start.


First drafts of Ultima-like adventure code A friend and I watched most of the thenewboston syntax videos for Python 3 and spent 2 hours attempting to piece together our first program / generic text adventure. First experience with coding.

I'm looking for feedback on silly code and poor conventions.

Pastebin

class Erdrick():
    exp = 0
    level = 1
    levelupthresholds = [10, 30, 60, 100]
    #some sort of xp system here    #where we left off

    equipment = {'Copper Sword': 3, 'Leather Tunic': 3}             # currently equipped
    weapon = equipment['Copper Sword']
    armor = equipment['Leather Tunic']
    damage = 2 + 1 * (level - 1) + weapon
    health = 20 + 10 * (level - 1) + armor

class Enemy():
    def __init__(self, name, health, damage, exp):
        self.name = name
        self.health = health
        self.damage = damage
        self.exp = exp

hero = Erdrick()
slime1 = Enemy('Slime', 7, 2, 2)

Kind of stuck on integrating character levels, xp, and level up thresholds.

The math for damage and health is probably stupid

Doubtful that our implementation of classes and creating objects (hero/enemies) is better than bad

print(slime1.name, str(slime1.health))
print("You encounter a %s!,"
      "\n\t[%s] Health: %s\n" % (slime1.name, slime1.name, str(slime1.health)))

while hero.health > 0 and slime1.health > 0:
    print("\tYou deal %s damage!" % hero.damage)
    slime1.health -= hero.damage
    if slime1.health <= 0:
        print("\tSlime is defeated!!")
    else:
        print("\t[Slime] Health: %s" % slime1.health)
        print("\tSlime deals %s damage!" % slime1.damage)
        hero.health -= slime1.damage
        if hero.health <= 0:
            print("YOU DIED")
        else:
            print("\t[Hero] Health: %s" % hero.health)

Combat will be revised with prompts and options. It's currently (poorly?) automated just to have things work.

Hero and Enemy health will be formatted to print together in more of a typical horizontal information layout you'd find in a traditional RPG.

Actually, I agree with Quentin. For a first effort, your code looks pretty good. You have a hero class called Erdrick and an Enemy class instantiated as a Slime.

A class structure like what I show below would allow you more flexibility with less repetition. See what you think...

You might want to work out some way for specialized weapons to increase the damage a hero deals and for armor to reduce the damage suffered.

class Character():
    def __init__(self, name, health, damage,
                 level=None, experience=None, equipment=None):
        self.name = name
        self.health = health
        self.damage = damage
        self.level = level or 1
        self.experience = experience or 0
        self.equipment = equipment or {}

    def strikes(self, other):
        other.health -= self.damage

    def alive(self):
        return 0 < self.health

class Hero(Character):
    level_thresholds = {10: 2, 30: 3, 60: 4, 100: 5}
    def grows(self):
        self.experience += 1
        curlev = self.level
        for thresh in level_thresholds:
            if thresh <= self.experience and self.level < level_thresholds[thresh]:
                self.level = level_thresholds[thresh]

        if curlev < self.level:
            print("{0} leveled up!".format(self.name))

class Monster(Character):
    ...

erdrick = Hero('Erdrick', 100, 2, equipment = {'Copper Sword': 3,
                                               'Leather Tunic': 3})
slime = Monster('Slimebot', 50, 3)
while erdrick.alive() and slime.alive():
    erdrick.strikes(slime)
    slime.strikes(erdrick)

if erdrick.alive():
    print("{0} has defeated the {1}!".format(erdrick.name, slime.name))
    erdrick.grows()
else:
    print("The {0} has killed {1}... );".format(slime.name, erdrick.name)


Updating the presentation of a credit card field as the card type becomes known The goal here is to, add a credit card image, and remove all other credit card images, of a certain input field.
I had a even larger code, with a lot of addClass() and removeClass()... then, I shrink it until I have this.
Still, I feel it's still a huge block of code.

The point here is to short things up, but NOT until it's hard to understand what's going on.

if (event.isValid) {

    if (event.card.type == 'visa') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-visa'));

    } else if (event.card.type == 'master-card') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-master-card'));

    } else if (event.card.type == 'e-american-express') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-american-express'));

    } else if (event.card.type == 'diners-club') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-diners-club'));

    } else if (event.card.type == 'discover') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-discover'));

    } else if (event.card.type == 'jcb') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-jcb'));

    } else if (event.card.type == 'unionpay') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-unionpay'));

    } else if (event.card.type == 'maestro') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-maestro'));
    }

} else {

    $("#card-number").removeClass(CREDIT_CARD_LIST);
}

Create a map from card.type to what class it should be:

var cardTypeToClass = {  'visa': 'e-visa',
                         'master-card': 'e-master-card',
                         //...
                       };

This creates an associative array so if you give it a key value then you can get out the associated value.

in your case that is done like:

if (event.isValid) {
    $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, cardTypeToClass[event.card.type]));
} else {
    $("#card-number").removeClass(CREDIT_CARD_LIST);
}

This concentrates the mapping into a single section and separates out what the code actually does.


Pretty print an object - JavaScript I've written a function to prettify a json object, as if it were using JSON.stringify.

I'm only focused on strings and other objects, nothing else.

Is there a better way of doing this? perhaps using a data structure?

const exampleJson = {"name":"Jon","facts":{"car":"Ford","address":{"city":"New York"},"watch":"Casio","other": {}}};

function prettyPrint(data) {
  let tabbify = n => Array(n).fill(' ').join('');

  let traverse = (data, tab = 1) => {
    let output = "",
        open   = `{\n`,
        close  = `\n${tabbify(tab - 1)}}`;

    output += open;

    Object.entries(data).forEach(([key, val], i, {length}) => {
      if (typeof val === 'string') {
        output += `${tabbify(tab)} "${key}": "${val}"`;
      } else if (typeof val === 'object') {
        if (Object.keys(val).length > 0) {
          output += `,\n${tabbify(tab)} "${key}": ${traverse(val, tab+2)}${i === length - 1 ? '' : ',\n'}`;
        } else {
          output += `,\n${tabbify(tab)} "${key}": {}`;
        }
      }
    })

    output += close;
    return output;
  }

  return traverse(data);
}

Output

{
    "name": "Jon",
    "facts": {
        "car": "Ford",
        "address": {
            "city": "New York"
        },
        "watch": "Casio",
        "other": {}
    }
}

I am prejudiced against variables named data.

You need to escape literal ", \, and \n characters in quoted strings.  It's good form to do tabs too.

Using a dispatch table for types (instead of if/else) makes it easy to handle all the types.

Instead of passing tab up and down the call stack, wrap the calls in a function that indents a block of text.  Nested traverse will be nested in indent too and get indented the right number of times.

Instead of checking whether you're at the end of a delimited list (to decide whether to add a delimiter), use arr.join(delimiter). It puts delimiters on inner boundaries only.

Array(n).fill(' ').join(''); is the same as ' '.repeat(n).

It would be nice to omit the quotes on property names that don't need them.  Doing this perfectly happens to be really laborious—the regex is 11 kilobytes long!!  It's manageable if we limit the exceptions to 7-bit ASCII, which is probably a good idea anyway.

function prettyPrint(obj) {
    const stringify = {
        "undefined": x => "undefined",
        "boolean":   x => x.toString(),
        "number":    x => x,
        "string":    x => enquote(x),
        "object":    x => traverse(x),
        "function":  x => x.toString(),
        "symbol":    x => x.toString()
    },
    indent = s => s.replace(/^/mg, "  "),
    keywords = `do if in for let new try var case else enum eval null this true
            void with await break catch class const false super throw while
            yield delete export import public return static switch typeof
            default extends finally package private continue debugger
            function arguments interface protected implements instanceof`
       .split(/\s+/)
       .reduce( (all, kw) => (all[kw]=true) && all, {} ),
    keyify = s => ( !(s in keywords) && /^[$A-Z_a-z][$\w]*$/.test(s) ? s : enquote(s) ) + ": ",
    enquote = s => s.replace(/([\\"])/g, '\\$1').replace(/\n/g,"\\n").replace(/\t/g,"\\t").replace(/^|$/g,'"'),
    traverse = obj =>  [
           `{`,
            indent( Object.keys(obj)
                    .map( k => indent( keyify(k) + stringify[ typeof obj[k] ](obj[k]) ) )
                    .join(",\n")
                    ),
            `}`
        ]
        .filter( s => /\S/.test(s) )
        .join("\n")
        .replace(/^{\s*\}$/,"{}");
    return traverse(obj);
}

console.log(prettyPrint(
    {
        "name":"Jon",
        "facts":{
            "car":"Ford",
            "address":{
               "city":"New York"
            },
            "watch":"Casio",
            "other": {
                "true":false,
                blargh:undefined,
                "111number":1e5,
                method:function(x){x++}
            }
        }
    }
));


LeetCode 3Sum Java Solution I have this Java solution to LeetCode's 3Sum problem.
I would be really grateful if I could get feedback on correctness, scalability, efficiency, coding best practices, and OOP design.
I'm relatively new to Java.
class Solution
{
    public List<List<Integer>> threeSum(int[] nums)
    {
        List l1 = new ArrayList<ArrayList>();

        for (int idx = 0; idx < nums.length; idx++)
        {
            if (idx > 0 && nums[idx] == nums[idx - 1]) continue;

            HashMap<Integer, Integer> hash = new HashMap<>();

            for (int idxInner = idx + 1; idxInner < nums.length; idxInner++)
            {
                if (
                     hash.get(nums[idxInner]) != null &&
                     nums[idxInner]+hash.get(nums[idxInner]) == -1*nums[idx]
                   )
                {
                    List l2 = new ArrayList<Integer>();

                    addInList(l2, nums[idx]);
                    addInList(l2, hash.get(nums[idxInner]));
                    addInList(l2, nums[idxInner]);

                    l1.add(l2);
                }
                else
                {
                    hash.put(-1*(nums[idx] + nums[idxInner]), nums[idxInner]);
                }
            }
        }

        removeDuplicates(l1);

        return l1;
    }

    public void addInList(List<Integer> list, int num)
    {
        if (list == null) return;

        int lSize = list.size();

        if (lSize == 0)
        {
            list.add(num);
            return;
        }

        int idx = 0;
        while (idx < lSize && list.get(idx) < num)
        {
            idx++;
        }

        if (idx < lSize) list.add(idx, num);
        else list.add(num);

        return;
    }

    public void removeDuplicates(List<ArrayList<Integer>> lists)
    {
        if(lists == null) return;

        for(int outer = 0; outer < lists.size(); outer++)
        {
            for(int inner = outer + 1; inner < lists.size(); inner++)
            {

                if (areDuplicates(lists.get(outer), lists.get(inner)))
                {

                    lists.remove(inner);
                    inner--;
                }
            }
        }
    }

    public boolean areDuplicates(ArrayList<Integer> first, ArrayList<Integer> second)
    {
        if (first == null && first == second) return true;
        if (first == null || second == null) return false;

        int fSize = first.size();
        int sSize = second.size();

        if (fSize != sSize) return false;

        for(int idx = 0; idx < fSize; idx++)
        {
            if (first.get(idx) != second.get(idx)) return false;
        }

        return true;
    }
}

I get a timeout error on LeetCode if I try to run this.

This is a nice start, but there's some stylistic stuff that could be improved.
Generally it's a good idea to name variables for what their purpose is. Names like l1 are difficult to understand. This varies depending on the language, but in Java people tend to be explicit with their variable naming, preferring descriptive names over concise ones.
When declaring a data structure, convention in Java is to declare the type as the interface.
public boolean areDuplicates(ArrayList<Integer> first, ArrayList<Integer> second)

is more commonly written as
public boolean areDuplicates(List<Integer> first, List<Integer> second)

This is because List defines a set of behaviours that you want to have, and ArrayList defines how they are implemented. If someone has made a data structure with the exact same behaviour as an ArrayList, then you want your methods to support that data structure as well.
The format of the code in quite unusual for Java. I'd run your code through an autoformatter, which deals with all that stuff for you. Personally I use Eclipse's default one, but there's loads of choice out there.
This isn't a complete review, and it's mainly my opinion, so feel free to include or discard this advice.