Summarize a directory with PowerShell I'm trying to output aggregate information about a directory tree (file extension plus accumulative count and size per file type) with PowerShell.

This is as far as I've got.

gci -r -ea Si `
| group { if ($_.PSIsContainer) {""} else {$_.Extension} } `
| select Name, Count, @{n="Measure"; e={$_.Group | measure Length -Sum -Average} }`
| sort Count -desc `
| ft Name, Count, @{n="SizeMB"; e={"{0:N1}" -f ($_.Measure.Sum / 1MB)}; a="right"} -Auto

which gives me

Name  Count SizeMB
----  ----- ------
.jar   2489  262,2
       1592    1,2
.vim   1147    7,1
.xml   1087    9,6
.dll    803  657,3
.png    762    9,1
.js     380    1,9
.txt    360   14,1
.py     305    1,4
.gyp    266    0,3
.exe    262  198,3
.mui    178   47,1
.c      157    0,1
.md     137    0,3
.html   132    1,1
.tmpl   113    0,1
[...]

Not too bad for my first stab at PowerShell, but it feels unwieldy and I'm wondering if this could be made more elegant.

You are running your select twice and saving an unused Average from your Select-Object

gci -r -ea Si `
| group { if ($_.PSIsContainer) {""} else {$_.Extension} } `
| sort Count -desc `
| ft Name, Count, @{n="SizeMB"; e={"{0:N1}" -f (($_.Group | measure Length -Sum).Sum / 1MB)};a="right"} -Auto

You have to move the Group Measurement to the FT line from the Select.

If you want this a little more readable, you can pull the formatting HashTable out of the pipeline:

$sizeMB = @{n="SizeMB";
            e={"{0:N1}" -f (($_.Group | measure Length -Sum).Sum / 1MB)};
            a="right"}

gci -r -ea Si `
| group { if ($_.PSIsContainer) {""} else {$_.Extension} } `
| sort Count -desc `
| ft Name, Count, $sizeMB  -Auto


NSAssert or NSLog in defaults switch case I am wondering if I should use NSLog(@"Switch out of range") or NSAssert(FALSE,@"Switch out of range"); in the following example:

typedef enum {
    kSectionFavoriteCarpark,
    kSectionPromotions,
    SectionsEnumCount
} SectionsEnum;

- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section {
    switch (section) {
        case kSectionFavoriteCarpark:
            return self.locationFavoritesKeyArr.count;
        case kSectionPromotions:
            return self.promotionCarparks.count;
        default:
            DLog(@"index out of switch range");
            return 0;
    }
}

- (NSInteger)numberOfSectionsInTableView:(UITableView *)tableView {
    if (self.promotionCarparks.count > 0) {
        return SectionsEnumCount;//one for user defined positions one for favorite parkings
    }
    else {
        return SectionsEnumCount-1;
    }
}

Assertions are for declaring facts that you know to be true, not for conditions that you hope to be true.  If this method is only ever called by your own code, then it makes sense to assert.  On the other hand, if this is a public method, an assertion would not be an appropriate way to perform what is essentially parameter validation.


Entering information about a business and finding matches I'm writing a Java Servlet application where the users must enter information about a business.  I'd like to prevent as many duplicate entries in the MySQL database as possible and I'd appreciate people's comments on the logic of the process.

When the user fills out the form and submits it, the business name and address are sent to a function which splits the address string, builds a query and queries the database for like-entries.  If there's no result or more than one result, the user's input gets cleaned up and stored.  If there's one result, the user's data is replaced by the existing business information.

private static List<Business> getBusinesses(String name, String address) throws ClassNotFoundException, SQLException {
    List<Business> businessList = new ArrayList<>();

    Connection conn = getConnection();

    //split the address for the where clause in the query
    String[] add = address.split(" ");

    //get rid of punctuation, h/t http://stackoverflow.com/users/3053630/jeff-ward
    name.replaceAll("[^a-zA-Z0-9\\s]", "").replaceAll("\\s+", " ");

    //build query
    String query = "select id from business where ";
    for (String add1 : add) {
        query += "streetAddress like '%" + add1 + "%' and ";
    }

    //find at least a partial match to the business name
    if(name.length()>5){
        query += "businessName like '%" + name.substring(0, 4) + "%';";
    }else{
        query += "businessName like '%" + name + "%';";
    }

    Statement s = conn.createStatement();
    ResultSet rs = s.executeQuery(query);

    while(rs.next()){
        //build list of businesses
        businessList.add(getBusiness(rs.getInt("id")));
    }

    rs.close();
    s.close();
    conn.close();

    return businessList;
}

I do understand that I may get some incorrect answers (for example, "Dominos Pizza" at 55 Marbach Road will match with "Dominos Pizza" at 2557 Marbach Road), but I plan on doing further evaluation/matching before finalizing.

From a security perspective, you are vulnerable to malicious SQL injection, it only takes 1 malicious user..

Consider using prepared statements to plug this.

Furthermore, this hit my eye:

//find at least a partial match to the business name
if(name.length()>5){
    query += "businessName like '%" + name.substring(0, 4) + "%';";
}else{
    query += "businessName like '%" + name + "%';";
}

Since calling substring is a cheap operation, and it will return the value of name if the length is less than 5, I would go for simply

//find at least a partial match (first five characters) to the business name
query += "businessName like '%" + name.substring(0, 4) + "%';";

Also, 4 is a magical constant here, you could have named that constant and defined it.

//find at least a partial match to the business name
query += "businessName like '%" + name.substring(0, PARTIAL_MATCH_LENGTH) + "%';";

Also, in my mind, Java is for industrial strength with fullout names, so I would name some variables differently:

add -> addressParts
add1 -> addressPart

Finally, should getBusinesses know how to clean up name? Are you doing this in every single querying method that receives name ? There should be a class or method that knows how to clean name which should then be used by getBusinesses.


Pad a function with random bytes I'm writing a function to append 5-10 bytes (count is random) at the beginning of a byte array, and 5-10 random bytes at the end:

func padWithRandomBytes(b []byte) []byte {
    startBytes := make([]byte, 10-rand.Intn(5))
    endBytes := make([]byte, 10-rand.Intn(5))
    newSlice := make([]byte, len(startBytes)+len(b)+len(endBytes))
    copy(newSlice[:len(startBytes)], startBytes)
    copy(newSlice[len(startBytes):len(startBytes)+len(b)], b)
    copy(newSlice[len(startBytes)+len(b):], endBytes)
    return newSlice
}

This feels pretty inefficient. Is there a more intuitive way to write this in Go?

For example,

package main

import (
    "fmt"
    "math/rand"
    "time"
)

func padWithRandomBytes(b []byte) []byte {
    s := 5 + rand.Intn(5+1)
    e := 5 + rand.Intn(5+1)
    r := make([]byte, s+len(b)+e)
    copy(r[s:], b)
    return r
}

func main() {
    rand.Seed(time.Now().UnixNano())
    b := []byte{1, 2, 3}
    fmt.Println(len(b), b)
    r := padWithRandomBytes(b)
    fmt.Println(len(r), r)
}

Output (random):

3 [1 2 3]
20 [0 0 0 0 0 0 0 0 1 2 3 0 0 0 0 0 0 0 0 0]

3 [1 2 3]
15 [0 0 0 0 0 0 1 2 3 0 0 0 0 0 0]


Member restricted game with API My website is poorly coded.

The structure is pretty simple:

A RewriteRule redirects /.... to /index.php?page=$1
mypages/page.php contains the page content and actions (model controller and view glued together...)
index.php wrap the demanded page into a theme (basically include header and footer around it)

The site is a complex MMORPG site (~40 differents pages). I need to convert each page to a view in my iPhone/Android app (probably made in JS+HTML with PhoneGap). So I guess I'll have to create an API and separate the model/controller/view on each page.

I don't know how to manage the connection, but I'm sure I'm doing it in a bad way (see code below).

My controller looks like this:

<?php
// Database connection, get $_GET("page") ...
if (file_exists($page)) {
    if(isMemberRestricted($page)) {
    $loadUserInf = TRUE;
        if(!isValid($_SESSION['usern'], $_SESSION['passw'], $db)) {
            if(!isValid($_COOKIE['cookie_password'] , $_COOKIE['cookie_password'], $db)) {
                if(!isValid($_POST['usern'], $_POST['passw'], $db)) {
                       if(isValid($_POST['usern'], $_POST['passw'], $dbt)) {
                          $page = CONTROLLER_PATH . "/desarchivage.php";
                       }
                       else {
                         $page = CONTROLLER_PATH . "/notConnected.php";
                       }
                       $loadUserInf = FALSE;
                 }
            }
        }
        else { // not yet connected
            if($loadUserInf) // but connexion accepted
                connexion ();
        }
    }
    if($loadUserInf) {
        require ('userInf.php'); // Loads informations about the connected user
        pageRoutine(); // Stuff to do on each member restricted page
    }
} else {
   $page = CONTROLLER_PATH . "/404.php";
}

require ($header);
require ($page);
require ($footer);
?>

I really don't know where to start. I've read about MVC/API.  I understood the theory, but I don't know where to begin with an existing project.

The first 'problem' I notice is what I call a roocky mistake (been there, done that) is the way you route a request.

If a person requests ?page=mypage.php you check or that file exists if so, you include it.
this means that your filesystem is hardcoded with your routing. Everytime you change a file-name, your routes also change. Also, some file-systems are case-sensitive (oh oh!)

A better would be to have a Router script that handles routing, it recieves a request, and it simply calls the correct method. Routes can be stored in a database table, local file or even written in PHP (as in laravel)

Another thing that really bugs me is all the $_ variables in your code. A better thing to do is to wrap them somehow (e.g. a Class or a set of functions). These functions would then have some validation in them. E.g. CSRF protection when _POST data is given, or instead of using the default php session handler, have your own that stores all session data in a database, ...

You talk about MVC and that your php files are always a Model, controller and view in one. So you didn't use MVC, you simply started coding everything together. And that is simply not how you should begin.

When designing an API, make your own (web)-inteface use the same API as your smartphone-apps. This way, you only have to work on one single API instead of having an API for apps and a completly different one for the web.


Random index generator in C++ I have a training dataset from which I want to draw samples in a random fashion such that all samples are used before getting randomly shuffled again. For this reason I implemented a simple random index generator.
For a dataset of 10 samples, the output looks something like this:
0 1 2 3 4 5 6 7 8 9
4 3 7 8 0 5 2 1 6 9
0 5 7 8 4 3 9 2 1 6
5 7 6 3 8 4 2 0 1 9
4 6 0 2 8 1 3 9 5 7
4 0 5 1 7 9 6 2 8 3
3 8 5 6 1 7 2 4 0 9
0 4 6 2 9 5 8 3 1 7
1 3 6 8 2 7 5 9 0 4
5 1 7 9 8 0 6 4 2 3

I would appreciate advice especially in the following areas:

Code style (readability, naming conventions, etc...)
Class design
Efficiency (how to avoid unnecessary complexity)
Reinventing the wheel (does the STL offer functionality that I should use?)
Are there perhaps bugs that I am not seeing right now?

Please be as hard as possible with this implementation and give me constructive feedback.
main.cpp
#include <iostream>
#include "random_index.hpp"

int main() {
    unsigned int size = 10;
    RandomIndex rand_idx(size);

    unsigned int n = 0;
    for (unsigned int i=0; i<100; ++i, ++n) {
        std::cout << rand_idx.get_index() << ' ';
        if ((n+1) % size == 0) {
            std::cout << '\n';
        }
    }

    std::cout << '\n';
}

random_index.cpp
#include "random_index.hpp"

RandomIndex::RandomIndex(unsigned int _size) {
    size = _size;
    index.resize(_size, 0);
    std::iota(index.begin(), index.end(), 0);
}

unsigned int RandomIndex::get_index() {
    if (counter < size) {
        return index[counter++];
    } else {
        counter = 0;
        std::random_shuffle(index.begin(), index.end());
        return index[counter++];
    }
}

random_index.hpp
#ifndef RANDOM_INDEX_H
#define RANDOM_INDEX_H

#include <vector>
#include <numeric>
#include <algorithm>

class RandomIndex {

    public:
        RandomIndex(unsigned int _size);
        unsigned int get_index();

    private:
        unsigned int size;
        unsigned int counter = 0;
        std::vector<unsigned int> index;

};

#endif

I compiled the code using the following command:
g++ -O -Wall main.cpp random_index.cpp

Overview
Your code produces the exact same set of samples each time you random_shuffle. So the output is always
0 1 2 3 4 5 6 7 8 9
4 3 7 8 0 5 2 1 6 9
0 5 7 8 4 3 9 2 1 6
5 7 6 3 8 4 2 0 1 9
4 6 0 2 8 1 3 9 5 7
4 0 5 1 7 9 6 2 8 3
3 8 5 6 1 7 2 4 0 9
0 4 6 2 9 5 8 3 1 7
1 3 6 8 2 7 5 9 0 4
5 1 7 9 8 0 6 4 2 3

You should initialize the seed. You do that before you call random_shuffle. If you do not do that it will give the same result each time since it relies on the seed.
You can fix that by calling std::srand in the beginning.
int main() {
    std::srand(std::time(0));
}

Now you can see the difference in the output
0 1 2 3 4 5 6 7 8 9
9 2 8 3 6 4 0 7 1 5
8 9 4 6 7 2 1 5 0 3
4 6 0 2 3 1 5 8 9 7
2 3 0 1 6 8 9 5 7 4
8 9 6 5 2 1 3 7 4 0
9 3 2 8 6 7 5 0 4 1
8 1 2 5 9 6 4 3 0 7
7 1 4 6 8 9 3 0 5 2
3 1 9 4 2 7 6 0 5 8

But the top row still will always be the same. The problem lies here
RandomIndex::RandomIndex(unsigned int _size) {
    size = _size;
    index.resize(_size, 0);
    std::iota(index.begin(), index.end(), 0);
}

iota will always will it starting with 0 and incrementing it will it reaches index.end(). get_index will only shuffle the container after the first row since that's when counter < size evaluates to false.
To fix that you can also shuffle in the beginning, when you construct the vector.

Use std::shuffle
random_shuffle has been deprecated. Use shuffle  with a random number generator so that your code can compile on the later c++ versions.

Avoid using _ as a prefix for your variables
There are some naming conventions you need to follow when it comes to underscores to avoid collisions. Moreover, it just looks ugly. To initialize size in your class constructor you are better off using member initializer lists.
RandomIndex::RandomIndex(unsigned int size)
    : size(size)  {

 //...
}

use an array with a fixed size over vector
In your class size is a value that is never going to change. If you plan on keeping it that way you can, use std::array here with a template so you can have a fixed size. This will avoid all the resizing and heap fiddling resulting in faster execution. std::shuffle will work the same way it does with vector thanks to the way STL is designed.
template < size_t s >
class RandomIndex {
   //...

private:
    std::array < uint32_t, s > index;

};


Count how many times a value is referenced in a grid I've got list of points: List<Point> seedPoints. Each Point contains x and y coordinates pointing to certain double value (let's call it pointValue). I would like to display how many times each pointValue is referenced in seedPoints list. To do so I've created following lambda expression:

seedPoints.stream()
    .collect(Collectors.groupingBy(p ->  pointGrid.get(p.x, p.y)))
    .entrySet()
    .stream()
    .map(entry -> "value:" + entry.getKey() + "   count: " + entry.getValue().size())
    .forEach(System.out::println);

The solution works fine, I get the following output:

value:0.0   count: 1
value:3.0   count: 2
value:4.0   count: 1
value:5.0   count: 4
value:6.0   count: 2

I don't find it very clean however. This solution requires two call to the stream() method (1st and 4th line of code), as collect() method is a terminal operation. Does anybody has any idea how could I improve my lambda expression?

There is no intermediate operation that groups items by key.

But in your example there are some changes that can make code more readable.

Instead of collectiing to Map<Double, List<Point>> you could collect directly to Map<Double, Long>

.collect(groupingBy(p ->  pointGrid.get(p.x, p.y), counting()))

Instead of creating stream and performing output just use forEach on a map

.forEach((value, count) -> System.out.println("value :" + value + " count: " + count);

So together it looks like

seedPoints.stream()
    .collect(groupingBy(p ->  pointGrid.get(p.x, p.y), counting()))
    .forEach((value, count) -> System.out.println("value :" + value + " count: " + count);


Secret Santa pair allocator in JavaScript I have a Secret-Santa gift swap with my family and we always forget who we had last year and never find time to do it so I wanted to create a simple web app to manage the whole thing. What I'm looking for is a critique on my code. Could it use more built-in JavaScript functions? How's my style?

//get people from database
var people = [{name: 'TestName', spouse: 'TestNameSpouse', current: '', past: ['Dad']},
  { name: 'TestNameSpouse', spouse: 'TestName', current: '', past: ['Mom']},
  { name: 'Mom', spouse: 'Dad', current: '', past: ['TestName']},
  {name: 'Dad',spouse: 'Mom',current: '',past: ['SisterInLaw']},
  {name: 'Brother',spouse: 'SisterInLaw',current: '',past: ['TestNameSpouse']},
  {name: 'SisterInLaw',spouse: 'Brother',current: '',past: ['Dad']}
];

const setCurrentToBlank = (people) => people.forEach((person) => person.current = '');
const verifyGiftPartners = (people) => people.every((person) => person.current);

//Knuth shuffle
var shuffle = function shuffle(array) {
  var currentIndex = array.length,
    temporaryValue,
    randomIndex;

  // While there remain elements to shuffle...
  while (0 !== currentIndex) {
    // Pick a remaining element...
    randomIndex = Math.floor(Math.random() * currentIndex);
    currentIndex -= 1;

    // And swap it with the current element.
    temporaryValue = array[currentIndex];
    array[currentIndex] = array[randomIndex];
    array[randomIndex] = temporaryValue;
  }
};
var assignGiftPartners = function(people) {
  var peopleLeftToAssign = people.map(person => person.name);

  people.forEach(function(person) {
    var choices = peopleLeftToAssign.filter(function(personToAssign) {
      return (
        personToAssign != person.name &&
        personToAssign != person.spouse &&
        person.past.indexOf(personToAssign) === -1
      );
    });
    shuffle(choices);
    person.current = choices[0];
    var index = peopleLeftToAssign.indexOf(choices[0]);
    peopleLeftToAssign.splice(index, 1);
  });
};

var allAssigned = false;
var loopCount = 0;
while (!allAssigned) {
  assignGiftPartners(people);
  allAssigned = verifyGiftPartners(people);
  //exit loop if going too long
  if (loopCount > 50) {
    throw 'Something went wrong with the assignment';
  }
  if (!allAssigned) {
    setCurrentToBlank(people);
  }
}

//save choices into database
console.log(people);

Also, I realize that in a year or so we'd have to repeat people, so would it be good to limit the past size to family size - 1?

This is not a review of the coding style and syntax, but of the algorithm used.

Permutations and randomised rule based selection.

A random search for a set of permutations is never a good method, especially if the set of previous results reduce the size of the set of valid permutations for the next set.

Random solution guessing

Your problem is finding one of a set of solutions. There are only a finite set of possible solutions.

Your code uses a random guess method.

Each iteration you randomly guess a possible combination then test its validity. Because the set of valid solutions gets smaller each year, your code needs to make more guesses as the number of valid solutions gets smaller but the set you are guessing in remains the same size.

This method also has been limited (to 50) and thus you could always have a chance of not finding a solution. For the first year there is a relatively small chance of not finding a solution, but this grows quickly. In three years you will need to (on average) run the code 5 times to find a solution.

Also the limit you must surely have seen is that after 4 years there are no more possible solutions, as everyone has given to everyone. This should be obvious from the code as well, but it still tries to guess a solution where there is none.

There is no need for the code to throw an error. The solutions are known and fixed, so there is no need to guess.

Systematic solution computation.

To do this effectively you need to know the set of all valid solutions. Once you have that set you just randomly select one. There is no need to search for valid solutions as you are only selecting from known valid values.

Pre-computation of the set of valid solutions has a limit, as the possible number of solutions quickly grows with the complexity of a solution. For 6 people there are 80 solutions that are valid, for 10 people that is approaching a million. For 20 people you need to start applying some serious optimisation and storage compression to use this method.

Abstraction

If you compute them systematically and hold them as an abstracted data set, e.g. each person is a letter A,B,C,D,E,F that identifies them both as an entry in the data struc and a position in the abstracted data, e.g. "A" is 1st person in a solution string and the first person in the people array. A solution for a particular year is a string e.g. "CDEFAB" which means person A gives to C, person B gives to D, C to E and so on.

If you ignore the previous year the possible solutions are only 80.

Computing permutations.

For your data the following is all valid solutions.

var allValidSolutions = "DCFEBA,DCFEAB,DCEFBA,DCEFAB,ECFADB,ECFABD,ECFBDA,ECFBAD,ECAFDB,ECAFBD,ECBFDA,ECBFAD,FCEADB,FCEABD,FCEBDA,FCEBAD,FCAEDB,FCAEBD,FCBEDA,FCBEAD,CDFEBA,CDFEAB,CDEFBA,CDEFAB,EDFACB,EDFABC,EDFBCA,EDFBAC,EDAFCB,EDAFBC,EDBFCA,EDBFAC,FDEACB,FDEABC,FDEBCA,FDEBAC,FDAECB,FDAEBC,FDBECA,FDBEAC,CEFADB,CEFABD,CEFBDA,CEFBAD,CEAFDB,CEAFBD,CEBFDA,CEBFAD,DEFACB,DEFABC,DEFBCA,DEFBAC,DEAFCB,DEAFBC,DEBFCA,DEBFAC,FEBADC,FEBACD,FEABDC,FEABCD,CFEADB,CFEABD,CFEBDA,CFEBAD,CFAEDB,CFAEBD,CFBEDA,CFBEAD,DFEACB,DFEABC,DFEBCA,DFEBAC,DFAECB,DFAEBC,DFBECA,DFBEAC,EFBADC,EFBACD,EFABDC,EFABCD".split(",");

This was created with the following code. Note this is a brute force method. You should do some research into optimal methods if you need to include more than a dozen people.

function permutations2Of(chars) {
    var i,j;
    const count = chars.length;
    const sets = [];
    for (i = 0; i < count; i ++) {
        for (j = 0; j < count; j ++) {
            if (j !== i) { sets.push(chars[j] + chars[i]) }
        }
    }
    return sets;
}
function isSetValid(set, ofSet) {
    var i = 0;
    while (i < ofSet.length) {
        if (set.indexOf(ofSet[i ++]) === -1) { return false }
    }
    return true;
}
function findUnique(arr, ofSet, result = [], index = 0, current = "") {
    var i;
    const testSet = index === arr.length - 1;
    for (i = 0;  i < arr[index].length; i ++)  {
        const aSet = current + arr[index][i];
        if (testSet) {
            if (isSetValid(aSet, ofSet)) { result.push(aSet) }
        } else {
            result = findUnique(arr, ofSet, result, index + 1, aSet);
        }
    }
    return result;
}
const AB = permutations2Of("CDEF");
const CD = permutations2Of("ABEF");
const EF = permutations2Of("ABCD");
const combos = findUnique([AB, CD, EF], "ABCDEF");

This is based on the smallest unit being a couple (e.g. AB is the first and second person, and they can give only to persons "CDEF")

It calculates all the possible permutations for each couple and then calculates all the possible permutations for all couples.

The  function findUnique uses recursion to create all possible solutions and then calls the function isValidSet to test if the solution should be kept.

The set of solutions should only be calculated once; once computed it can be retrieved from a data store and filtered when there is a need for extra rules to be applied - e.g. last years results.

Random solution without the guesswork.

Now it is very easy to find the first years arrangement.

var secretSantas = combos[Math.random() * combos.length | 0];

And convert to your data:

const baseCode = "A".charCodeAt(0);
secretSantas.split("").forEach((id, index) => {
    const personIndex = id.charCodeAt(0) - baseCode;
    people[index].current = people[personIndex];
});

Additional rules.

The above does not consider the previous year's gifts, but that can be stored as the abstracted string e.g. "DEFGAB". As there are only 4 people for each person to give to there are only 4 years before you are forced to repeat. If you limit the repeat only to 4 years then after the 3rd year everyone will know who is giving to who (if anybody bothered to keep track that is).

So the better rule is to just not repeat two years. You can't give to the person you gave to last year.

Then you can use a filter on the combos array to eliminate any invalid combination due to previous years results.

const lastYearsCombo = "CDEFAB".split("");
const comboFiltered = combos.filter(combo => {
     for (var i = 0; i < lastYearsCombo.length; i++) {
         if (combo[i] === lastYearsCombo[i]) { return false }
     }
     return true;
});

comboFiltered will only have 17 possible combinations as the rest have been eliminated due to the previous year. The random selection is still the same.

var secretSantas = comboFiltered[Math.random() * comboFiltered.length | 0];


Apply discount changes with Strategy pattern I am implementing  strategy pattern. I have defined a strategy as interfaces and concrete classes to implement the strategy. Based on user selection/configuration, the algorithm to apply the discount changes.

  public interface IStrategy
    {
        double calculate(double x, double y);
    }

concrete classes implementing the gold strategy is listed below -

public class clsGoldDiscountStrategy : IStrategy
    {
        public double calculate(double x, double y)
        {
            return (x * y) * 0.8;
        }
    }
}

concrete classes implementing the platinumstrategy is listed below -

public class clsPlatinumDiscountStrategy : IStrategy
    {
        public double calculate(double x, double y)
        {
            return (x * y) * 0.7;
        }
    }

The business logic to apply

public class clsBL
    {
        public double costPrice { get; set; }
        public double qty { get; set; }
        IStrategy _strategy;

        public clsBL(IStrategy strategy)
        {
            _strategy = strategy;
        }

        public double GetfinalPrice(double cp, double qty)
        {
            return _strategy.calculate(cp, qty);
        }

    }

//Main method

static void Main(string[] args)
        {
            Console.WriteLine("Enter the discount Plan (Gold/Platinum)");
            string filter = Console.ReadLine().ToUpper();
            double result = 0;

            if (filter.Length > 0)
            {
                switch (filter)
                {
                    case "GOLD":
                        //Gold
                        clsBL blgold = new clsBL(new clsGoldDiscountStrategy());
                        blgold.costPrice = 5;
                        blgold.qty = 10;

                        result = blgold.GetfinalPrice(blgold.costPrice, blgold.qty);
                        break;

                    case "PLATINUM":
                        //Platinum
                        clsBL blplatinum = new clsBL(new clsPlatinumDiscountStrategy());
                        blplatinum.costPrice = 10;
                        blplatinum.qty = 8;

                        result = blplatinum.GetfinalPrice(blplatinum.costPrice, blplatinum.qty);
                        break;
                    default:
                        Console.WriteLine("Enter the discount value as either  gold or platinum");
                        break;
                }

                Console.WriteLine("The result for " + filter + " is " + result);

            }
            else
            {
                Console.WriteLine("Enter the discount value");
                return;
            }

            Console.ReadLine();

        }

The classes seem OK but the usage makes no sense:

                case "GOLD":
                    //Gold
                    clsBL blgold = new clsBL(new clsGoldDiscountStrategy());
                    blgold.costPrice = 5;
                    blgold.qty = 10;

                    result = blgold.GetfinalPrice(blgold.costPrice, blgold.qty);
                    break;

This code (and the other switch case) does not use the polymorphism that the interface IStrategy enables. Rather, you can have simply written this code as:

                case "GOLD":
                    double GetGoldPrice(double x, double y) => (x * y) * 0.8;
                    result = GetGoldPrice(5, 10);
                    break;

Here, all classes and interfaces are deleted. There is just a static method.

If you want to use the strategy pattern it must look something like:

IStrategy strategy = GetStrategy();
strategy.Calculate(...);

Here, GetStrategy() can return any object compliant with that interface and further code does depend on any concrete type.

Regarding naming, the classes do not have to be called "Strategy". I'd use the names IDiscountCalculator and GoldDicountCalculator. Or, instead of "Calculator" you could use "Model" or "Provider".


3 last distinct values of a list taken from a list of dictionaries I have a list of orders (each order is a dict) which looks like this (simplified version) :

[{'name' : XXX,
  'id' : { 'order_id_local' : 'xxx_001'}},
 {'name' : XXX,
  'id' : { 'order_id_local' : 'xxx_002'}},
 {'name' : XXX,
  'id' : {}},
 {'name' : XXX,
  'id' : { 'order_id_local' : 'xxx_002'}},
 {'name' : XXX,
  'id' : { 'order_id_local' : 'xxx_003'}},
  ...]

As you can see there could be duplicate for the key 'order_id_local' but also nothing.
What I would like to do is to get the last distinct 3 'order_id_local' in a list, beginning from the last one.
Here it will be ['xxx_0003', 'xxx_002', 'xxx_001'].

What i did is :

 id_orders = [x['id']['order_id_local'] for x in order_list if 'order_id_local' in x['id']]
 id_orders = [x for x in id_orders if x is not None]
 id_orders = list(reversed(sorted(set(id_orders))[-3:]))

It works but when i see this id_orders three times and those nested functions, i'm wondering if there is no a more efficient and pythonic way to do this.

Use a dict comprehension to keep track of the last index for each order_id_local and skip blank entries:

local_ids = {order['id']['order_id_local']:n for n,order in enumerate(data) if order['id']}

Then reverse sort the dictionary keys by their value and slice off the first 3:

sorted(local_ids.keys(), reverse=True, key=local_ids.get)[:3]

result:

['xxx_003', 'xxx_002', 'xxx_001']


Match blood types in C I have written a solution to the blood-type matching problem, as described at https://projectlovelace.net/problems/blood-types/.  The problem is to determine whether a given recipient (in this case, argv[1]) will find a match for a blood transfusion in an array of available donors (argv + 2).

Input blood type: B+
Input list of available blood types: A- B+ AB+ O+ B+ B-
Output: match

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <err.h>

typedef struct {
    enum { O, A, B, AB } abo;
    enum { P, M } rh;
} Blood;

const int abo[4][4] = {
    { O, O, O, O }, // O
    { O, A, O, A }, // A // *
    { O, B, O, B }, // B // *
    { O, A, B, AB }, // AB
};

const int rh[2][2] = {
    { P, M }, // P
    { M, M }, // M // *
};

Blood
parse(char *s){
    char rh0 = s[strlen(s)-1];
    char *abo0 = strdup(s);
    abo0[strlen(s)-1] = '\0';
    Blood b = {
        !strncmp(abo0, "O", 1) ? O
            : !strncmp(abo0, "A", 1) ? A
            : !strncmp(abo0, "B", 1) ? B
            : !strncmp(abo0, "AB", 2) ? AB
            : -1,
        rh0 == '+' ? P
            : rh0 == '-' ? M
            : -1,
    };
    return b;
}

int
main(int argc, char *argv[]) {
    if(argc < 3)
        err(1, "no arguments given\n");
    int n = argc - 2;
    char *a0 = argv[1];
    char **as = argv + 2;

    Blood b0 = parse(a0);
    Blood *bs = malloc(sizeof(Blood) * n);
    for(int i = 0; i < n; i++)
        bs[i] = parse(as[i]);

    for(int i = 0; i < 4; i++) {
        for(int j = 0; j < 2; j++) {
            for(int k = 0; k < n; k++) {
                if(abo[b0.abo][i] == bs[k].abo && rh[b0.rh][j] == bs[k].rh) {
                    printf("match\n");
                    return 0;
                }
            }
        }
    }
    printf("no match\n");
    return 1;
}

which works correctly in my tests, but uses some hacks.  Specifically, lines marked with // * have repeated values of enums, since I couldn't find a way to make loop sizes conditional on blood types.

What would be a better way, without forcing the blood type enums to be the same size?  And more generally, is there a simpler version of the blood-type matching algorithm?

Each blood factor can be present or not present.  The blood types can be bit-encoded using one bit for the A factor, one bit for the B factor, and one bit for the Rh factor.

enum { A=1, B=2, Rh=4 };

You would parse the blood type "AB+" as A + B + Rh == 7, and "O-" as 0 because it does not contain any A factor, B factor, or Rh factor.

Survival requires the donor's blood not contain any factors missing in the recipient's blood type.  Compliment the recipient's blood type to get the bad factors, and use a bit-wise and operation to test that all of the factors are not present in the donor's blood.

bool donor_matches = (donor & ~recipient) == 0;

If you want to leave you enum's as encoding the 4 blood types, and the Rh factor separately, you can get to a similar coding.

Note that:

enum { O, A, B, AB } abo;

will define O=0, A=1, B=2, AB=3, so you have a similar bit assignment as above.  The Rh factor on the other hand would be better to reverse the order:

enum { M, P } rh;

so that M=0, P=1.  Now you could express the match condition:

bool donor_match = (donor_abo & ~recipient_abo) == 0 && (donor_rh & ~recipient_rh) == 0;

Finally, if you want to leave the rh enum as you have originally defined it:

enum { P, M } rh;

then note that donor_rh >= recipient_rh would express that the rh factors match, or the donor has a less-restrictive (numerically higher) rh factor.


Task management program in Python I made a simple program in Python

stores tasks in the file
allows the user to manage them.

I'm interested if my implementation is efficient and could I do something better. I'm not sure whether loading all data from file to memory and again saving it in the file is a good way to implement list saving tasks.
#!/usr/bin/env python3
# -*- coding: utf-8 -*-

import os

CHOICE_LIST_TASKS = 1
CHOICE_ADD_TASK = 2
CHOICE_REMOVE_TASK = 3
CHOICE_QUIT = 4

def read_int(prompt='> ', errmsg='Invalid number!'):
    num = None
    while num is None:
        try:
            num = int(input(prompt))
        except ValueError:
            print(errmsg)
    return num

def display_menu():
    print('What do you want to do?')
    print('[1] List all tasks')
    print('[2] Add a new task')
    print('[3] Delete task')
    print('[4] Quit')

choice = None
file = None
if not os.path.exists('tasks'):
    file = open('tasks', 'w+')
else:
    file = open('tasks', 'r+')
tasks = []

for line in file.readlines():
    tasks.append(line.strip())

while choice != CHOICE_QUIT:
    display_menu()
    choice = read_int()
    if choice == CHOICE_LIST_TASKS:
        if len(tasks) == 0:
            print('Task list is empty!')
        else:
            print('Tasks:')
            for x in tasks:
                print(x)
    elif choice == CHOICE_ADD_TASK:
        desc = input('Which task do you want to do? ')
        index = tasks.append(desc)
        print('Successfully added a new task!')
    elif choice == CHOICE_REMOVE_TASK:
        if len(tasks) == 0:
            print('Task list is empty!')
        else:
            for x in tasks:
                print(tasks.index(x), x)
            index = read_int('Which task do you want to delete? ')
            try:
                tasks.pop(index)
                print('Successfully deleted task!')
            except IndexError:
                print('Please enter proper task number')
    elif choice == CHOICE_QUIT:
        print('Good bye!')
    else:
        print('Invalid choice!')

file = open('tasks', 'w+')
for x in tasks:
    file.write(x + '\n')

Let's start from the very top.
CHOICE_LIST_TASKS = 1
CHOICE_ADD_TASK = 2
CHOICE_REMOVE_TASK = 3
CHOICE_QUIT = 4

It's clear to me that you want to create variables and assign them certain integers so you don't have to use magic numbers anywhere, kudos to you for this. There is a much better way to implement this idea in python
Enumerations : Enum
Enumerations in Python

An enumeration is a set of symbolic names
(members) bound to unique, constant values

creating an Enum
class Choices(Enum):
    list_task = 1
    add_task = 2
    remove_task = 3
    quit = 4

This already looks much clearer than random global variables. You can also iterate through the enumeration and display its values with names .
print(Choices.add_task.name)

add_task

print(Choices.add_task.value)

1

Use more functions
file = None
if not os.path.exists('tasks'):
    file = open('tasks', 'w+')
else:
    file = open('tasks', 'r+')
tasks = []

for line in file.readlines():
    tasks.append(line.strip())

Why can't we move this into a function called read_task_file()?
This way it is extremely clear to me, you, you after a year reading old projects, and anybody else who will ever read this code that those few lines of code will read the task file. Without that, one must read the code thoroughly and understand its purpose.
def read_task_file():
    file = None
    if not os.path.exists('tasks'):
        file = open('tasks', 'w+')
    else:
        file = open('tasks', 'r+')
    tasks = []
    for line in file.readlines():
        tasks.append(line.strip())
    return tasks

now getting the tasks is
tasks = read_task_file()

This exact same applies to the next set of code, which will let the user enter their choice so we can perform it.
Factoring out the part where we read a choice from the user and perform  into a function would look like
def read_task_choice():
    display_menu()
    choice = read_int()
    if choice not in Choices._value2member_map_:
        print("Invalid input! Please enter a correct choice\n")
        read_task_choice()

Note: Choices._value2member_map_ will be all the values in an enumeration.
This already looks much cleaner! We eliminate the need of having it in a loop where it will check all the cases, the function only returns choice when there is a valid input.
As I said, factor out the work into functions and all them when required. I have done the same for the 4 main jobs ( the 4 choices ).

List tasks
def list_tasks(tasks):
    if len(tasks) == 0:
        print("Task list is empty!")
        return None

    for i in range(len(tasks)):
        print(f"{i} {tasks[i]}")

Add new task
def add_new_task(tasks):
    return input("What task would you like to finish ?: ")
    tasks.append(task)

Remove task
def remove_task(tasks):
    if len(tasks) == 0:
        print("Task list is already empty!")
        return None

    list_tasks(tasks)
    task = int(input(("Which task would you like to remove: ?")))

    if task < 0 or task >= len(tasks):
        print("Invalid input! Please select an appropriate task")
        remove_task(tasks)
    tasks.pop(task)

And finally
Update task file
def update_task_file(tasks):
    file = open('tasks', 'w+')
    for x in tasks:
        file.write(x + '\n')

What is the point of having millions of functions when I can put everything like I have already done?
Now when you have to play the game
tasks = read_task_file()

while True:
    choice = read_task_choice()
    if choice == Choices.list_task.value:
        list_tasks(tasks)

    elif choice == Choices.add_task.value:
        add_new_task(tasks)

    elif choice == Choices.remove_task.value:
        remove_task(tasks)

    else:
        break

    update_task_file(tasks)

This is the main advantage of having strongly typed functions.
Clearing the screen and waiting for input
Due to the absence of Graphics, we can see that after a few choices the terminal looks a little strange
We can improve this by simply clearing the screen after each choice, and then wait for the user to press a key
Clearing the terminal
There are a few ways. A popular way is to use os.system("cls") if you're on windows, but calling this is quite expensive and also makes your program more platform dependant.
You can do
print(chr(27) + "[2J")

And this will also work.
Waiting for input
Adding this line input("Press any key to continue...") will wait for the user's response before displaying all the choices again, making the experience better.
Final
#!/usr/bin/env python3
# -*- coding: utf-8 -*-

import os
from enum import Enum

class Choices(Enum):
    list_task = 1
    add_task = 2
    remove_task = 3
    quit = 4

def read_int(prompt='> ', errmsg='Invalid number!'):
    num = None
    while num is None:
        try:
            num = int(input(prompt))
        except ValueError:
            print(errmsg)
    return num

def display_menu():
    print('What do you want to do?')
    print('[1] List all tasks')
    print('[2] Add a new task')
    print('[3] Delete task')
    print('[4] Quit')

def read_task_file():
    file = None
    if not os.path.exists('tasks'):
        file = open('tasks', 'w+')
    else:
        file = open('tasks', 'r+')
    tasks = []
    for line in file.readlines():
        tasks.append(line.strip())
    return tasks

def update_task_file(tasks):
    file = open('tasks', 'w+')
    for x in tasks:
        file.write(x + '\n')

def read_task_choice():
    display_menu()
    choice = read_int()
    if choice not in Choices._value2member_map_:
        print("Invalid input! Please enter a correct choice\n")
        read_task_choice()
    return choice

def add_new_task(tasks):
    task = input("What task would you like to do: ")
    tasks.append(task)

def list_tasks(tasks):
    if len(tasks) == 0:
        print("Task list is empty!")
        return None

    for i in range(len(tasks)):
        print(f"{i} {tasks[i]}")

def remove_task(tasks):
    if len(tasks) == 0:
        print("Task list is already empty!")
        return None

    list_tasks(tasks)
    task = int(input(("Which task would you like to remove: ?")))

    if task < 0 or task >= len(tasks):
        print("Invalid input! Please select an appropriate task")
        remove_task(tasks)
    tasks.pop(task)

def clear_screen():
    print(chr(27) + "[2J")

def pause():
    input("Press any key to continue...")

tasks = read_task_file()

while True:
    clear_screen()
    choice = read_task_choice()
    if choice == Choices.list_task.value:
        list_tasks(tasks)

    elif choice == Choices.add_task.value:
        add_new_task(tasks)

    elif choice == Choices.remove_task.value:
        remove_task(tasks)

    else:
        break

    update_task_file(tasks)
    pause()


Switching back and forth between forms Topic 1: Switching to another form from the project startup form

I use the following code that calls an instance of my form for payment (frmPayment) from my startup form (frmMainMenu) using a click event:

Private Sub btnPayment_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles btnPayment.Click, PaymentToolStripMenuItem.Click
        Dim frmPaymentX As New frmPayment() 'declare payment form
        Me.Visible = False
        frmPaymentX.ShowDialog()
End Sub

Should I use something other than Me.Visible = False in handling my startup form (frmMainMenu) during my form switch?

Topic 2: Switching back to the startup form

Likewise, I use the following code to return to my startup form (frmMainMenu) from my form for payment (frmPayment):

Private Sub btnMainMenu_Click(ByVal sender As System.Object, ByVal e As
System.EventArgs) Handles btnMainMenu.Click
        frmMainMenu.Show()
        Me.Close()
End Sub

Should I directly call my startup form (frmMainMenu) as I am doing in my example or should I be calling an instance of my starup form (frmMainMenu)?
I believe that I should use show() and not showdialog() for this.  Is this correct?
Should I use something other than Me.Close()?

Firstly, I think you need to clarify why you are wanting to show the payment form and hide the main form. In most applications, when you want to show a dialog of some description, you would show it modally (using dialog.ShowDialog()) so that it appears over the top of your current form and prevents the user from interacting with the other form until the opened dialog is closed.

Secondly, if you are wanting your current main form and payment dialog as two screens as opposed to a form and a dialog, then you may be better off creating some shell form that can contain a UserControl. You can then build the main form and payment form as two UserControl's and simply switch which is being displayed on your window.


Dice game with rules and three dice I'm trying to make a dice game with three dice and with rules.

It costs 10 credits to roll the dice.
If you get 6 6 6 you win 90 credits.
If you get 5 5 5, 4 4 4, 3 3 3, 2 2 2, 1 1 1, you win 40 credits.
If two of the dice have the same number you win 0 credits (because it costs 10 to roll).

Here's what I've done so far:

#include <iostream>
#include <stdlib.h>
#include <time.h>

int main(void) {

    srand((int)time(NULL));

    int diceRoll1, diceRoll2, diceRoll3;
    char yn;

    do {
        printf("\nDo you want to play? (y/n): ");
        scanf_s("%c", &yn);
        getchar();

        if (yn == 'n') {
            printf("Coward!");
            getchar();
            return 0;
        }

        diceRoll1 = (rand() % 6) + 1;
        diceRoll2 = (rand() % 6) + 1;
        diceRoll3 = (rand() % 6) + 1;

        printf("Your dices are %d %d %d \n", diceRoll1, diceRoll2, diceRoll3);

        if (diceRoll1 == 6 && diceRoll2 == 6 && diceRoll3 == 6) {
            printf("Congratulations! You won 90 credits.\n");
        }

        if (diceRoll1 == 5 && diceRoll2 == 5 && diceRoll3 == 5) {
            printf("Congratulations! You won 40 credits.\n");
        }

        if (diceRoll1 == 4 && diceRoll2 == 4 && diceRoll3 == 4) {
            printf("Congratulations! You won 40 credits.\n");
        }

        if (diceRoll1 == 3 && diceRoll2 == 3 && diceRoll3 == 3) {
            printf("Congratulations! You won 40 credits.\n");
        }

        if (diceRoll1 == 2 && diceRoll2 == 2 && diceRoll3 == 2) {
            printf("Congratulations! You won 40 credits.\n");
        }

        if (diceRoll1 == 1 && diceRoll2 == 1 && diceRoll3 == 1) {
            printf("Congratulations! You won 40 credits.\n");
        }

    } while (yn != 'n');

    getchar();
    return 0;

}

I have not implemented all of the rules and the ones I have done can probably be done in a better way. I don't have much experience, so I wonder what the best way is to have all the rules in the game and so the code looks good.

Reducing the amount of code in any program is good because that reduces the possible errors or bugs in the code. Providing general functions that can be reused is a good habit to get into.

DRY Code

There is a programming principle called the Don't Repeat Yourself Principle sometimes referred to as DRY code. If you find yourself repeating the same code multiple times it is better to encapsulate it in a function. If it is possible to loop through the code that can reduce repetition as well.

As shown in another answer all the if statements of the form

        if (diceRoll1 == **x** && diceRoll2 == diceRoll1 && diceRoll3 == diceRoll1) {
            printf("Congratulations! You won **YY** credits.\n");
        }

Can be reduced to a simple function that can be reused.

This code can also be simplified, although it is not absolutely necessary

    diceRoll1 = (rand() % 6) + 1;
    diceRoll2 = (rand() % 6) + 1;
    diceRoll3 = (rand() % 6) + 1;

Can be

static constexpr int dieFaces = 6;      // Allows the number of faces of the dice to be changed in one place
                                        // if this is used in a board game such as D & D

int rollOneDie()
{
    return (rand() % dieFaces) + 1;
}

void rollTheDice(int &die1, int &die2, int &die3)
{
    die1 = rollOneDie();
    die2 = rollOneDie();
    die3 = rollOneDie();
}

Any time I see variables with the names var1, var2 and var3 I see the potential for using an array instead of separate variables. The entire program might be simplified if instead of 3 separate variables diceRoll1, diceRoll2 and diceRoll3 there was one array of dice. This would also allow the program to be easily changed so that more or less dice are used.

static constexpr int diceCount = 3;
static constexpr int highScore = 90;
static constexpr int normalScore = 40;
static constexpr int lowScore = 10;
static constexpr int noScore = 0;

void rollTheDice2(int dice[])
{
    for (int i = 0; i < diceCount; i++)
    {
        dice[i] = rollOneDie();
    }
}

bool allTheDiceEqualThisNumber(int dice[],int faceValue)
{
    for (int i = 0; i < diceCount; i++)
    {
        if (dice[i] != faceValue)
        {
            return false;
        }
    }
    return true;
}

int earnCredit(int dice[])
{
    if (allTheDiceEqualThisNumber(dice, dieFaces))
    {
        return highScore;
    }
    else if (allTheDiceEqualThisNumber(dice, 1))
    {
        return lowScore;
    }
    else
    {
        for (int i = 2; i < dieFaces - 1; i++) {
            if (allTheDiceEqualThisNumber(dice, i)) {
                return normalScore;
            }
        }
    }
    return noScore;
}

void showDice(int dice[])
{
    printf("Your dices are ");
    for (int i = 0; i < diceCount; i++)
    {
        printf("%d ", dice[i]);
    }
    printf("\n");
}

void playGame()
{
    int diceRolls[diceCount];

    rollTheDice2(diceRolls);

    showDice(diceRolls);

    int newCredits = earnCredit(diceRolls);
    if (newCredits > 0)
    {
        printf("Congratulations! You won 90 credits.\n");
    }
}

Complexity

The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

  that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

int main(void)
{
    srand((int)time(NULL));

    char yn;

    do {
        printf("\nDo you want to play? (y/n): ");
        scanf_s("%c", &yn);
        getchar();

        if (yn == 'n') {
            printf("Coward!");
            getchar();
            return 0;
        }

        playGame();

    } while (yn != 'n');

    getchar();
    return 0;
}

Magic Numbers

There are Magic Numbers in the main() function (6, 90, 40, 10), it might be better to create symbolic constants for them to make the code more readble and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

The possible symbolic constants are show in the examples for DRY Code

C versus C++

This question was originally tagged with C rather than C++. The strict version of C can't compile #include <iostream>, it uses #include <stdio.h> instead. Also input in C++ generally prefers

std::cin >> variableName;

over scanf, scanf_s or getchar() and

std::cout << "Your score was " << variableName << "\n";

over printf().

C++ also has a newer random number generator with better distribution using #include <random>.


Select some indexes in a list of lists I have a record type which holds a list of table rows. Each row is also a list. The first row is assumed to be the header row. In addition the type optionally defines a filter - a list of columns (the actual string value of the column header).

Then the type have a member property FilteredRows. If no filter is defined, the property is the entire table. But if there is a filter, FilteredRows return the list of rows, but only with the columns defined by the filter.

I'm not interested in alternative ways to represent this. This is part of a bigger solution, it's working, and the design makes sense to me. What I believe I need however is some more eyes on the internals of FilteredRows. I'm looking for clever tricks on solving this problem in different ways, and other pointers on how I can improve the solution.

type Table =
    {
        Rows : string list list
        OnlyColumns : string list option
    }
    member internal x.FilteredRows =
        match x.OnlyColumns with
        | None -> x.Rows
        | Some xs ->
            let indexesToInclude = seq {
                let headers = List.head x.Rows
                for c in xs do
                    yield List.findIndex ((=) c) headers }
            x.Rows
            |> List.map
                (fun row ->
                    indexesToInclude
                    |> Seq.map (List.nth row)
                    |> List.ofSeq )

As you can see, what I've done is first to fetch all the column indexes using List.findIndex. And then for each row I map over the column indices and re-create the row using List.nth.

The only real change that pops out at me without touching the record datatypes is to make indexesToInclude an array, you're evaluating it over and over again in that seq comprehension each time. Since it's an array and lookups are fast, and you know its size, you might as well pre-allocate it instead of using a comprehension which will be much faster. Then do a mapi over the rows, and just look up if you want that index in the array.

type Table =
    {
        Rows : string list list
        OnlyColumns : string list option
    }
    member internal x.FilteredRows =
        match x.OnlyColumns with
        | None -> x.Rows
        | Some xs ->
            let headers = List.head x.Rows |> List.toArray
            let isIncluded = Array.init (headers.Length) (fun hi -> xs |> List.exists (fun lv -> lv = headers.[hi]))
            x.Rows
            |> List.map (fun row -> row |> List.mapi (fun i v ->  (isIncluded.[i], v))
                                        |> List.filter fst |> List.map snd)

The most important thing here is that we're no longer indexing into a list (almost always a bad idea in F#), which costs O(n) where n is the index you're indexing into. If you do this every time it gets very expensive!


Checking all checkboxes Problems:

Wanting to check all checkboxes when the checkbox w/id=checkall is checked.
Wanting to uncheck the "checkall" box when any other checkbox is unchecked.

Solution:

$(document).ready(function () {
$('#checkall').on('click', function () {
if ($(this).is(':checked')) {
    $('input[type="checkbox"]').each(function () {
        console.log(this);
        this.checked = true;
    });
} else {
    $('input[type="checkbox"]').each(function () {
        this.checked = false;
    });
}
// Uncheck "Checkall" if any other checkbox is unchecked
if ($('#checkall').is(':checked')) {
    $('input[type="checkbox"]:not("#checkall")').click(function () {
        console.log(this);
        $('#checkall').attr('checked', false);
    });
}
});
});

I'm new with jQuery and I'm not sure if what I've written is really and truly "production value" code, but it is operational. It's not a matter of life and death, it's really just a personal project I'm working on that will likely be used by myself and some friends only. I am in the game to learn, though, and if the code can be improved upon please point me in the right direction to read about it more in depth. I've only been able to find bits and pieces on this topic.

You can streamline the code by caching the selectors; this will improve performance as well. Next, you should use prop instead of attr as the jQuery documentation states:

  The .prop() method should be used to set disabled and checked instead
  of the .attr() method.

Lastly, instead of using an if..else statement, you can write the condition inline, since checked expects a boolean, and the expression resolves to a boolean. In other words:

// Cache selectors
var $all = $('#checkall');
var $checks = $('input[type="checkbox"]').not($all);

$all.on('click', function () {
  $checks.prop('checked', $(this).is(':checked'));
});

$checks.on('click', function() {
  var checked = $checks.filter(':checked').length;
  $all.prop('checked', checked === $checks.length);
});

Here is a demo: http://jsfiddle.net/z7qy10nn/


Guessing game - am I using the concept of objects in Java properly? I'm new to Java and find it hard to grasp the concept of objects. I wrote a simple guessing game to practice the concept of OO programming but I am not sure if I am doing it correctly.

The objective of the game is to guess a number from 0-9 with 3 players and the first player who guesses it correctly will win the game.

Player

public class Player {

    private String playerName;
    private int number;

    public int getNumber() {
        return number;
    }

    public String getPlayerName() {
        return playerName;
    }

    public void setPlayerName(String playerName) {
        this.playerName = playerName;
    }

    public void guessNumber(){
        number = Integer.parseInt(JOptionPane.showInputDialog(playerName
            + "'s Turn\nGuess the Number"));
        JOptionPane.showMessageDialog(null, playerName+ "'s GUESS is " + number);
    }
}

GuessGame

public class GuessGame {

    private int numberToGuess;
    private Player p1;
    private Player p2;
    private Player p3;

    public void startGame(){
        numberToGuess = (int) (Math.random()*10);
        p1 = new Player();
        p2 = new Player();
        p3 = new Player();

        p1.setPlayerName(JOptionPane.showInputDialog("Enter Player 1 Name: "));
        p2.setPlayerName(JOptionPane.showInputDialog("Enter Player 2 Name: "));
        p3.setPlayerName(JOptionPane.showInputDialog("Enter Player 3 Name: "));

        int flagWinner = 0;
        while(0==flagWinner){
            p1.guessNumber();
            if (p1.getNumber()==numberToGuess){
                flagWinner = 1;
                break;
            }
            JOptionPane.showMessageDialog(null, p1.getPlayerName()
                + "'s Guess is Wrong!");
            p2.guessNumber();
            if (p2.getNumber()==numberToGuess){
                flagWinner = 2;
                break;
            }
            JOptionPane.showMessageDialog(null, p2.getPlayerName()
                + "'s Guess is Wrong!");
            p3.guessNumber();
            if (p3.getNumber()==numberToGuess){
                flagWinner = 3;
                break;
            }
            JOptionPane.showMessageDialog(null, p3.getPlayerName()
                + "'s Guess is Wrong!");
        }
        if (1 == flagWinner){
            JOptionPane.showMessageDialog(null,p1.getPlayerName()+ " Wins!");
        } else if (2 == flagWinner){
            JOptionPane.showMessageDialog(null,p2.getPlayerName()+ " Wins!");
        } else JOptionPane.showMessageDialog(null,p3.getPlayerName()+ " Wins!");
    }
}

And I have a gameLauncher where I have the main() method which creates the game.

Am I doing it correctly especially in the Player class where I created a method guessNumber() instead of setter setNumber(int number)?

Am I violating any object-oriented concepts in my code?

How can I improve my code?

To start with, spot the inconsistency. For the name you create it in your main loop and then use a setter; for the number you have a method in the Player object to acquire the guess. The latter is more in line with the message-passing philosophy of OO. (Many people seem to think that using getters and setters is being OO. They're often - not always - a sign of non-OO thinking). However, it doesn't take it as far as it could. The number guessed isn't a property of the player - it's a property of the guess.

Guessing a number isn't the same thing as displaying it - either rename the method or, probably better - a method should do one thing - move that out.

Next up, the flagWinner is a big red flag. Its sole purpose is to identify one of the objects p1, p2, p3, or none of the above. What's the OO way to do that? Use the object to identify itself.

I'd be inclined to factor out the IO as an object too, to be a bit less procedural. So without going into a full over-patterned architecture we have

public class IO {
    public void output(String msg) {
        JOptionPane.showMessageDialog(null, msg);
    }

    public String input(String prompt) {
        return JOptionPane.showInputDialog(prompt)
    }

    public int inputInt(String prompt) {
        return Integer.parseInt(input(prompt));
    }
}

public class Player {
    private String playerName;

    private Player(String name) {
        playerName = name;
    }

    public static Player createPlayer(IO io, String prompt) {
        String name = io.input("Enter " + prompt + " Name: ");
        return new Player(name);
    }

    public String getPlayerName() {
        return playerName;
    }

    public int guessNumber(IO io) {
        return io.inputInt(playerName +"'s Turn\nGuess the Number"));
    }
}

public class GuessGame {

    private final IO io;
    private final int numPlayers;
    private int numberToGuess;
    private List<Player> players = new LinkedList<Player>();

    public GuessGame(IO io, int numPlayers) {
        this.io = io;
        this.numPlayers = numPlayers;
    }

    public void init() {
        numberToGuess = (int) (Math.random()*10);
        for (int i = 0; i < numPlayers; i++) {
            players.add(Player.createPlayer(io, "Player " + (i+1)));
        }
    }

    public void runGame(){
        init();

        Player winner = null;
        Iterator<Player> it = players.iterator();
        while (winner == null) {
            if (!it.hasNext()) {
                it = players.iterator();
            }

            Player turnPlayer = it.next();
            String playerName = turnPlayer.getPlayerName();

            int guess = turnPlayer.guessNumber(io);
            io.output(playerName+ "'s GUESS is " + number);

            if (guess == numberToGuess) {
                winner = turnPlayer;
            }
            else {
                io.output(playerName + "'s Guess is Wrong!");
            }
        }

        io.output(winner.getPlayerName()+ " Wins!");
    }
}


Software Key Generator I have created a Java program which generates a key (for activating a piece of software for example) my code works correctly however there are parts of it that I know can be improved. In particular inside the createKey method were I have repeated an if statement in multiple places.

I would appreciate if you can take some time to look at and spots ways to improve my code.I would rather learn first and foremost ways to refine the program in its current form.  Also, if you see any bad habits, let me know.

I plan to save each key to a file and build a validator that allows input and references the file.

  Description

  Creates a key of 20 letters and numbers - the letters and numbers are
  set a weighting, if two is returned as a weight for a letter and
  three for numbers.

  AA232BC433 ... so two letters then three numbers and so on.

  10 numbers and 10 letters are then selected.

  Each is chosen through a loop that runs 10 times and randomly assigns
  a value from each final array into a set of 'chosen' values.

  The create key method then cycles through various loops which use the
  weights to add a further random selection of the chosen values -
  checks are added which ensure that the string builder stops adding new
  values when the length is 20.

public class KeyTestDrive {

public static void main(String[] args) {
    Key key = new Key();
    key.createKey();
}

}

import java.util.Random;

public class Key {

private final int[] possibleNumbers = { 1, 2, 3, 4, 5, 6, 7, 8, 9 };
private final char[] possibleLetters = { 'A', 'B', 'C', 'D', 'E', 'F', 'G',
        'H', 'I', 'J', 'K', 'L', 'M', 'O', 'P', 'Q', 'R', 'S', 'T', 'U',
        'V', 'W', 'X', 'Y', 'Z' };

Random randomNum = new Random();
StringBuilder key = new StringBuilder();

public void createKey() {
    int letterWeighting = determineLetterWeigthing();
    int numberWeighting = determineNumberWeigthing();

    int[] chosenNumbers = selectNumbers();
    char[] chosenLetters = selectLetters();

    int randomIndex;

    boolean isSizeTwenty = false;

    for (int i = 0; i < 20; i++) {

        if(isSizeTwenty == true){
            break;
        }

        for (int j = 0; j < letterWeighting; j++) {
            randomIndex = randomNum.nextInt(9);
            key.append(chosenLetters[randomIndex]);

            if(key.length() == 20){
                isSizeTwenty = true;
                break;
            }
        }

        if(isSizeTwenty == true){
            break;
        }

        for (int k = 0; k < numberWeighting; k++) {
            randomIndex = randomNum.nextInt(9);
            key.append(chosenNumbers[randomIndex]);

            if(key.length() == 20){
                isSizeTwenty = true;
                break;
            }
        }

        if(isSizeTwenty == true){
            break;
        }

    }
    System.out.println(key.length());
    System.out.println(key);
}

private char[] selectLetters() {
    char[] chosenLetters = new char[10];

    for (int j = 0; j < 10; j++) {
        int randomIndex = randomNum.nextInt(possibleLetters.length);

        chosenLetters[j] = possibleLetters[randomIndex];
    }

    return chosenLetters;
}

private int[] selectNumbers() {
    int[] chosenNumbers = new int[10];

    for (int j = 0; j < 10; j++) {
        int randomIndex = randomNum.nextInt(possibleNumbers.length);

        chosenNumbers[j] = possibleNumbers[randomIndex];
    }

    return chosenNumbers;
}

private int determineLetterWeigthing() {
    int letterWeighting;
    letterWeighting = randomNum.nextInt(4) + 1;

    return letterWeighting;
}

private int determineNumberWeigthing() {
    int numberWeighting;
    numberWeighting = randomNum.nextInt(4) + 1;

    return numberWeighting;
}
}

Improving createKey

First of all, when the same couple of lines appear duplicated several times, there's clearly a problem. I'm talking about this one:

if(isSizeTwenty == true){
    break;
}

A good first step is to try to eliminate this. Consider this situation:

boolean isSizeTwenty = false;

for (int i = 0; i < 20; i++) {

    if(isSizeTwenty == true){
        break;
    }

    // ...

    if(isSizeTwenty == true){
        break;
    }
}

Notice that the first check inside the loop will be always false: isSizeTwenty is initialized with false, and since you have this check as the last step of the loop, the condition can never be true at the beginning of the loop.

Also, don't write conditions on boolean expressions like this:

if (someboolean == true) { ... }

You can write simpler and better like this:

if (someboolean) { ... }

But, the entire method can be refactored far better, for example:

public String createKey() {
    int letterWeighting = determineLetterWeigthing();
    int numberWeighting = determineNumberWeigthing();

    int[] chosenNumbers = selectNumbers();
    char[] chosenLetters = selectLetters();

    StringBuilder key = new StringBuilder();

    while (key.length() < targetLength) {
        for (int j = 0; j < letterWeighting && key.length() < targetLength; j++) {
            int randomIndex = random.nextInt(chosenLetters.length - 1);
            key.append(chosenLetters[randomIndex]);
        }
        for (int k = 0; k < numberWeighting && key.length() < targetLength; k++) {
            int randomIndex = random.nextInt(chosenNumbers.length - 1);
            key.append(chosenNumbers[randomIndex]);
        }
    }
    return key.toString();
}

I made a couple of changes there:

Replaced the number 20 with a meaningful variable named targetLength
Removed of the isSizeTwenty variable, and moved the condition key.length() < targetLength
Moved StringBuilder inside the method. No need to make that a member variable
Replaced the outermost for (int i; i < 20; i++) { ... } with a more natural while (key.length() < targetLength) { ... }
Replaced the hardcoded number 9 with chosenLetters.length - 1 and chosenNumbers.length - 1. This is more than just about removing another magic number, this is also safer, because now randomIndex can never be accidentally out of bounds. Now it is derived from the length of the available letters and numbers.
Renamed randomNum to just random, because there is nothing "num"-like about an instance of Random

Improving determineLetterWeigthing and determineNumberWeigthing

You could reduce these methods to single lines:

private int determineLetterWeigthing() {
    return random.nextInt(4) + 1;
}

private int determineNumberWeigthing() {
    return random.nextInt(4) + 1;
}

Unit testing

I would make random and targetLength final members, and add these constructors:

private final Random random;
private final int targetLength;

public Key(int targetLength) {
    this(targetLength, new Random());
}

public Key(int targetLength, Random random) {
    this.random = random;
    this.targetLength = targetLength;
}

This way, and thanks to the new version of createKey that returns the key instead of printing it, now you can add some unit tests:

@Test
public void testExampleKeys() {
    assertEquals("VAT3735TVV4949RVJ793", new Key(20, new Random(0)).createKey());
    assertEquals("SBM8PBJ8PSJ6MWS2SBO2", new Key(20, new Random(1)).createKey());
    assertEquals("DGG77DJS28SDP13GGD77", new Key(20, new Random(2)).createKey());
    assertEquals("PGB844MGM477GCG728PL", new Key(20, new Random(3)).createKey());
}

@Test
public void testCreateMultipleKeysFromOneGenerator() {
    Key keygen = new Key(20, new Random(10));
    assertEquals("YTD15DTT52YTO12IRL21", keygen.createKey());
    assertEquals("IK617LP637LG611IK619", keygen.createKey());
    assertEquals("VH1WY5YO1YP8SO9YO1YW", keygen.createKey());
}

Actually, it would be better to rename your class to KeyGenerator.
Notice the 2nd unit test above: I called the instance keygen, which is just natural, and changing the class name to match would be natural too.

Other things

An easier way to create possibleLetters:

private final char[] possibleLetters = "ABCDEFGHIJKLMOPQRSTUVWXYZ".toCharArray();

Btw, do you have something against the letter "N"? It wasn't on your list :)

In selectLetters and selectNumbers, 10 is a magic number:

char[] chosenLetters = new char[10];

It would be better to make it a private static final constant at the top of the class with a good name.

Actually it would be better to make selectLetters and selectNumbers take a parameter.


Regex detection for Puppet configuration I've got a PR to add a feature to a Ruby application (Puppet) to allow Regex detection.

Right now, the method only does comparison on exact string matching:

defaultfor :operatingsystem => :fedora, :operatingsystemmajrelease => ['22', '23', '24']

This is annoying, as we have to add in new values everytime a new Fedora release comes out, for example.

I've extended it to match on a Regex:

defaultfor :operatingsystem => :fedora, :operatingsystemmajrelease => /^2[2-9]$/

The old method looks like this:

def self.fact_match(fact, values)
  values = [values] unless values.is_a? Array
  values.map! { |v| v.to_s.downcase.intern }

  if fval = Facter.value(fact).to_s and fval != ""
    fval = fval.to_s.downcase.intern

    values.include?(fval)
  else
    false
  end
end

This is my current proof-of-concept code, which will match both regex and strings, but it feels off.

def self.fact_match(fact, values)

  values = [values] unless values.is_a? Array

  values.map! do |v|
    if v.is_a? Regexp
      v
    else
      v.to_s.downcase.intern
    end
  end

  if fval = Facter.value(fact).to_s and fval != ""
    fval = fval.to_s.downcase.intern
    if values.any? {|v| v.is_a? Regexp }
      regex_match = Regexp.union(values)
      fval =~ regex_match
    else
      values.include?(fval)
    end
  else
    false
  end
end

I feel like there's a much easier way of detecting if it's a Regex or not.

You're working too hard, trying to distinguish strings from regular expressions.  Rather, you should take advantage of the fact that the === operator for a RegExp and the === operator for a String both express the idea of a "match", which is what you want here.

I don't understand why you need to #intern all the strings.

I don't see why the fval = Facter.value(fact).to_s assignment appears inside an if condition.  The result of the assignment would only be false if Facter.value(fact).to_s is false or nil, which can never happen.  (Even nil.to_s results in an empty string.)

To treat values as an array regardless of whether a scalar or an array was passed in, you can use the [*values] idiom.

def self.fact_match(fact, values)
  fact_val = Facter.value(fact).to_s.downcase
  fact_val != "" and [*values].any? { |v| v === fact_val }
end

In addition to supporting literal case-insensitive matches and regular expression matches, also consider supporting Ranges.


Spring autowiring in managed beans with support for serialization - is this safe? Im trying to solve two problems I see with JSF2 and Spring.
First

@Autowired in @ManagedBean does not work
Nor does @ViewScoped with @Controller
@ManagedProperty is not type safe and requires setter

Second

You can not serialize singleton spring beans without hassle
Managed beans must be serializable so the container can serialize the session

So I enabled @Autowired in managed beans and fixed serialization with this common base class which all managed beans (at least @SessionScoped and @ViewScoped ones) derive from.
public abstract class Handler implements Serializable {
    private static final long serialVersionUID = 1L;

    @PostConstruct
    private void init() {
        Spring.appCtx.getAutowireCapableBeanFactory().autowireBean(this);
    }

    private void readObject(ObjectInputStream ois) throws ClassNotFoundException, IOException {
        ois.defaultReadObject();
        Spring.appCtx.getAutowireCapableBeanFactory().autowireBean(this);
    }
}

Handlers are now implemented like that:
@ManagedBean
@RequestScoped
public class Index extends Handler implements Serializable {
    private static final long serialVersionUID = 1L;

    @Autowired
    SampleService ss;

    public String hello() {
        return ss.sayHello();
    }
}

First tests seem fine, but I'm curious if I'm causing any problems I don't know of.

Handler is too generic name for a class. Try to find something more descriptive (which doesn't force readers to check the implementation if they want to know what the class does), SerializationHandler, for example.
In Java classes could have only one superclass. If you make it an abstract superclass you can't have another one which might be cumbersome in some cases.

  Inheritance is appropriate only in circumstances where the subclass really is a
  subtype of the superclass. In other words, a class B should extend a class A only if
  an “is-a” relationship exists between the two classes. If you are tempted to have a
  class B extend a class A, ask yourself the question: Is every B really an A? If you
  cannot truthfully answer yes to this question, B should not extend A. If the answer
  is no, it is often the case that B should contain a private instance of A and expose a
  smaller and simpler API: A is not an essential part of B, merely a detail of its
  implementation.

Source: Effective Java, Second Edition, Item 16: Favor composition over inheritance

I'd use composition with a helper class:

@ManagedBean
@RequestScoped
public class Index implements Serializable {
    private static final long serialVersionUID = 1L;

    @Autowired
    SampleService ss;

    public String hello() {
        return ss.sayHello();
    }

    @PostConstruct
    private void init() {
        WireHelper.wireObject(this);
    }

    private void readObject(ObjectInputStream ois) throws ClassNotFoundException, IOException {
        ois.defaultReadObject();
        WireHelper.wireObject(this);
    }
}

public final class WireHelper {
    public static void wireObject(Object object) {
        Spring.appCtx.getAutowireCapableBeanFactory().autowireBean(this);
    }
}

If you stay with the original implementation: you don't necessarily need implements Serializable here since Handler already implements that interface:

public class Index extends Handler implements Serializable {
    ...
}


Display data from two MySQL database tables This is my first attempt at PHP PDO. I needed to get data from two tables in my MySQL database. I'm pleased to say the code works, and I was just wondering if what I've done is correct.

<?php

// MySQL Database Connection
$db = new PDO('mysql:host=localhost;dbname=database;charset=utf8mb4', 'root', '');
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

// Display Data
foreach($db->query('SELECT * FROM data1 LEFT JOIN data2 ON data1.id = data2.id WHERE data1.id=1') as $row) {
    // Data is displayed in their respected places.
}

?>
<!doctype html>

<meta charset="utf-8">

<title>Title</title>
<meta name="description" content="Description">
<meta name="author" content="Author">

<link rel="stylesheet" href="css/styles.css">

        <?php echo $row['sample1']; ?>
        <?php echo $row['sample2']; ?>

        <?php echo $row['extra1']; ?>
        <?php echo $row['extra2']; ?>

foreach
Don't have an empty foreach loop... ever. Similarly, just because php lets $row escape from the context it is in does not mean you should use it after the foreach-loop is over.
PDO::query
I recommend against using PDO::query, especially because you lack a thorough understanding of what you are doing. You can use it when executing complete static queries (those that do not have variables), but you already have a where-clause in that sql statement and it is only a matter of time until you want to change that to something dynamic. Use PDO::prepare instead.
PDO::prepare
You are using $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);. Good. Except that you do not use prepared statements at all. Keep in mind that your mysql driver must be able to use native prepared statements too.
PDO::ERRMODE_EXCEPTION
You are setting the error mode to PDO::ERRMODE_EXCEPTION. Good. Except that you do not catch any of them and instead let the page return blank.


Find maximum in sorted and rotated array This is an interview question.
A sorted rotated array is a sorted array which was rotated 0 or more times.

For example: [1,2,3] -> [2,3,1]

Please tell me what do you think about the following (correctness, efficiency, coding conventions) and specifically if I can remove in some way the special handle for array of two elements:

static int findMax(int arr[]) {
    return findMax(arr, 0 , arr.length - 1);
}

static int findMax(int arr[], int low, int high) {
    int middle;

    if (low == high) {
        return arr[low];
    }
    if ( Math.abs(high - low) == 1 ) {
        return Math.max(arr[low], arr[high]);
    }

    middle = (low + high) >> 1;
    if (arr[middle] > arr[middle + 1]) {
        return arr[middle];
    }
    if (arr[low] > arr[middle]) {
        return findMax(arr, low, middle - 1);
    }
    return findMax(arr, middle + 1, high);
}

Math.abs in Math.abs(high - low) == 1 is suspicious. high should always be not less than low. Remove it.
middle = (low + high) >> 1 may (and will) overflow. Do middle = low + (high - low) >> 1.

That said, >> 1 is no better than) / 2.
Java is notorious in not eliminating the tail recursion. You should do it manually. First, rewrite it in a tail-recursive form:

    if (low == high) {
        return arr[low];
    }
    if ( Math.abs(high - low) == 1 ) {
        return Math.max(arr[low], arr[high]);
    }

    middle = (low + high) >> 1;
    if (arr[middle] > arr[middle + 1]) {
        return arr[middle];
    }
    if (arr[low] > arr[middle]) {
        high = middle - 1;
    } else {
        low = middle + 1;
    }
    return findMax(arr, low, high);

then eliminate a recursive call:

    while (high - low > 2) {
        middle = low + (high - low) / 2;
        if (arr[middle] > arr[middle + 1]) {
            return arr[middle];
        }
        if (arr[low] > arr[middle]) {
            high = middle - 1;
        } else {
            low = middle + 1;
        }
    }
    return Math.max(arr[low], arr[high]);


Periodically comparing the play count of two Last.FM users There is some little program (using a Last.FM API wrapper) which gets the play count of 2 specific users every 5 minutes and appends the difference between these 2 numbers to log file.

Can you review that part of the code?
I think that using the <- operator so many times decreases the readability and simplicity of the code.

main = forever $ do
  diff <- user1 `subtractCounts` user2
  timestamp <- formatTime defaultTimeLocale dateFormat <$> getZonedTime
  let logMessage = printf "%s %d\n" timestamp diff :: String
  homeDir <- getHomeDirectory
  let logFilePath = homeDir </> logFile
  appendFile logFilePath logMessage
  threadDelay $ 10^6 * 60 * timeDelay -- in microseconds
    where
      subtractCounts = liftM2 ((-) `on` fromMaybe 0) `on` getPlayCount
      dateFormat = "[%s] %D %H:%M"

I don't see why you say <- "decreases readability", but you could line them up if you think that helps (this style is typical of Haskell programmers)

main = forever $ do
  diff      <- user1 `subtractCounts` user2
  timestamp <- formatTime defaultTimeLocale dateFormat <$> getZonedTime
  homeDir   <- getHomeDirectory
  let logMessage  = printf "%s %d\n" timestamp diff
      logFilePath = homeDir </> logFile
  appendFile logFilePath logMessage
  threadDelay $ 10^6 * 60 * timeDelay -- in microseconds

subtractCounts = liftM2 ((-) `on` fromMaybe 0) `on` getPlayCount
dateFormat     = "[%s] %D %H:%M"

<- is a visual cue that the thing on the right might to perform an effect before producing a result and binding it to the name on the left.

Note that the type annotation on logMessage is unnecessary. Because it is used as the second argument to appendFile it will be inferred to be String.

I personally might make a few more refactorings, but they aren't really necessary:

logMessage :: String -> Integer -> String
logMessage  = printf "%s %d\n"

(f .: g) x y = f $ g x y

subtractCounts = liftM2 ((-) `on` fromMaybe 0) `on` getPlayCount

main = forever $ do
  homeDir <- getHomeDirectory
  let logFilePath = homeDir </> logFile
      logDiff     = appendFile logFilePath .: logMessage
  forever $ mainLoop logDiff

mainLoop logDiff = do
  diff      <- user1 `subtractCounts` user2
  timestamp <- formatTime defaultTimeLocale "[%s] %D %H:%M" <$> getZonedTime
  logDiff timestamp diff
  threadDelay $ 10^6 * 60 * timeDelay -- in microseconds

The type signature on logMessage is probably unnecessary, but imho it's a good idea to give it an explicit signature if you are using the result of printf as a function, rather than a String or IO ().


Delete orphaned SIDS + change to full control I need to delete all the orphaned SIDs in the ACLs of about 20 shares (between 100GB/6TB) and change full control of users groups to other permissions (modify or read/execute). I have done this script but I'm pretty sure it's easy to improve. Any advice?

$root = "\\192.168.1.1\folder"
$log = "C:\log.txt"

#log creation
"----------SIDS----------" | Out-File $log

#folders list
Get-ChildItem $root -Recurse -Directory | ForEach-Object -Begin {
    $folders = @((Get-Item $root).FullName)
} -Process {
    $folders += $_.FullName
}

##############################DELETE ORPHANED SIDS########################

foreach ($folder in $folders) {
    try {
        $acl = Get-Acl -Path $folder
        foreach ($acc in $acl.access ) {
            $acc_ir = $acc.IdentityReference.Value
            if ($acc_ir -match "S-1-5*") {
                $ACL.RemoveAccessRule($acc) | Out-Null
                Set-Acl -Path $folder -AclObject $acl -ErrorAction Stop
                $hour = Get-Date
                Write-Host -ForegroundColor Green "$hour - $acc_ir deleted in $folder"
                Write-Output "$hour - $acc_ir deleted in $folder" | Out-File -Append $log
            }
        }
    } catch {
        $hour_e = Get-Date
        Write-Host -ForegroundColor Red "$hour_e - error with $acc_ir in $folder"
        Write-Output "$hour_e - error with $acc_ir in $folder" | Out-File -Append $log
    }
}

##############################CHANGE FULLCONTROL########################

"----------FULLCONTROL----------" | Out-File -Append $log

foreach ($folder in $folders) {
    try {
        $acl = Get-Acl -Path $folder
        foreach ($acc in $acl.access ) {
            $acc_ir = $acc.IdentityReference
            $acc_fsr = $acc.FileSystemRights
            if ($acc_ir -match "GROUP" -and $acc_fsr -match "FullControl") {
                $newacc = New-Object System.Security.AccessControl.FileSystemAccessRule($acc_ir, 'Modify', $acc.InheritanceFlags, $acc.PropagationFlags, $acc.AccessControlType)
                $ACL.RemoveAccessRule($acc) | Out-Null
                $ACL.AddAccessRule($newacc) | Out-Null
                Set-Acl -Path $folder -AclObject $acl -ErrorAction Stop
                $new_perm = $newacc.FileSystemRights
                $hour = Get-Date
                Write-Host -ForegroundColor Green "$hour - $folder : the object $acc_ir now have $new_perm"
                Write-Output "$hour - $folder : the object $acc_ir now have $new_perm" | Out-File -Append $log
            }
        }
    } catch {
        $hour_e = Get-Date
        Write-Host -ForegroundColor Red "$hour_e - error with the object $acc_ir in $folder"
        Write-Output "$hour_e - error with the object $acc_ir in $folder" | Out-File -Append $log
    }
}

Should be able to add a little more if you answers my questions in a comment I made on your question. In the mean time there definitely are some points worth bringing to light. I am going to assume you have at least PowerShell v3.0.

Over complicated folder name query

Property Expansion done right

You don't need to build an array for the fullname. Nor do you really need to get the fullname via a directory object for the $root as you already have it. If dot notation does not work Select-Object Fullname would work just as well.

$folders = @($root) + (Get-ChildItem $root -Recurse -Directory).FullName

Since $root is not an array we make it one, just as you did, with @() then simply concatenate the rest of the folder names to make the list of folders.

Avoid array concatenation

This is not a bid deal if you read into my first couple of points but I still want to cover this. It is a memory intensive waste as it destroys and recreates the array adding the new element. Use the pipeline if possible to avoid this.

Regular Expressions

I wanted to draw attention to -match. That operator and others like -replace support regular expressions as well.

In regex "S-1-5*" would match anything that had those characters anywhere in the string and not just the beginning. More importantly it would match strings like "S-1-5555" since 5* matches 0 or as many 5's as it can find. If you wanted to keep using match I would recommend the start of line anchor. In the spirit of what you were trying to do "^S-1-5" would accomplish the same thing. Look for a string that starts with "S-1-5". However the easier solution is just to use -like which is the operator you were, in essence, using

if ($acc_ir -like "S-1-5*")

Double processing

While you have two actions to perform (remove dead sids and change permissions) all the work is still being done on the same folder set. I would not bother looping twice. You could just loop the once and merge your if statements inside the single loop.

foreach ($folder in $folders) {
    try {
        $acl = Get-Acl -Path $folder
        foreach ($acc in $acl.access ) {
            $acc_ir = $acc.IdentityReference
            $acc_fsr = $acc.FileSystemRights
            if ($acc_ir -match "GROUP" -and $acc_fsr -match "FullControl") {
                # Logic for when the group is matched and has fullcontrol
            } elseif ($acc_ir -match "S-1-5*") {
                # Logic for when their is a dead sid.
            }
        }
    } catch {
        # Errors captured here.
    }
}

This would have implications in your log as the entries would no longer be grouped by action but I will cover that later.

Perhaps you prefer the separation

If you didn't want to go the route of the single loop (which you should for performance) I would suggest you use Where-Object in place of your if's as that is in effect what you are doing. Consider you first loop group where you remove the dead sids.

$acl.access | Where-Object{$_.IdentityReference.Value -match "^S-1-5"} | ForEach-Object{
    $acc_ir = $_.IdentityReference.Value
    $acl.RemoveAccessRule($_) | Out-Null
    Set-Acl -Path $folder -AclObject $acl -ErrorAction Stop
    $hour = Get-Date
    Write-Host -ForegroundColor Green "$hour - $acc_ir deleted in $folder"
    Write-Output "$hour - $acc_ir deleted in $folder" | Out-File -Append $log
}

Logging

About Out-File

It is considered one of the weaker means to output text to a file. Add-Content is a good upgrade as it is just as easy to use and performs better. It also is a cmdlet that appends by default and adds a newline to the end of the string automatically. You could also use Set-Content for the first line if you intend to clean the log every time the script is run.

Functions

You should be using at least one function here. You are repeating your blocks of code several times throughout your script. A function would make it easier to read the code and more importantly simpler to make changes to your logging.

function Log-Action($reference,$location,[switch]$LogError){
    $now = (get-Date).ToString()
    If($LogError.IsPresent){
        Write-Host -ForegroundColor Red "$now - error with $reference in $location"
        Write-Output "$now - error with $reference in $location" | Add-Content $log
    } else {
        Write-Host -ForegroundColor Green "$now  - $reference deleted in $location"
        Write-Output "$now  - $reference deleted in $location" | Add-Content $log
    }
}

That would work but some would say that it has a terrible name as it performs two duties.... Logging both errors and information. Also removes the need to save the current time in different variables.

Either way this would be how you would call it in the script...

try {
    $acl = Get-Acl -Path $folder
    foreach ($acc in $acl.access ) {
        $acc_ir = $acc.IdentityReference.Value
        if ($acc_ir -match "S-1-5*") {
            $ACL.RemoveAccessRule($acc) | Out-Null
            Set-Acl -Path $folder -AclObject $acl -ErrorAction Stop
            Log-Action $acc_ir $folder
        }
    }
} catch {
    Log-Action $acc_ir $folder -LogError
}

Objects and CSV output

Something else to consider would be to try and export CSV data so that it would not matter what order your output was as your could organize it as you saw fit with a simple column sort in Excel or similar program.

This one might be harder to show without major changes but here is how it could go.

# Need to use this for each contruct in order to use the pipeline.
$folders | ForEach-Object{
    $folder = $_
    # Build the row
    $result = [pscustomobject][ordered]@{
        Time = (Get-Date).ToString()
        Folder = $folder
        IdentityReference = $null
        Action = "Remove dead SID"
        ActionSuccedded = $false # Assume False

    }

    # reset in case of early failure where this might not get populated
    $acc_ir = $null

    try {
        $acl = Get-Acl -Path $folder
        $acl.access | ForEach-Object{
            $acc = $_
            $acc_ir = $acc.IdentityReference.Value
            if ($acc_ir -match "S-1-5*") {
                # Stuff Happens
                # Log the changes
                $result.Time = (Get-Date).ToString()
                $result.IdentityReference = $acc_ir
                $result.ActionSuccedded = $true
                # send the result through the pipeline
                $result
            }
        }
    } catch {
        # Error has occured.
        $result.Time = (Get-Date).ToString()
        $resulr.IdentityReference = $acc_ir
        # send the result through the pipeline
        $result
    }
} | Export-CSV -NoTypeInformation $log

This way you have highly functional output since we populated a result object with the status of each action and get output that looks something like this

Time                  Folder       IdentityReference                             Action          ActionSuccedded
----                  ------       -----------------                             ------          ---------------
1/23/2016 11:28:17 PM C:\temp      S-1-5-21-1220945662-1202665555-839525555-5555 Remove dead SID            True
1/23/2016 11:28:17 PM C:\temp\test S-1-5-21-1220945662-1204465555-839525555-5555 Remove dead SID           False

Most of my suggestions don't build together but I wanted to cover each one separately in an effort to maintain focus. You will have to forgive me if I lost you on any part of this.


A minimal init script I tried looking up some good example init scripts, but all that I found were excessively complicated. So I wrote my own simple init script targeting CentOS 6. I tried to follow LSB where I understood what it says. CentOS uses chkconfig instead of the LSB comment block.

This is my first serious shell script ever. What do you guys think?

#!/bin/sh
# chkconfig: 2345 80 20
# description: Perforce server

. /etc/profile.d/p4d.sh

command_line='p4d -d'

running() {
    pgrep -fx "$command_line" > /dev/null
}

start() {
    if ! running; then
        $command_line > /dev/null
    fi
}

stop() {
    if running; then
        p4 admin stop > /dev/null
    fi
}

case "$1" in
    start)
        start
    ;;
    stop)
        stop
    ;;
    restart|force-reload)
        stop
        start
    ;;
    status)
        if running; then
            echo "Running"
        else
            echo "Stopped"
            exit 3
        fi
    ;;
    *)
        echo "Usage: $0 {start|stop|restart|force-reload|status}"
        exit 1
    ;;
esac

exit 0

It's clear, nicely formatted, and passes http://www.shellcheck.net/# with flying colors. I have only minor nitpicks.

The variable name command_line is too generic.
If you make it more descriptive,
for example start_command,
then the purpose of the pgrep line becomes easier to understand:
it should match whatever was used to start the command.

This maybe a matter of taste, but I like to place the ;; in case statements at the same indent level as the body of the statement, for example:

case "$1" in
    start)
        start
        ;;       # aligned with the commands, not the label
    stop)

I'm not really aware of a standard, but for what it's worth,
Vim's auto-indent function (and auto-reindent with gg=G) indents this way.
This writing style is also practical when folding is enabled in Vim,
making long case statement blocks appear like this:

case "$1" in
    start)
        --- # folded lines
    stop)
        --- # folded lines
    restart|force-reload)
        --- # folded lines

Well, this is really a nitpick, and a matter of taste,
your way is perfectly fine too.