Marking class with no abstract methods as an abstract Consider an interface:

public interface IWindowOperations
{
    // Some operation methods.
}

Class definitions:

public abstract class BaseWindow<T> : IWindowOperations
{
    // partially implemented class.
}

public class TraditionalWindow<T> : BaseWindow<T>
{
    // Fully implemented class.
}

public class ModernWindow<T> : BaseWindow<T>
{
    // Fully implemented class.
}

(1) is already marked as an abstract so it cannot be instantiated. (2) and (3) are fully implemented though they should not be instantiated, rather they should be derived by other class specifying <T> so the application framework can register them.
e.g.

 public class SliderWindow<Win8> : ModernWindow<Win8>
 {
        // In general case this is just an empty class
        // unless special handling is needed.
 }

In this case is it right to mark (2) and (3) as an abstract? Those are just template defining some concrete functionality but also allows derived classes to override if required.

It depends on your design entirely. If classes (2) and (3) can not operate on their own, should not be instanciated and are there to serve as base classes only, then you are justified to make them abstract, and i think you should.

Also, you should probably reflect your intent in class names by adding Base suffix to (2) and (3) (or prefix, but i think it is less common).

Edit Objects with protected constructor can still be instanciated. For example, you can create them from some method in derived class. It is not a realistic use-case in most cases, but you should not leave such holes nonetheless. If you want to forbid instanciation completely, the only way to do it is to mark the class abstract.


Tetris clone in Java I have a representation for each initial shape as an Enum with points and another for each rotated shape in the same Enum.

Here is an example of the latter Enum:

//
//     * *
//     *
//     *
J_SHAPE_SOUTH(new Point[] {
        new Point(2, 0),
        new Point(2, 1),
        new Point(2, 2),
        new Point(3, 2)
}),

//
//   * *
//     *
//     *
L_SHAPE_SOUTH(new Point[] {
        new Point(1, 2),
        new Point(2, 0),
        new Point(2, 1),
        new Point(2, 2)
});

I was then wondering whether to move the rotated shapes into individual Enums, say RotatedJShape, or simply keep all the rotated shapes in the same Enum. My concern is that some shapes like the I-shape only has one rotated condition, so it seems pointless to have an Enum for that shape.

What would be the most suitable approach in my case in terms of scalability and code correctness?

Ages ago I did a Tetris clone, and my idea for a data structure was this:

A circular linked list of matrices.

How do you rotate?  Walk the list...

You can have a single array of these lists (one per each block type), and your game logic's notion of the "current piece" is a reference to something inside that table.  That way the game logic can have no knowledge of the different shapes, and you declare a small array that defines what the shapes are.


Simple PHP Auth Class feedback this is a basic PHP auth class I've put together for use with simple sites I do from time to time which don't warrant using a framework. I'm wondering if i'm separating out responsibilities all that well, even at this early stage! For example, I'm not sure where I should be filtering user input with mysql_real_escape_string, and I don't know if I should be passing the DB into the constructor, but then I need it to be able to run my auth check.

Anyway, any pointers would be great thanks!

class Auth {

    protected $db;

    /**
     * Auth::__construct()
     *
     * @return
     */
    public function __construct($db = null)
    {
        $this->db = $db;
    }

    /**
     * Auth::login()
     *
     * @param mixed $username
     * @param mixed $password
     * @return
     */
    public function login($username, $password)
    {
        $query = $this->db->query("SELECT username, password
                                    FROM users
                                    WHERE username = '".mysql_real_escape_string($username)."'
                                    AND password = '".self::crypt_pass(mysql_real_escape_string($password))."'
                                  ");

        $result = $this->db->fetch_array($query);

        if (count($result) == 0)
        {
            return FALSE;
        }
        else
        {
            //set session variable
            Session::instance()->set('logged_in', TRUE);
            return TRUE;
        }
    }

    /**
     * Auth::logout()
     *
     * @return
     */
    public function logout()
    {
        //set session variable
        return Session::instance()->delete('logged_in');
    }

    private function crypt_pass($pass)
    {
        if (CRYPT_BLOWFISH == 1)
        {
            $salt = SHA1('acssalty457');
            $blowfish_salt = "\$2a\$07\$".substr($salt, 0, CRYPT_SALT_LENGTH);
            return crypt($pass, $blowfish_salt);
        }
    }
}

This part doesn't make sense to me:

private function crypt_pass($pass)
{
    if (CRYPT_BLOWFISH == 1)
    {
        $salt = SHA1('acssalty457');
        $blowfish_salt = "\$2a\$07\$".substr($salt, 0, CRYPT_SALT_LENGTH);
        return crypt($pass, $blowfish_salt);
    }
}

Functions that only return on condition are a bad idea. You should return something anyway, even if it's false. Now for the specific problem, what if CRYPT_BLOWFISH != 1? It's ok to want to use blowfist, but why cripple the library for everyone else? You could rewrite as:

private function crypt_pass($pass) {
    $salt = "acssalty457";

    if (CRYPT_BLOWFISH == 1) {
        $blowfish_salt = "\$2a\$07\$" . substr($salt, 0, CRYPT_SALT_LENGTH);
        return crypt($pass, $blowfish_salt);
    }

    return sha1($pass . $salt);
}

Notice that I don't hash the salt? That's because security wise that's useless. You can ask around at Cryptography Stack Exchange for more. If you want to make the sha1 crypted pass a little bit harder you could do something weird like:

return sha1(strrev($salt) . $pass . $salt);

I'm using sha1 as an example of a natively included function, you can use any hash algorithm you like.


Search in a 2D array to find if there is a row or a column that contains the word “error” in every cell of that row or column Search in a 2D array to check if there is a column or a row in the 2D Array thats contains all the word “error”, if yes return the index of that row or column else return 0;
Exception: if the row index equals the column index then it does not matter if contains the word "error" or not. FYI, n is the size of 2D array (number of rows = number of columns)

I wrote this code and I want to check with you if you agree with the logic:
int search(int n, String[][] myArray) {
        int j, k;
        boolean isError;

        //To search per ROW
        for (int i = 1; i <= n; i++) {
            j = 1;
            isError = true;

            while (j <= n && isError) {
                if ((i != j) && (myArray[i][j] != "error")) {
                    isError = false;
                }

                if (j == n && isError) {
                    return i;
                } else {
                    j++;
                }
            }
        }//end for loop

        //To search per COLUMN
        for (int i = 1; i <= n; i++) {
            k = 1;
            isError = true;

            while (k <= n && isError) {
                if ((i != k) && (myArray[k][i] != "error")) {
                    isError = false;
                }

                if (k == n && isError) {
                    return i;
                } else {
                    k++;
                }
            }
        }//end for loop

        return 0;

    }

My Java is a bit rusty but I'm not seeing any logical issues with the code doing what you want it to.

Since you return just the row or column index, the caller can't tell from that if the match was a row or a column.


Rewriting WPF GridViewColumn alignment in a less verbose way I wanted just to right align one column in WPF and I found out that the syntax is not exactly syntetic... Is there a way to make it more syntetic?

Consider that this one colum, but I have 20 columns which share the same identical structure, changing only the displayed property.

<GridViewColumn Header="trial" Width="110">
 <GridViewColumn.CellTemplate>
 <DataTemplate>
  <TextBlock HorizontalAlignment="Right">
   <TextBlock.Text>
    <MultiBinding StringFormat="{}{0:N} {1}">
      <Binding Path="Income"></Binding>
      <Binding ElementName="UserControl" Path="DataContext.Pinco"></Binding>
    </MultiBinding>
   </TextBlock.Text>
  </TextBlock>
 </DataTemplate>
 </GridViewColumn.CellTemplate>
</GridViewColumn>

XAML is XAML - I'm not sure what the meaning of "syntetic" is in this context, but the only thing I can see that could improve, is the closing tags for <Binding> - the empty elements can be collapsed:

<MultiBinding StringFormat="{}{0:N} {1}">
  <Binding Path="Income" />
  <Binding ElementName="UserControl" Path="DataContext.Pinco" />
</MultiBinding>

There aren't 20 different ways of specifying the horizontal alignment of a TextBlock:

<TextBlock HorizontalAlignment="Right">

I don't see anything wrong with this.

Actually the indentation could use more spaces, too - but that's cosmetic (and probably just a copy/paste glitch with the tabs):

<GridViewColumn Header="trial" Width="110">
    <GridViewColumn.CellTemplate>
        <DataTemplate>

            <TextBlock HorizontalAlignment="Right">
                <TextBlock.Text>
                    <MultiBinding StringFormat="{}{0:N} {1}">
                        <Binding Path="Income" />
                        <Binding ElementName="UserControl" Path="DataContext.Pinco" />
                    </MultiBinding>
                </TextBlock.Text>
            </TextBlock>

        </DataTemplate>
    </GridViewColumn.CellTemplate>
</GridViewColumn>

I like using vertical whitespace to separate things - it makes the markup easier to read, and maintain.

EDIT: Considering that there would be 20x such markup, with the only thing changing being the <Binding Path="Income" /> part, as @GeorgeHowarth commented you can look into making the DataTemplate a StaticResource, like this.


Datetimepicker script I'm working on my way to write most efficient, reusable and robust code, and this is why I'd like to receive some support. I've written a datetimepicker script that allows datetime chosing, and a shortcut button that will subtract from higher date (the right input) a variable number of minutes:

Is there a way to simplify my code even more?

<form>
  <input type="text" name="poczatek" id="od" />
  <input type="text" name="koniec" id="do" />
  <input type="button" value="10" class="odejmijCzas"/>
  <input type="button" value="20" class="odejmijCzas"/>
  <input type="button" value="30" class="odejmijCzas"/>
</form>

JavaScript

dateTimeFormat = new Object();
dateTimeFormat.dwucyfrowa = function(m) {
    return m < 10 ? '0' + m : m;
}
dateTimeFormat.format = function(date) {
    return date.getFullYear() + '-' + dateTimeFormat.dwucyfrowa(date.getMonth() + 1) + '-' + dateTimeFormat.dwucyfrowa(date.getDate()) + ' ' + dateTimeFormat.dwucyfrowa(date.getHours()) + ':' + dateTimeFormat.dwucyfrowa(date.getMinutes());
}
$(document).ready(function() {
    inputOd = $("#od");
    inputDo = $("#do");

    inputOd.add(inputDo).datetimepicker({
        changeMonth: 'true'
    });

    currentTime = new Date();
    inputOd.add(inputDo).val(dateTimeFormat.format(currentTime));

    $('.odejmijCzas').click(function() {
        odejmijCzas($(this).attr('value'));
    });
});

function odejmijCzas(liczbaMinut) {
    czasDo = new Date(inputDo.val());
    roznicaCzasu = czasDo.getTime() - liczbaMinut * 1000 * 60;
    czasOd = new Date();
    czasOd.setTime(roznicaCzasu);
    inputOd.val(dateTimeFormat.format(czasOd));
}

A quick review:

High Level Overview

This code is okay except for 2 major items: you are not declaring your variables, making them escape all scope. And you use non-English variable names and function names, not every developer speaks Polish, though we should all understand English.

Naming

English is the one true language for naming, please find English equivalents for odejmijCzas, roznicaCzasu etc.

Idioms and Custom

Do not use = new Object();, use = {}

Scope

You must declare everything with var, let, or const
I would declare dateTimeFormat and odejmijCzas inside $(document).ready(function() { to keep the namespace clean

JSHint.com

Consider using http://jshint.com/ to perfect your code, most of these items were found by this tool as well


3 layers architecture in C# I will build a system in C# and I want to use the three layers architecture is next:

Entity Layer

namespace EntitiesLayer
{
    public class ECategoria
    {
        private int _idcategoria;
        private string categoria;
        private string descripcion;
    public int Idcategoria
    {
        get { return _idcategoria; }
        set { _idcategoria = value; }
    }

    public string Categoria
    {
        get { return categoria; }
        set { categoria = value; }
    }

    public string Descripcion
    {
        get { return descripcion; }
        set { descripcion = value; }
    }

Data Access Layer

public class DCategoria
{
    public string Act_Categoria(ECategoria categoria)
    {
        string rpta = "";
        DataSet ds = new DataSet();

        try
        {
        string query = "SP_ACTCATEGORIA";
        Dictionary<string, object> parameters = new Dictionary<string, object>();
        parameters.Add("@idcategoria", categoria.Idcategoria);
        parameters.Add("@categoria", categoria.Categoria);
        parameters.Add("@descripcion", categoria.Descripcion);
        ds = dbconnection.execute_query(query, parameters);
        if (ds == null)
        {
            rpta = "Error Insertando";
        }
        else
        {
            rpta = "REGISTRO REGISTRADO/ACTUALIZADO EXITOSAMENTE";
        }
    }
    catch (Exception ex)
    {
        rpta = ex.Message;
    }

    return rpta;
}

Business Logic Layer

public class BCategoria
{
    private DCategoria data_categoria = new DCategoria();

public string Act_categoria(ECategoria categoria)
{
    return data_categoria.Act_Categoria(categoria);
}

public DataSet Select_categoria()
{
    return data_categoria.Select_Categoria();
}

public string Delete_categoria(ECategoria categoria)
{
    return data_categoria.Delete_Categoria(categoria);
}

public DataSet FilterbyID(ECategoria categoria)
{
    return data_categoria.FilterbyID(categoria);
}

public DataSet FilterByName(ECategoria categoria)
{
    return data_categoria.FilterByName(categoria);
}

Presentation Layer

public ECategoria get_data()
{
    ECategoria categoria = new ECategoria();
    categoria.Idcategoria = Convert.ToInt32(this.txtcodigo.Text.Trim());
    categoria.Categoria = this.txtcategoria.Text.Trim();
    categoria.Descripcion = this.txtcategoria.Text.Trim();
    return categoria;
}

private void btnNuevo_Click(object sender, EventArgs e)
{
    categoria = get_data();
    rpta = bus_categoria.Act_categoria(categoria);
}

Is this a good implementation of the three layers architecture?

Few notes about naming conventions (I do not repeat for each method/class but they apply to all of them).

Namespace EntitiesLayer does not help to categorize your code. Namespace are hierarchical and here you're not using them in that way. Obviously it's not mandatory but something like Juan.AwesomeProduct.Data, Juan.AwesomeProduct.Model and Juan.AwesomeProduct.Presentation may help you to organize your code. I am shameless and I admit that at the very beginning (with Managed Extensions for C++, you're lucky if you never had to use it) I used namespaces like you're doing and I still regret.

Classes like ECategoria should not be prefixed with E (or C or anything else.) That's what namespaces are for.

Idcategoria is a public property then it should be PascalCase IdCategoria.

_idcategoria is underscore prefixed but descripcion and categoria are not. Pick a convention (with or without prefix) but be consistent. There are good reasons both to use it or to avoid it, have an informed decision and don't look back.

You're not validating any value in your entities layer. Is acceptable to have null for Descripcion? A negative ID? A blank string?

I see no reason for ECategoria to do not be sealed.

Also take care of indentation.

DCategoria (again, remove that prefix) is an utility class. More often than not you do not need it. Its responsibility may be moved to another class that does something else. It may also be a generic class for many entities. Something like a repository, reconsider that name. Also consider to make it sealed (do you want to extend it through inheritance? Where extension points are?)

Usually underscore is not used to separate words, Act_Categoria() should be PascalCase (it's a public method) then ActCategoria() may be more appropriate. However you're mixing two languages. Use English or your own language but do not mix them in code. If you can you may write everything in English and keep comments in your own language (if you work in an company they should have a procedure for this.)

rpta and ds are declared at the beginning of your method. In C# you can (and you should) declare them as late as possible. Ideally in the same moment you initialize them. Let's see a proof of concept which also includes some others tips:

public string Act_Categoria(ECategoria categoria)
{
    try
    {
        var parameters = new Dictionary<string, object>();
        parameters.Add("@idcategoria", categoria.Idcategoria);
        parameters.Add("@categoria", categoria.Categoria);
        parameters.Add("@descripcion", categoria.Descripcion);
        const string query = "SP_ACTCATEGORIA";
        var ds = dbconnection.execute_query(query, parameters);

        return ds == null ? "Error Insertando" : "REGISTRO REGISTRADO/ACTUALIZADO EXITOSAMENTE";
    }
    catch (Exception ex)
    {
        return ex.Message;
    }
}

Note that you're catching Exception. It's a very bad thing to do (I won't repeat here the reasons, do a quick search of check any of my previous reviews here.) Catch the exceptions you know may happen and you want to ignore. NullReferenceException is a programming error, not a run-time transitory error condition you may want to swallow.

In your business layer data_categoria may be readonly. In this moment and with this code I do not see any reason to have a BCategoria class. It just forwards calls to DCategoria. Drop one of them until you will actually need it. Business logic should add some value, what's the value of a tunnel? You still expose the same model (ECategoria) and you just forward calls. In a somehow simple scenario you may merge business layer and data layer but when things are more complex the business layer should have its own model (it is the model...) I'd suggest to do not make things complicate if unnecessary, maybe a 2-tiers architecture is what you should start with.

Presentation layer is just a proof of concept however (besides naming convention) it's where you should put more logic. Business layer and Presentation Layer are what will change more often (and probably independently) but here you're directly calling business layer without any abstraction. How do you test your user interface without the attached business layer? What will happen when you need, for example, a IsChanged property in ECategoria? Will you add it in the shared class even if it's used only in presentation? Check MVC, MVP and MVVM to pick the one that best suits your needings (and your UI framework).

Also note that this line:

categoria.Idcategoria = Convert.ToInt32(this.txtcodigo.Text.Trim());

May fail (and trimming is optional here.) User input may be wrong and you must handle that, proof of concept:

int categoryId;
if (Int32.TryParse(this.txtCodigo.Text, out categoryId))
    categoria.CategoryId = categoryId;
else
    ; // Ooops input is not a number (formatted according to current culture)


Effective use of multiple streams I am experimenting with streams and lambdas in Java 8. This is my first serious foray using functional programming concepts; I'd like a critique on this code.

This code finds the Cartesian product of three sets of ints. My questions/complaints about this snippet are:

Are nested flatMaps really the best (most understandable, most CPU/memory efficient) way to do an arbitrary Cartesian product? Or should I stick to imperative style for Cartesian products?
Is there a better way to format nested lambdas (see, e.g., my use of flatMap)?

I just learned about Collectors; those might be what I need. I'm still learning how to best combine these things.

long count = IntStream.rangeClosed(0,9) /* 0 <= A <= 9 */
    .parallel()
    .mapToObj(Integer::valueOf)
    .flatMap(a ->
            IntStream.rangeClosed(0,9) /* 0 <= B <= 9 */
            .mapToObj(Integer::valueOf)
            .flatMap(b ->
                    IntStream.rangeClosed(0,9) /* 0 <= C <= 9 */
                    .mapToObj(c -> (new Product()).A(a).B(b).C(c))
            )
     ).count();
System.out.println("Enjoy your Cartesian product." + count);

UPDATE: I deliberately omitted boilerplate code and class/method definitions, for brevity. The only important thing about Product is that it stores three ints.

Your code is odd in the sense that it is going to a lot of effort to calculate that 103 is 1000.

I understand why you are doing it, but I took the liberty of changing the count() terminating function and replacing it with:

StringBuilder sb = new StringBuilder();

.......
.forEachOrdered(prod -> sb.append(prod.toString()).append("\n");

This way we can store each result, instead of just counting it.

There are a few things to go through here.

Product

This class looks awkward:

 .mapToObj(c -> (new Product()).A(a).B(b).C(c))

Why not just have a useful constructor on Product like:

    public Product(int a, int b, int c) {
        super();
        this.a = a;
        this.b = b;
        this.c = c;
    }

Then, your mapping would look like:

 .mapToObj(c -> new Product(a, b, c))

Magic Numbers

You have 0 and 9 as magic numbers in multiple places. These should be declared as (effective) constants, or calculated somehow. With the 9 value especially, if you want to change the size of the product you have to change it in three places.

int vs. Integer

You start off with an IntStream, but then convert it to a Stream<Integer>. Why? If you can leave things as primitives, you should.

The actual stream

The most concerning aspect is the stream itself.

The flatMap operation is not doing what I think you think it does....

Let's consider the contents of the stream as it goes through things:

'a' range from 0-9 IntStream
convert each int to an Integer
for each Integer, flatMap that Integer
the flatMap creates an IntStream which it converts to a Stream
even though we are flatMapping the 'a' value, we are not using the 'a' value in the mapping. All we are really doing is creating a new 'b' value, and who cares that the stream now has the b values streaming through....
with these 'b' values on the stream, we then flatMap again....
again, we ignore what the actual 'b' value is, abut we generate a new 'c' value in a third nested stream, and then we combine the 'a' and 'b' values which are 'in scope', with the 'c' value from the stream, and we make a Product.

The point of what I am saying is that you actually are only using the Stream in the most inside IntStream, the 'c' stream. All the other streams are just 'tokens' that you need to count the events you are doing. A for-loop is the right thing for this construct (if it was not for the parallelism). If you include the parallelism, then a fork-join process is right. Not a stream.

My Attempt

I am not claiming this is right, or best practice, but consider this attempt to get essentially the same result as you:

/** Append an int to an array after the last array item **/
public static final int[] append(int[] base, int val) {
    int[] ret = Arrays.copyOf(base, base.length + 1);
    ret[base.length] = val;
    return ret;
}

/** Stream an int range and map to an appended array */
private static final Stream<int[]> appendInts(int[] base, int size) {
    return IntStream.rangeClosed(0, size).mapToObj(a -> Cartesian.append(base, a));
}

public static void main(String[] args) {

    final int last = 9;
    final StringBuilder sb = new StringBuilder();
    IntStream.rangeClosed(0,last) /* 0 <= A <= 9 */
             .parallel()
             .mapToObj(a  -> new int[]{a})
             // stream is int[1] array
             .flatMap(ab  -> appendInts(ab, last))
             // stream is int[2] array
             .flatMap(abc -> appendInts(abc, last))
             // stream is int[3] array
             .forEachOrdered(res -> sb.append(Arrays.toString(res)).append("\n"));;
    System.out.println("Enjoy your Cartesian product." + sb.toString());
}

Update

I have rewritten the OP's code to extract the stream preparations. Consider the following code:

    // convert two input values to a stream consisting of an arrays of int
    BiFunction<Integer, Integer, Stream<int[]>> stage3 = (a,b) -> {
        return IntStream.rangeClosed(0, last).mapToObj(c -> new int[]{a, b, c});
    };

    // convert an input value to a stream consisting of an arrays of int
    Function<Integer, Stream<int[]>> stage2 = a -> {
        Stream<Integer> s = IntStream.rangeClosed(0, last).boxed();
        Stream<int[]> ret = s.flatMap(b -> stage3.apply(a, b));
        return ret;
    };

    IntStream.rangeClosed(0,last) /* 0 <= A <= 9 */
            .parallel()
            .boxed()
            .flatMap(a -> stage2.apply(a))
            .map(r -> Arrays.toString(r))
            .forEachOrdered(System.out::println);

Which is essentially what the OP's code does.


ASP.NET REST controller with try-catch error handling Yesterday someone guided me through this article and I'm wondering whether I should use try-catch implementation or not? I have the following code below which is actually a RESTful service and I have implemented the try-catch pattern. I'm throwing errors only in debug mode just for testing purpose and during the release mode, I'm catching errors and returning the meaningful error messages such as Http Status Codes.

After reading this article, I'm confused whether I'm on the right track and if not, do I need to remove try-catch and throw errors instead of such meaningful messages to the end user?

Any code review/feedback is appreciated.

namespace NewEmployeeBuddy.Facade.Controllers.Employee
{
    [RoutePrefix("api/employee")]
    [RequireHttps]
    [BasicAuthorization]
    public class EmployeeController : ApiController
    {
        #region Properties
        private IEmployeeService _employeeService { get; set; }
        #endregion

        #region Constructor
        public EmployeeController()
        {
          //_employeeService = new EmployeeService();
        }

        public EmployeeController(IEmployeeService employeeService)
        {
            _employeeService = employeeService;
        }
        #endregion

        #region Actions
        [HttpPost, Route("add")]
        [ValidateRequest]
        public IHttpActionResult AddEmployee(AddEmployeeRequestDTO request)
        {
            try
            {
                if (request == null)
                    return Content(HttpStatusCode.BadRequest, ErrorCodes.E002);

                var response = _employeeService.AddEmployee(request);
                if (response.OperationResult)
                    return Content(HttpStatusCode.Created, response);

                return Content(HttpStatusCode.InternalServerError, ErrorCodes.E001);
            }
            catch (Exception ex)
            {
#if (DEBUG)
                Logger.Instance.Error(ex);
                throw ex;

#endif
                //Logger.Instance.Error(ex);
                //return Content(HttpStatusCode.InternalServerError, ErrorCodes.E001);
            }
        }
        #endregion
    }
}

throw ex;

This is a fatal mistake. It'll create a new stack-trace and you won't be able to tell where the actual exception occured. If you want to rethrow it then use just

throw;

without  ex.


Simple C# HashTable Implementation Haven't put one of these together since college. How does this look overall?

public class HashTable<T, TU>
    {
        private LinkedList<Tuple<T, TU>>[] _items;
        private int _fillFactor = 3;
        private int _size;

        public HashTable()
        {
            _items = new LinkedList<Tuple<T, TU>>[4];
        }

        public void Add(T key, TU value)
        {
            var pos = GetPosition(key, _items.Length);
            if (_items[pos] == null)
            {
                _items[pos] = new LinkedList<Tuple<T, TU>>();
            }
            if (_items[pos].Any(x=>x.Item1.Equals(key)))
            {
                throw new Exception("Duplicate key, cannot insert.");
            }
            _size++;
            if (NeedToGrow())
            {
                GrowAndReHash();
            }
            pos = GetPosition(key, _items.Length);
            if (_items[pos] == null)
            {
                _items[pos] = new LinkedList<Tuple<T, TU>>();
            }
            _items[pos].AddFirst(new Tuple<T, TU>(key, value));
        }

        public void Remove(T key)
        {
            var pos = GetPosition(key, _items.Length);
            if (_items[pos] != null)
            {
                var objToRemove = _items[pos].FirstOrDefault(item => item.Item1.Equals(key));
                if (objToRemove == null) return;
                _items[pos].Remove(objToRemove);
                _size--;
            }
            else
            {
                throw new Exception("Value not in HashTable.");
            }
        }

        public TU Get(T key)
        {
            var pos = GetPosition(key, _items.Length);
            foreach (var item in _items[pos].Where(item => item.Item1.Equals(key)))
            {
                return item.Item2;
            }
            throw new Exception("Key does not exist in HashTable.");
        }

        private void GrowAndReHash()
        {
            _fillFactor *= 2;
            var newItems = new LinkedList<Tuple<T, TU>>[_items.Length * 2];
            foreach (var item in _items.Where(x=>x != null))
            {
                foreach (var value in item)
                {
                    var pos = GetPosition(value.Item1, newItems.Length);
                    if (newItems[pos] == null)
                    {
                        newItems[pos] = new LinkedList<Tuple<T, TU>>();
                    }
                    newItems[pos].AddFirst(new Tuple<T, TU>(value.Item1, value.Item2));
                }
            }
            _items = newItems;
        }

        private int GetPosition(T key, int length)
        {
            var hash = key.GetHashCode();
            var pos = Math.Abs(hash % length);
            return pos;
        }

        private bool NeedToGrow()
        {
            return _size >= _fillFactor;
        }
    }

A few small points:

public class HashTable<T, TU>

Use more mnemonic names, like TKey, TValue here.

public class HashTable<T, TU>

The name indicates how the class is implemented; name it after what it does logically. This is a map, or a dictionary, or a key-value store, or whatever you want to call it.  That it is a hash table is a mechanism; name classes after their semantics.

public class HashTable<T, TU>

I would expect a map to implement appropriate generic interfaces IDictionary, IEnumerable, ICollection, and so on.

private int _fillFactor = 3;

Why 3? Explain to the reader what is going on here.

_items = new LinkedList<Tuple<T, TU>>[4];

Why 4?

throw new Exception("Duplicate key, cannot insert.");

Throw more specific exceptions.

        if (_items[pos] == null)
        {
            _items[pos] = new LinkedList<Tuple<T, TU>>();
        }
        if (_items[pos].Any(x=>x.Item1.Equals(key)))
        {

That could be an else if.

        if (NeedToGrow())
        {
            GrowAndReHash();
        }

You never call NeedToGrow and GrowAndRehash separately, which indicates to me that they are the same thing. You could make one method called GrowAndRehashIfNecessary().

public TU Get(T key)
public void Add(T key, TU value)

You might also implement an indexer.

var newItems = new LinkedList<Tuple<T, TU>>[_items.Length * 2];

The number of array slots will always be a power of two, and therefore you are taking the bottom n bits of the hash as your hash key.  Depending on how the data is distributed and the quality of the hash codes given to you, that might lead to a fair number of collisions. You'd want to try this out on real data and see how it goes.

public void Add(T key, TU value)
public void Remove(T key)

Add resizes and rehashes if the table gets too full, but Remove does not resize and rehash if the table gets too empty. Apparently you have made the implicit and undocumented design decision that many adds are more likely than many removes, and that in the case where many removes happen, it is OK to waste memory.  Was that decision intentional, or an accident?

Finally, consider studying the implementation of the Dictionary type; the .NET sources are available and the dictionary hash table implementation is entertaining reading. You get a sense of how feature-complete and robust these things really have to be to be a workhorse of the base class libraries.


My lru_cache implementation This code is intended to function exactly like functools.lru_cache. Is there anything I could improve in design, implementation, style, or any other area? (Python version = 3.6.*).

from collections import OrderedDict
from functools import wraps

def lru_cache(maxsize=None):
    def decorator(func):

        cache = OrderedDict()
        @wraps(func)
        def decorated(*arg, **kwargs):
            key = (*arg, None, *kwargs)
            try:
                val = cache[key]
                del cache[key]
            except KeyError:
                val = func(*arg, **kwargs)

            cache[key] = val
            if maxsize and len(cache) > maxsize:
                cache.popitem(last=False)

            return val

        return decorated
    return decorator

You have quite a large bug with how you handle kwargs.

You don't care about the value, only that the key exists.
You care about the order of the keywords.
You don't care about default keywords.

To test this, we can make a simple function to see what your keys are.
With some simple tests.

def test(*args, **kwargs):
    return (*args, None, *kwargs)

print(test(a=1)) # (None, 'a')
print(test(a=2)) # (None, 'a')
print(test(a=1, b=2)) # (None, 'a', 'b')
print(test(b=2, a=1)) # (None, 'b', 'a')

And so you should instead sort your kwargs.items().
However to correctly change args and kwargs to a single standard that the function will take is a little bit more complex.
The simplest way to do this is to use inspect.signature to get something like:

def test(fn):
    signature = inspect.signature(fn)
    def call(*args, **kwargs):
        bind = signature.bind(*args, **kwargs)
        bind.apply_defaults()
        return bind.args, tuple(sorted(bind.kwargs.items()))
    return call

def fn1(a, b, c=1, *, d=2, **kwargs):
    pass

def fn2(a, b, *args, c=1, d=2, **kwargs):
    pass

# Checking that it works as intended
for fn in (fn1, fn2):
    t = test(fn)
    print(t('a', 'b'))
    print(t('a', 'b', 'c'))
    print(t(a='a', b='b', c='c'))
    print(t(b='b', c='c', a='a'))
    print(t('a', 'b', d='d'))
    print(t('a', 'b', test='test'))
    print(t('a', 'b', t1='t1', t2='t2'))
    print(t('a', 'b', t2='t2', t1='t1'))

And so I'd use:

from collections import OrderedDict
from functools import wraps
from inspect import signature

def lru_cache(maxsize=None):
    def decorator(func):
        func_sig = signature(func)
        cache = OrderedDict()
        @wraps(func)
        def decorated(*arg, **kwargs):
            bind = func_sig.bind(*args, **kwargs)
            bind.apply_defaults()
            args, kwargs = bind.args, bind.kwargs
            key = (arg, tuple(sorted(kwargs.items())))
            try:
                val = cache[key]
                del cache[key]
            except KeyError:
                val = func(*arg, **kwargs)

            cache[key] = val
            if maxsize and len(cache) > maxsize:
                cache.popitem(last=False)

            return val

        return decorated
    return decorator

Adding typed is also fairly easy and comes down to making more tuples with an extra value of type. Which I'm fairly sure you're capable of doing.


Running arbitrary number of mongo queries in nodejs I am faced yet again with a callback headache:

Simply I need to return to the client the results of multiple mongo queries, the number of which I don't know in advance.
In short:

fetch the first document in a collection
if the document has some
fields with alphabetic values, fetch all distinct values

Since I can only do the next query in the result callback of the previous one, that's what am doing keeping track of how many fields I have queried so far:

function getFieldsFromCollection (collection, socket) {
  MongoClient.connect(MAIN_DB, function (err, db) {
    if (err) {
      return console.log(err)
    }
    var c = db.collection(collection)
    c.findOne({}, {}, function (err, doc) {
      if (err) {
        return log(err)
      }
      if (!doc) {
        return log('document is null')
      } else {
        var alphaFields = []
        Object.keys(doc).forEach(function (key) {
          if (/^[A-Z]/.test(doc[key])) {
            alphaFields.push(key)
          }
        })
        // prepare the response
        var reply = {
          alphaFields: alphaFields,
          alphaFieldsValues: {}
        }
        fetchDistinctValues(c, alphaFields, 0, reply, socket)
      }
    })
  })
}

function fetchDistinctValues(collection, fields, index, reply, socket) {
  if (index < fields.length) {
    collection.distinct(fields[index], function (err, docs) {
      reply.alphaFieldsValues[fields[index]] = docs
      fetchDistinctValues(collection, fields, ++index, reply, socket)
    })
  } else {
    socket.emit('getFieldsFromCollectionReply', reply)
  }
}

Needless to say this feels very hackysh..
I've looked into some libraries or promises but they seem to offer solution for when one knows exactly how many queries have to be made.
Please tell me there is a better way

I'm pretty sure Mongo has native Promise support, that in conjunction with async/await would really tidy this up

async getFieldsFromCollection (collection, socket) {
  try {
    const db = await MongoClient.connect(MAIN_DB);
    const c = db.collection(collection);
    const doc = await c.findOne({}, {});
    if (!doc) throw new Error('document is null');

    const alphaFields = Object.keys(doc).filter(k => /^[A-Z]/.test(doc[k]));
    const alphaFieldsValues = {};
    const subQueries = alphaFields.map(async (f) => {
      const docs = await c.distinct(f);
      alphaFieldsValues[f] = docs;
    });
    await Promise.all(subQueries);
    return socket.emit('getFieldsFromCollectionReply', {
      alphaFields,
      alphaFieldsValues
    });
  } catch (e) {
    return log(e);
  }
}


A* search algorithm: open set and heap I'm working on an A* search algorithm implemention and this is what I came up with for the open list:

from heapq import heappush, heappop

class OpenList(set):
    '''
    This uses a heap for fast retrieval of the node with the lowest path score.
    It also uses a set for efficiently checking if a node is contained.
    '''

    REMOVED = object()

    def __init__(self, *args, **kwargs):
        set.__init__(self, *args, **kwargs)
        self._heap = []

    def add(self, node):
        set.add(self, node)

        heappush(self._heap, (node.get_path_score(), node))

    def remove(self, node):
        set.remove(self, node)

        i = self._heap.index((node.get_path_score(), node))
        self._heap[i] = self.REMOVED

    def pop_node_with_lowest_path_score(self):
        ''' remove and return the node with the lowest path score '''

        item = heappop(self._heap)
        while item is self.REMOVED:
            item = heappop(self._heap)
        set.remove(self, item[1])
        return item[1]

Should I keep the path_score in the heap, when I mark the item as removed as in the following code:

self._heap[i] = (node.get_path_score(), self.REMOVED)

What do you think about it? What can be made more efficient, simple or cleaner?

1. Bug

You don't properly synchronize the set and the heap in all cases. For example, when taking a union:

>>> a = OpenList()
>>> a |= set([1,2,3])
>>> a
OpenList([1, 2, 3])
>>> a._heap
[]

If you insist on your class being a subclass of set, then you probably ought to intercept every method that modifies the set (and there are quite a few: __isub__, __ior__, __ixor__, __iand__, discard, difference_update, intersection_update, symmetric_difference_update, update, and clear, not to mention copy) and ensure that the heap is modified in a corresponding fashion.

But this would be overkill for what ought to be a fairly simple class. What I would do instead would be to inherit from object instead of set, and implement the set as an instance member _set, in the same way that you have _heap as an instance member. With this approach, you can ensure that operations on _set always have corresponding operations on _heap so that they cannot get out of sync.

2. Other issues

Your class is not as general as it could be. In particular, it can only be used to store objects which have a get_path_score method. Why not allow the user to pass in a key function that gets the score for an object? (As in sorted and heapq.nlargest and similar functions.)
You call the superclass method directly via set rather than via the super() function. This means that it's not possible for someone to create a new class that subclasses both your class and some other class C. (Because the method resolution order will go directly from your class to set instead of via the other class C.)
In the remove method, you replace the removed item with a placeholder. This takes time O(n) because you have to find the item in the heap. But removing the item from the heap and re-heapifying would also take O(n), and it would be simpler (because then there would be no need to handle the placeholder item in the pop method). So it's not clear to me that your approach is worth it.

You should think about what you are trying to achieve: if you really want to have fast removal of items, then you need to be cleverer about how you find the removed item in the heap; but if you're happy for removal to be O(n), then you can simplify the code by just deleting the item from the heap and re-heapifying the remainder.

3. Naming and documentation

The data structure used by the A* search algorithm is usually known as a priority queue: that is, a collection of items each of which is associated with a priority (or score), and which can be retrieved in order of their priority. So I think you could choose a better name than OpenList for your class.
Your docstring for the class is written with the wrong audience in mind. It contains implementation details ("uses a heap ... also uses a set") but this is not helpful for users of the class, who want to know what it is for and how to use it.
Your class is a good candidate for a doctest or two.
The method name pop_node_with_lowest_path_score is absurd. Why not just pop?

4. Rewrite

Here's how I'd rewrite the class to fix all the issues discussed above, assuming that you really do want fast removal of items. I'll let you reverse-engineer it to figure out how it works!

from heapq import heappush, heappop

class PriorityQueueElement(object):
    """
    A proxy for an element in a priority queue that remembers (and
    compares according to) its score.
    """
    def __init__(self, elem, score):
        self._elem = elem
        self._score = score
        self._removed = False

    def __lt__(self, other):
        return self._score < other._score

class PriorityQueue(object):
    """
    A priority queue with O(log n) addition, O(1) membership test and
    amortized O(log n) removal.

    The `key` argument to the constructor specifies a function that
    returns the score for an element in the priority queue. (If not
    supplied, an element is its own score).

    The `add` and `remove` methods add and remove elements from the
    queue, and the `pop` method removes and returns the element with
    the lowest score.

    >>> q = PriorityQueue([3, 1, 4])
    >>> q.pop()
    1
    >>> q.add(2); q.pop()
    2
    >>> q.remove(3); q.pop()
    4
    >>> list(q)
    []
    >>> q = PriorityQueue('vext cwm fjord'.split(), key = lambda s:len(s))
    >>> q.pop()
    'cwm'
    """
    def __init__(self, *args, **kwargs):
        self._key = kwargs.pop('key', lambda x:x)
        self._heap = []
        self._dict = {}
        if args:
            for elem in args[0]:
                self.add(elem)

    def __contains__(self, elem):
        return elem in self._dict

    def __iter__(self):
        return iter(self._dict)

    def add(self, elem):
        """
        Add an element to a priority queue.
        """
        e = PriorityQueueElement(elem, self._key(elem))
        self._dict[elem] = e
        heappush(self._heap, e)

    def remove(self, elem):
        """
        Remove an element from a priority queue.
        If the element is not a member, raise KeyError.
        """
        e = self._dict.pop(elem)
        e._removed = True

    def pop(self):
        """
        Remove and return the element with the smallest score from a
        priority queue.
        """
        while True:
            e = heappop(self._heap)
            if not e._removed:
                del self._dict[e._elem]
                return e._elem


Extracting opening and closing times for each day of the week I have these two methods:

private static OpeningDay GetUsallyOpeningHour(List<OpeningDay> openingHoursDaily, DayOfWeek dayOfWeek)
{
    var listOfSpecificDayOfWeek = openingHoursDaily.Where(x => x.DateTime.DayOfWeek == dayOfWeek);

    if(listOfSpecificDayOfWeek.Any())
        return listOfSpecificDayOfWeek.GroupBy(x => x.From).OrderByDescending(x => x.Count()).ThenByDescending(x => x.Key).First().First();

    return new OpeningDay { From = new TimeSpan(0, 0, 0), To = new TimeSpan(0, 0, 0), DateTime = DateTime.Now };
}

private static OpeningDay GetUsallyClosingHour(List<OpeningDay> openingHoursDaily, DayOfWeek dayOfWeek)
{
    var listOfSpecificDayOfWeek = openingHoursDaily.Where(x => x.DateTime.DayOfWeek == dayOfWeek);

    if (listOfSpecificDayOfWeek.Any())
        return listOfSpecificDayOfWeek.GroupBy(x => x.To).OrderByDescending(x => x.Count()).ThenByDescending(x => x.Key).First().First();

    return new OpeningDay { From = new TimeSpan(0, 0, 0), To = new TimeSpan(0, 0, 0), DateTime = DateTime.Now };
}

The only difference between them is that one is grouping by x.From and other by x.To, and I was wondering how I can improve this code.

Basically, I can a create method which takes lambda, but how long? At first I was thinking to pass this:

GroupBy(x => x.To).OrderByDescending(x => x.Count()).ThenByDescending(x => x.Key)

as a lambda parameter. But this would take me ending with function that takes a lambda and then returns sth like lambda.First().First().

What do you recommend? Are there some other improvements?

Your thinking is good: it's a small difference but... there is a difference. And you have to find a way to extract the difference out of the methods and try create a parameter from this. Small example:

public class User
{
    public int Id { get; set; }
    public string Name { get; set; }
    public int Value { get; set; }
}

We could then group them by Id or Value for example:

public Item GetItemGroupedByName(List<Item> items)
{
    return items.GroupBy(x => x.Id).First().First();
}

public Item GetItemGroupedByValue(List<Item> items)
{
    return items.GroupBy(x => x.Value).First().First();
}

This gives the same kind of situation like yours. Luckily there's the Func<T, TResult> Delegate. The method can now look like this:

public Item GetItemGrouped(List<Item> items, Func<Item, int> rule)
{
    return items.GroupBy(rule).First().First();
}

And you call it like this:

GetItemGrouped(list, x => x.Value);
//or:
GetItemGrouped(list, x => x.Id);

Of course you can't group by Name in this case as it is a string property and the Func expects an integer. You might change the int to object, but please don't as that is hacky code. Since the purpose of your method is to group by a TimeSpan, you can implement this easily:

private static OpeningDay GetUsallyOpeningHour(List<OpeningDay> openingHoursDaily,
                                               DayOfWeek dayOfWeek,
                                               Func<OpeningDay, TimeSpan> groupByRule)
{
    var listOfSpecificDayOfWeek = openingHoursDaily.Where(x => x.DateTime.DayOfWeek == dayOfWeek);

    if(listOfSpecificDayOfWeek.Any())
        return listOfSpecificDayOfWeek.GroupBy(groupByRule)
                                      .OrderByDescending(x => x.Count())
                                      .ThenByDescending(x => x.Key)
                                      .First().First();

    return new OpeningDay { From = new TimeSpan(0, 0, 0), To = new TimeSpan(0, 0, 0), DateTime = DateTime.Now };
}

And now you can call the method like:

var openingDay = GetUsallyOpeningHour(openingHours, DayOfWeek.Thursday, x => x.From);
//or:
var openingDay = GetUsallyOpeningHour(openingHours, DayOfWeek.Thursday, x => x.To);


Set Label content and make it disappear after 5 seconds Let's say I have a label and a button. If I click the button, the label is set to some text, say "hello, world!", and after 5 seconds, it should disappear. Easy enough, right? I have the following implementation:

private async void ButtonBase_OnClick(object sender, RoutedEventArgs e)
{
  Label = "hello, world!";
  await Task.Delay(5_000);
  Label = string.Empty;
}

Now let's say I have the additional requirement that the button should remain responsive on each subsequent button click, and now the label text should remain hello, world! for the next 5 seconds i.e. the label text should remain for 5 seconds since the last time the button was clicked. The problem with the previous code is that if I had clicked the button 2.5 seconds ago and click it again now, the label text will disappear after 2.5 seconds. My idea to solve this was to maintain a list of tasks and add the delay task and await the task list before clearing the label. This is the implementation:

private static readonly List<Task> TaskList = new List<Task>();
private async void ButtonBase_OnClick(object sender, RoutedEventArgs e)
{
  Label = "hello, world!";
  TaskList.Add(Task.Delay(5_000));
  await Task.WhenAll(TaskList);
  if (TaskList.TrueForAll(t => t.IsCompleted))
  {
    TaskList.RemoveAll(t => t.IsCompleted);
    Label = string.Empty;
  }
}

Implementation with a cancellation token:

private static CancellationTokenSource _cancellationTokenSource = new CancellationTokenSource();
private async void ButtonBase_OnClick(object sender, RoutedEventArgs e)
{
  Label = "hello, world!";
  _cancellationTokenSource.Cancel();
  _cancellationTokenSource = new CancellationTokenSource();
  try
  {
    await Task.Delay(5_000, _cancellationTokenSource.Token);
    Label = string.Empty;
  } catch (TaskCanceledException) { }
}

As far as I know, these implementation works, but I don't know much about asynchronous programming so I don't know how to be absolutely sure it works.

Questions:
How would you implement this? Is there a better way to implement this? I originally figured that Task.WhenAll() would take care of waiting for all tasks to complete, but this does not seem to be the case. What am I getting wrong?

I would use a timer.

On callback, you set the content of your label to empty. You just have to restart the timer on every button click.

I think it simplifies a lot the code.


Form Validation Evaluation I've been trying several styles of validation code for a form.  I was wondering if this is good or needs some improvement or just trash it and use jQuery validator code?

$("#registration-form").on("submit", function(e) {

    e.preventDefault();
    var error_field = "";

    $(this).find(".input-field").each(function() {

        var field_id = $(this).attr("id");
        var value = $.trim($(this).val());
        var length = value.length;
        var error_value = "";

        // Do field checks
        if (length == 0) {
            error_value = "You missed this field. D:";
        } else if (length > 0 && length < 7) {
            error_value = capitalize(field_id) + " too short";
        } else if (length > 30 && field_id == "username") {
            error_value = capitalize(field_id) + " too long";
        } else if (length > 50 && field_id == "password") {
            error_value = capitalize(field_id) + " too long";
        } else if (passwordCombination(value) === false && field_id == "password") {
            error_value = capitalize(field_id) + " must contain at least one string, number, and symbol.";
        } else if (emailValidate(value) === false && field_id == "email") {
            error_value = "Invalid email! :(";
        }

        // Do show error labels
        if (error_value != null) {
            $("#registration-form .content-row."+ field_id + " .box-error").text(error_value);
            error_field += error_value;
        }
    });

    if (error_field == "") {

        var username = $.trim($("#username").val());
        var email = $.trim($("#email").val());
        var password = $.trim($("#password").val());

        $.ajax({ type: "POST", url: "process/app_usage.php",
            data: { username: username, email: email, process: "validate_user_email" },
            success: function(data) {

                // Do stuff
            }
        });
    }
});

I was wondering if this is good

Well, are you just looking to prevent the normal user from messing up during registration? I think it would be fine for that.

I'll see if I can point out flaws in your validation...

First, you check to see if length is 0. If it is, you warn them. Cool. But right after, you check to see if it's greater than 0. This is redundant? It would have to be longer than 0 characters to pass the first conditional!
Now you check length < 7. But what if my email is a@a.ca? Eeek! But I was lucky and set up my domain www.a.ca with email support and registered with my first initial!
Why can't the username be longer than 30 characters? Is your database not capable of such long strings? If it truly just cannot handle those awfully long names, fine. If it can, I'd suggest you support long names! Besides, I'd hate to find a new username if I've always used my most favorite 32 characters username!
Why can't my password be longer than 50 characters either? I might be one of those users who has funky passwords that are always has 9 words in their passwords.
What exactly is passwordCombination()? If I'm one of those funky users I mentioned in point 4, I'd be upset with this rule.
Use <input type="email" /> to validate emails on the client-side, not emailValidate(). Then check it again on the server with something such as PHP's filter_var().
":(" should really be "):" to be consistant with eye placement in "D:".

There's some critique on your validation. So maybe it would be best to find an established validation library and make slight alterations to suit you! Google has some validation standards, and W3C also has some tips and tricks up their sleeve!

But always remember:

  Servers should not rely on client-side validation. Client-side validation can be intentionally bypassed by hostile users, and unintentionally bypassed by users of older user agents

Now I'm not JavaScript maniac, but I might be able to help a little!

I believe you could generalize $(this).find(".input-field") into something such as $(this).find("input[type=text], input[type=password]"). This way you don't rely on having that class.
You just check if error_field is empty, so why not make it simpler and turn it into a counter.  Each error increments it. In the end, if it's value is equal to 0, it's all good!


Constructing a tibble from another tibble (without for loops?) tibbles documentation
I've got this doing what I want, but it's using a for loop and I've read many times to avoid for loops in R for efficiency and style. Well, it's not a computationally demanding task and I think it might be as clear as it could be in a for loop. In any case, it seems a bit tricky to process this functionally rather than iteratively.
I'm starting with one tibble: source_tbl.
I want to make another tibble: constructed_tbl.
Every column in constructed_tbl is a sum of a subset of columns in source_tbl.
The column mapping is stored in col_conversion_tbl.
>print(col_conversion_tbl)

# A tibble: 6 x 2
source_col       constructed_col
<chr>            <chr>
1 col1             A
2 col2             A
3 col3             A
4 col4             A
5 col5             B
6 col6             B

So,
constructed_tbl[,'A']
should be equal to ...
rowSums(source_tbl[,c('col1','col2','col3','col4')])
The best way I've come up with
The best way I've come up with is to

convert col_conversion_tbl to a list, col_conversion_lst, where each item is an array of source_cols and is named after a constructed_col.
initialize an empty constructed_tbl tibble w/ appropriate nrows, ncols, and column names
Loop through names(col_conversion_lst) replacing the empty column from constructed_tbl with the sum of the appropriate columns from source_tbl.

Here's my code
library(tibble)
library(magrittr)
library(purrr)

source_tbl <- tibble(col1=c(1,1,1),col2=c(2,2,2),col3=c(3,3,3),col4=c(4,4,4),col5=c(5,5,5),col6=c(6,6,6))
col_conversion_tbl <- tibble(source_col = c('col1','col2','col3','col4','col5','col6'), constructed_col = c('A','A','A','A','B','B'))

col_conversion_lst <- col_conversion_tbl %>%
    split(.$constructed_col) %>%
    map(~.$source_col)

constructed_tbl <- as_tibble(matrix(nrow=nrow(source_tbl),ncol= length(col_conversion_lst) ))
colnames(constructed_tbl) <- names(col_conversion_lst)

for (n in names(col_conversion_lst)){
    constructed_tbl[,n] <- rowSums(source_tbl[ ,col_conversion_lst[[n]]])
}

I would use a matrix multiplication. It is both efficient and leads to much shorter code, as you can see:

constructed_tbl <- as.tibble(as.matrix(source_tbl) %*% table(col_conversion_tbl))


Concurrent queue with pthread implementation I'm writing a multi-threaded queue implemented with pthreads. Since I came up with the code based on several tutorials from Internet, I would like to make sure there are no logic errors in my code:

template <typename T>
class ThreadQueue {
public:
    ThreadQueue() {
        pthread_mutex_init(&m_qmtx, NULL);
        pthread_cond_init(&m_condv, NULL);
    }

    ~ThreadQueue() {
        pthread_mutex_lock(&m_qmtx);
        m_queue.clear();
        pthread_mutex_unlock(&m_qmtx);
    }

    void push(T t_data) {
        pthread_mutex_lock(&m_qmtx);
        m_queue.push_back(t_data);
        pthread_mutex_unlock(&m_qmtx);

        pthread_cond_signal(&m_condv);
    }

    T front() {
        T ret;
        pthread_mutex_lock(&m_qmtx);
        while (m_queue.empty()) {
            pthread_cond_wait(&m_condv, &m_qmtx);
        }
        ret = m_queue.front();
        pthread_mutex_unlock(&m_qmtx);
        return ret;
    }

    void pop() {
        pthread_mutex_lock(&m_qmtx);
        if (!m_queue.empty())
            m_queue.pop_front();
        pthread_mutex_unlock(&m_qmtx);
    }

private:
    std::deque<T> m_queue;
    pthread_mutex_t m_qmtx;
    pthread_cond_t m_condv;
};

Correctly initialized members.

ThreadQueue() {
    pthread_mutex_init(&m_qmtx, NULL);
    pthread_cond_init(&m_condv, NULL);
}

But you don't check the error codes.
The object is potentially created with invalid members. Use exceptions to gurantee the state of your object.

Destructor does not do anything useful.

~ThreadQueue() {
    pthread_mutex_lock(&m_qmtx);
    m_queue.clear();
    pthread_mutex_unlock(&m_qmtx);
}

The destructr is supposed to clean resources.
It does not. You should destroy the mutex and condition variable in the destructor (reverse order of creation to be consistent).

Also locking before clearing the queue does not buy you anything. If at the point the destructor is called the object is still visible to other threads then it is going to break anyway. The queue is going to call clear in its own destructor so it is pointless doing it manually.

The call to signal should be inside the lock.

void push(T t_data) {
    pthread_mutex_lock(&m_qmtx);
    m_queue.push_back(t_data);
    pthread_mutex_unlock(&m_qmtx);

    pthread_cond_signal(&m_condv);
}

Since you should be using RAII for you locking anyway (See below) the signal is going to fall inside the lock stratergy unless you do something complex.

Also you should pass the parameter by const reference rather than value.

Normally front() is going to give you a reference to the front object.

T front() {
    T ret;
    pthread_mutex_lock(&m_qmtx);
    while (m_queue.empty()) {
        pthread_cond_wait(&m_condv, &m_qmtx);
    }
    ret = m_queue.front();
    pthread_mutex_unlock(&m_qmtx);
    return ret;
}

Returning a copy is probably not what you want (especially for anything interesting). But returning a reference is not really an option either as that opens you to situations where you have race conditions (you have a reference to the top object and another thread pops it (destroying it) just before you call a method).

So you need to provide a method for accessing the object while maintaining the lock (this will probably require the return of a wrapper object that maintains a lock on the queue) or alternatively removing this function.

Other non C++ things that should be addressed:

The mutex should be locked/unlocked using RAII. You are leaving your code open to exception handling problems. If an exception is generated by T which you have no control over then you could leave your queue in an unusable state with the mutex locked and no way to unlock it (because the stack of the locker has been unwound past the unlock because of an exception).

Yes this is correct:

   if (!m_queue.empty())
        m_queue.pop_front();

But don't get into bad habbits. No using the braces can lead to maintenance problems on C/C++ code because of macros. Best practice dictates that you always use {} for sub statements of if/while/for.

To make the code easy to read when including the code in the interface declaration put the variables at the top. Thus you can read the code in with the context of the variables and knowing their type. If you interface declaration is just an interface (and the code is put in the source file rather than the header) then it is fine to put the private variables at the bottom.


Better code for converting a char to its UTF-8 percent encoding representation? This is working code for a URI template (RFC 6570) implementation; when the character to render is not within a specific character set, it is needed to grab the UTF-8 representation of that character and encode each byte as a percent-encoded sequence.

The question is whether something more efficient than this exists?

private static String encodeChar(final char c)
{
    final String tmp = new String(new char[] { c });
    final byte[] bytes = tmp.getBytes(Charset.forName("UTF-8"));
    final StringBuilder sb = new StringBuilder();
    for (final byte b: bytes)
        sb.append('%').append(UnsignedBytes.toString(b, 16));
    return sb.toString();
}

Note: UnsignedBytes.toString() is from Guava.

Documentation here

public static void main(final String[] args) {
        try {
            System.out.println(URLEncoder.encode("a?>éàùA-Z",
                    System.getProperty("sun.jnu.encoding")));
        } catch (final UnsupportedEncodingException e) {
            System.out.println("BIG PB");
            return;
        }
    }

Output :

  a%3F%3E%E9%E0%F9A-Z

You have the same for URLEDecoder

EDIT

For URI
Found in src  of iMeMex Dataspace ManagementSystem

public static URI encode(String uriStr) throws URISyntaxException {
    if (uriStr != null) {
        // poor man's encoding
        uriStr = uriStr.replaceAll(" ", "%20");
    }
    return new URI(uriStr);
}

JavaDoc here

But URI from Java does not change other %VALUES.


Producing the intersection of several sequences Based on this SO answer I have created a method that produce the set intersection of several sequences:

private static IEnumerable<TSource> Intersect<TSource>(this IEnumerable<IEnumerable<TSource>> source)
{
    using (IEnumerator<IEnumerable<TSource>> sourceIterator = source.GetEnumerator())
    {
        //if there is no first element return an empty list
        if (!sourceIterator.MoveNext())
        {
            yield break;
        }

        IEnumerable<TSource> firstSequence = sourceIterator.Current;
        if (sourceIterator.MoveNext())
        {
            //create a hashset of the first sequence
            HashSet<TSource> hashSet = new HashSet<TSource>(firstSequence);

            //intersect the other sequences
            do
            {
                hashSet.IntersectWith(sourceIterator.Current);
            }
            while (sourceIterator.MoveNext());
            foreach (TSource element in hashSet)
            {
                yield return element;
            }
        }
        else
        {
            //if there is only one sequence, return it without any intersection
            foreach (TSource element in firstSequence)
            {
                yield return element;
            }
        }
    }
}

I'm particularly annoyed that the method is lot of times slower than the following implementation, when there is only one sequence:

public static IEnumerable<TSource>
          Intersect<TSource>(this IEnumerable<IEnumerable<TSource>> source)
{
    return source.Aggregate((x, y) => x.Intersect(y));
}

I already put a special test and code path when there is only one element, but it does not seems to be enough. Beside this problem, is there any obvious way to optimize the method?

First let me point out that you shouldn't name this extension method as you have named: Intersect(). The name Intersect() already exists and will return the intersection of two collections.  Your method is intersecting a collection of collections with each other.  It should must be named differently.  I would suggest IntersectAll() or possibly IntersectMany().

I feel your implementation for the case of a single collection is wrong.  You are performing a set operation here.  You need to ensure that for all cases, you maintain set semantics.  You assume that for a single collection that you may return that collection.  But what if that collection contains duplicates?  You would need to filter out those duplicates.  The aggregated version should as well.

With that in mind, I would change your implementation like so:

public static IEnumerable<TSource> IntersectAll<TSource>(
        this IEnumerable<IEnumerable<TSource>> source)
{
    using (var enumerator = source.GetEnumerator())
    {
        if (!enumerator.MoveNext())
            yield break;

        var set = new HashSet<TSource>(enumerator.Current);
        while (enumerator.MoveNext())
            set.IntersectWith(enumerator.Current);
        foreach (var item in set)
            yield return item;
    }
}

I'm not sure how you did your testing but even with your implementation, I still get much faster results, even in the single collection case.  Tested in LINQPad with optimizations enabled:

Your implementation:

public static IEnumerable<TSource> IntersectAll1<TSource>(this IEnumerable<IEnumerable<TSource>> source)
{
    using (IEnumerator<IEnumerable<TSource>> sourceIterator = source.GetEnumerator())
    {
        //if there is no first element return an empty list
        if (!sourceIterator.MoveNext())
        {
            yield break;
        }

        IEnumerable<TSource> firstSequence = sourceIterator.Current;
        if (sourceIterator.MoveNext())
        {
            //create a hashset of the first sequence
            HashSet<TSource> hashSet = new HashSet<TSource>(firstSequence);

            //intersect the other sequences
            do
            {
                hashSet.IntersectWith(sourceIterator.Current);
            }
            while (sourceIterator.MoveNext());
            foreach (TSource element in hashSet)
            {
                yield return element;
            }
        }
        else
        {
            //if there is only one sequence, return it without any intersection
            foreach (TSource element in firstSequence)
            {
                yield return element;
            }
        }
    }
}

public static IEnumerable<TSource> IntersectAll2<TSource>(
        this IEnumerable<IEnumerable<TSource>> source)
{
    using (var enumerator = source.GetEnumerator())
    {
        if (!enumerator.MoveNext())
            yield break;

        var set = new HashSet<TSource>(enumerator.Current);
        while (enumerator.MoveNext())
            set.IntersectWith(enumerator.Current);
        foreach (var item in set)
            yield return item;
    }
}

public static IEnumerable<TSource> IntersectAllControl<TSource>(
        this IEnumerable<IEnumerable<TSource>> source)
{
    return source.Aggregate((x, y) => x.Intersect(y));
}

void Main()
{
    var rng = new Random();
    var collections = Enumerable.Range(0, 500)
        .Select(i => Enumerable.Range(rng.Next(0, 50), 100).ToArray())
        .Cast<IEnumerable<int>>()
        //.Take(1)
        .ToArray();

    const int Iterations = 1000; // 1000000 for single collection

    object result1 = null;
    var timer1 = Stopwatch.StartNew();
    for (var i = 0; i < Iterations; i++)
        result1 = collections.IntersectAll1().ToList();
    timer1.Stop();
    result1.Dump(String.Format("result1: {0}", timer1.Elapsed));

    object result2 = null;
    var timer2 = Stopwatch.StartNew();
    for (var i = 0; i < Iterations; i++)
        result2 = collections.IntersectAll2().ToList();
    timer2.Stop();
    result2.Dump(String.Format("result2: {0}", timer2.Elapsed));

    object controlResult = null;
    var controlTimer = Stopwatch.StartNew();
    for (var i = 0; i < Iterations; i++)
        controlResult = collections.IntersectAllControl().ToList();
    controlTimer.Stop();
    controlResult.Dump(String.Format("controlResult: {0}", controlTimer.Elapsed));
}

Note: The ToList() calls were needed on all invocations to ensure that the intersections are actually generated.

Run on my machine with multiple collections:

result1:       00:00:01.0210291
result2:       00:00:01.0285069
controlResult: 00:00:03.1512838

And with a single collection:

result1:       00:00:02.9254441
result2:       00:00:04.8505489
controlResult: 00:00:00.2102433

I have to apologize, I have changed my review as I wrote this due to some details I overlooked as I was writing. Thank you mjolka for pointing out my mistake.

For multiple collections, the new implementations will always be a win over the aggregation.  In the aggregated approach, each sub-instersection needs to be determined for every collection.  In our new implementations, only a single set intersection needs to be maintained.

The new implementations will not beat out the aggregate method in the single collection case however, because the aggregate will simply pass the collection through as the result untouched.  The new implementations will always do some bit of processing.  However again, I feel that if set semantics were preserved, this difference will probably be cancelled out anyway.


Create albums out of songs I have made a method which returns a list of all songs on my device. Now I want to get a list of albums out of these songs.

What I basically do is go through every song I have in my list, check if its album is already in the list and if not I create a new album.

This method will take some time since there are 1400 songs on my device so I wondered how I can make this method any faster? Also, any other improvements?

public static ArrayList<Album> loadAlbums( Context c ){
    ArrayList<Song> tmpList = getAllSongs( c);
    ArrayList<Album> albumList = new ArrayList<Album>();

    for( Song s : tmpList ){

        boolean added = false;

        for( Album a : albumList ){

            if( a.getAlbumName().contentEquals( s.getAlbum() ) && a.getArtistName().contentEquals( s.getArtist() ) ){
                a.addSong( s.getPath() );
                added = true;
            }
        }

        if( !added ){
            Album a = new Album( s.getAlbum() );
            a.setAlbum_id( s.getAlbumId() );
            a.setArtistName( s.getArtist() );
            a.addSong( s.getPath() );

            albumList.add(a);
        }

    }

    tmpList = null;
    return albumList;
}

What you need is a Map. A map is a collection of key --> value combinations. There can not be two equal keys pointing to different values.

There are a few ways to organize this map. The simplest is to make Map<String, Album> albumNames;, but that would make it possible for collisions if two albums had the same album name.

Another alternative is Map<String, List<Album>> for fixing the problem with albums having the same name.

There are more alternatives but they can get quite complex so I think this is enough for now.

Then when you loop through your songs, for each song:

List<Album> possibleAlbums = albumNames.get(song.getAlbum()).

If that returns null, then add a new list and add an album to it.
otherwise loop through the list to find the album you are looking for (that matches the artist). Again, if none is found create a new one.

Once you have found or created a matching album, you can add the song to it.

Other comments:

Declare by interface and not implementation. Make the method return List<Album>, also use for example List<Song> tmpList = getAllSongs(c);
Use longer and better names. s --> song, a --> album, c --> context, tmpList --> songs.
tmpList = null; in the bottom of the method will have no effect whatsoever. You can safely remove that line.


Removing duplicate file references from history I have some code that searches for duplicate files and then removes those files from a csv. To make the set of files I am searching over much smaller, I first group the files by size; if a file has a size that it is unique, it is safe to assume that it is unique and does not need to be included in the set of files I am checking for duplicates. Additionally, I must only consider files that are listed in the csv. Occasionally the csv has filenames of files that are not in the directory, which leaves me with several choices.

I can initially check my list of files for those which do not exist and remove them. This adds a loop over all the files and seems unnecessary/inefficient; it is very rare that a file does not exist.
Use the following code with exception handling when the file is not found and delete file from my list at this point. However this introduces a side effect to the grouping function. Clearly this function does more than its name suggests.

Code:

def group_by_size(self, files_to_group):
    grouped_by_size = dict()
    for fileid in files_to_group:
        full_path = os.path.join(self.folder, fileid)
        try:
            size = os.path.getsize(full_path)
        except WindowsError:
            print WindowsError
            print "The file wasn't found. Deleting row from PremsPhotos."
            del self.files[fileid]
            del self.spreadsheet_reader.main_row_dict[fileid]
        if grouped_by_size.get(size):
            grouped_by_size[size].append(fileid)
        else:
            grouped_by_size[size]=[]
            grouped_by_size[size].append(fileid)
    return grouped_by_size

Alternatively, eliminate duplicates beforehand:

    def remove_nonexistant_photos(self):
        for photo in self.files:
            os.path.exists(os.path.join(self.folder, photo))

The context of this code is in a script that will likely be run a lot, but will not likely be modified once it works. Looping over all the files in the directory takes about half a second when there are 15k files in the directory. With both of these in mind, I suppose the decision is not of huge importance, but I have been doing a lot of reading about best practices, and want to improve my coding intuition/style as I go forward.

The group_by_size function has multiple responsibilities:

Group files by size
Verify file
Modify attributes of the class (self.files, self.spreadsheet_reader.main_row_dict)

It would be better to reorganize your code so that each function has a single responsibility.

There is some code duplication here that could be improved:

if grouped_by_size.get(size):
    grouped_by_size[size].append(fileid)
else:
    grouped_by_size[size]=[]
    grouped_by_size[size].append(fileid)

Like this:

files = grouped_by_size.get(size)
if files is None:
    files = []
    grouped_by_size[size] = files
files.append(fileid)


Python implementation of Netcat, new design I've made a simple netcat in Python and would appreciate any advice/opinions of how I've implemented it.

test.py:

#!/usr/bin/env python
# -*- coding: utf-8 -*-

import sys

import netcat

def main(*args):
    netcat.connect('localhost', 4444)
    netcat.listen('', 4444)

if __name__ == '__main__':
    sys.exit(main(*sys.argv))

netcat.py:

# -*- coding: utf-8 -*-

import sys
import select
import socket
from contextlib import closing

from servers import Server
from cat import cat_line

def netcat(sock, infile=sys.stdin, outfile=sys.stdout):
    with closing(sock) as s:
        sf = s.makefile()
        while True:
            r, w, e = select.select([s, infile], [], [])
            if s in r:
                if not cat_line(sf, outfile):
                    break
            if infile in r:
                if not cat_line(infile, sf):
                    break

def listen(lhost, lport, *args, **kwargs):
    with Server(lhost, lport) as server:
        client = server.get_client()
        netcat(client, *args, **kwargs)

def connect(rhost, rport, *args, **kwargs):
    server = socket.create_connection((rhost, rport))
    with closing(server) as s:
        netcat(s, *args, **kwargs)

servers.py:

# -*- coding: utf-8 -*-

import socket

class Server(object):

    def __init__(self, lhost, lport):
        self._sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        self._sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
        self._sock.bind((lhost, lport))
        self._sock.listen(1)

    def __iter__(self):
        return self

    def next(self):
        return self.get_client()

    def __enter__(self):
        return self

    def __exit__(self, *exc):
        self.close()

    def get_client(self):
        return self._sock.accept()[0]

    def close(self):
        self._sock.close()

cat.py:

# -*- coding: utf-8 -*-

import sys

def cat_line(infile=sys.stdin, outfile=sys.stdout):
    line = infile.readline()
    if not line:
        return False
    outfile.write(line)
    outfile.flush()
    return True

def cat(infile=sys.stdin, outfile=sys.stdout):
    while True:
        if not cat_line(infile, outfile):
            break

Edit: Maybe I should take the responsibility of closing the socket from the netcat function...

def netcat(sock, infile=sys.stdin, outfile=sys.stdout):
    sf = sock.makefile()
    while True:
        r, w, e = select.select([sock, infile], [], [])
        if sock in r and not cat_line(sf, outfile):
                break
        if infile in r and not cat_line(infile, sf):
                break

and instead, put that functionality into the listen function?

def listen(lhost, lport, *args, **kwargs):
    with Server(lhost, lport) as server:
        client = server.get_client()
    with closing(client) as c:
        netcat(c, *args, **kwargs)

Also...

def cat(infile=sys.stdin, outfile=sys.stdout):
    while True:
        if not cat_line(infile, outfile):
            break

could be

def cat(infile=sys.stdin, outfile=sys.stdout):
    while cat_line(infile, outfile):
        pass

github

test.py

If you look at the definition of main(), you will see that it doesn't explicitly return anything.  By default, functions return None, so you will always be exiting with None.  Since it is always the same, it is useless even to use sys.exit().  The program will exit once the file has been read, so you don't need to tell Python to do that.  If you want to use exit codes, you'll need main() to return them.

main() doesn't use its arguments, so why give it any?  If sys.argv is ignored, take it out and remove the useless import of sys.

netcat.py

if s in r:
    if not cat_line(sf, outfile):
        break

The only time your program does something in the case of s in r is when not cat_line(sf, outfile).  Since you want to handle only the cases where both are true, use and:

if s in r and not cat_line(sf, outfile):
    break

Same with the next if block.  I wouldn't recommend it because it makes your lines too long, but you could put both if blocks in one:

if ((s in r) and not cat_line(sf, outfile)) or ((infile in r) and not cat_line(infile, sf):
    break

servers.py and cat.py look okay to me, but my opinion is that you don't need multiple files.  You don't have a bunch of lines that need to be split up, so you might as well put all of it in one file (not including test.py).


Java probability if 2 integers are coprime Hi I am trying to write a programme that outputs the probability that the two randomly selected numbers are coprime

Two integers are coprime if the only positive integer that divides into both of them
is 1. In other words, the greatest common divisor of two coprime numbers is 1.
If two numbers x and y are selected randomly, the odds that they will be coprime
with each other is 61% (6 divided by PI^2). But what if we put some restrictions on
the two numbers? For example, if x is a randomly selected number that is divisible
by 7 and y is a randomly selected number that is divisible by 13? In this case, the
odds that they are coprime is 49%.
I am given a divisor of the first number and a divisor of the
second number. I must then output an integer from 0 to 100, which is the
percentage probability that the two randomly selected numbers are coprime.

here is what I have so far
import java.util.Scanner;
public class Main {
    public static int myTestMethod(int a, int b)
    //method taking two numbers a and b
    {
        //Calculating gcd because if gcd is 1 then a and b are //coprimes.
        if (b == 0)
            return a;
        else
            return myTestMethod(b, a % b);
    }
    public static void main(String args[]) {
        Scanner sc = new Scanner(System.in);
        System.out.println("Enter a and b");
        int y;
        float p = 0;
        //taking inputs a and b two numbers.
        int a = sc.nextInt();
        int b = sc.nextInt();
        //Calculating gcd by calling method
        y = (myTestMethod(a, b));
        System.out.print("gcd is:");
        System.out.println(y);
        p = (myTestMethod(a, b));
        //if gcd is 1 the probability should be 1
        if (y == 1) {
            System.out.println("probability is:");
            System.out.println(p);
        }
        //else print probability as p/100.
        else {
            System.out.println("The probability is :");
            System.out.println(p / 100);
        }
    }
}

is this the correct way to go about it?

This review is for the code only, not the optimization / correctness; I assume the code works as IS.
Avoid using C-style array declaration
In the main method, you declared a C-style array declaration with the args variable.
before
String args[]

after
String[] args

In my opinion, this style is less used and can cause confusion.
Always add curly braces to loop & if
In my opinion, it's a bad practice to have a block of code not surrounded by curly braces; I saw so many bugs in my career related to that, if you forget to add the braces when adding code, you break the logic / semantic of the code.
Before
if (b == 0)
   return a;
else
   return myTestMethod(b, a % b);

After
if (b == 0) {
   return a;
} else {
   return myTestMethod(b, a % b);
}

Other things
To save space in the main method, you can inline the variable y and p
//[...]
int y = (myTestMethod(a, b));
//[...]
float p = (myTestMethod(a, b));
//[...]

You can take less lines for the printing in the main method
You can make a printing method
public static void main(String[] args) {
   if (y == 1) {  //if gcd is 1 the probability should be 1
      printMultiLine("probability is:", p);
   } else { //else print probability as p/100.
      printMultiLine("The probability is :", p / 100);
   }
}

private static void printMultiLine(String message, float value) {
   System.out.println(message);
   System.out.println(value);
}

or
You can uses java.io.PrintStream#printf
java.io.PrintStream#printf offer you to use patterns to build the string without concatenating it manually. The only downside is you will be forced to add the break line character yourself; in java you can use the %n to break the line (portable between various platforms) or uses the traditional \n / \r\n.
public static void main(String[] args) {
   //[...]
   if (y == 1) {  //if gcd is 1 the probability should be 1
      System.out.printf("probability is:%n%f", p);
   } else { //else print probability as p/100.
      System.out.printf("The probability is:%n%f", p / 100);
   }
}


PHP Config file loader class I know this is a pretty simple class for loading a config file into an object, and then accessing it's properties.  I think config options should be lightweight and a class like this seems to adds a lot of overhead IMO but I really like being able to access the config properties with $config->propertyName.  So do you see any room for improvement?  Mainly performance improvements?

<?php
// config.class.php
/*
example usage
$config = Config::getInstance(PATH TO FILE, FILE TYPE);
echo $config->ip;
echo $config->db['host'];

example array file
<?php
return array(
    'db' => array(
        'host' => 'localhost',
        'user' => 'user1',
        'pass' => 'mypassword'),

    'ip' => '123456',

)

*/

class Config
{
    private static $_instance = null;
    public $options = array();

    /**
     * Retrieves php array file, json file, or ini file and builds array
     * @param $filepath Full path to where the file is located
     * @param $type is the type of file.  can be "ARRAY" "JSON" "INI"
     */
    private function __construct($filepath, $type = 'ARRAY')
    {
        switch($type) {
            case 'ARRAY':
                $this->options = include $filepath;
                break;

            case 'INI';
                $this->options = parse_ini_file($filepath, true);
                break;

            case 'JSON':
                $this->options = json_decode(file_get_contents($filepath), true);
                break;

            //TO-DO add Database option for settings. Table = id, property, value
            case 'DATABASE':
                $this->options = json_decode(file_get_contents($filepath), true);
                break;
        }
    }

    private function __clone(){}

    public static function getInstance($filepath, $type = 'ARRAY')
    {
        if (!isset(self::$_instance)) {
            self::$_instance = new self($filepath, $type);
        }
        return self::$_instance;
    }

    /**
     * Retrieve value with constants being a higher priority
     * @param $key Array Key to get
     */
    public function __get($key)
    {
        if (isset($this->options[$key])) {
            return $this->options[$key];
        }else{
            trigger_error("Key $val does not exist", E_USER_NOTICE);
        }
    }

    /**
     * Set a new or update a key / value pair
     * @param $key Key to set
     * @param $value Value to set
     */
    public function __set($key, $value)
    {
        $this->options[$key] = $value;
    }

}
?>

Mainly performance improvements?

There doesn't seem to be much oppurtunity for performance improvements. This class basically consists of (1) loading and parsing the configuration and (2) accessing data. For loading, parse_ini_file and json_decode are builtin methods so unless you write your own parser from scratch, you won't make this any faster. And accessing data is basically a single condition checking and an array lookup, not much to improve there either.

However, here is a different kind of suggestion. You probably want to make sure that $filepath in the constructor is restricted to specific directories, for example your web root or better, a subfolder of that. That also means checking for attempts to escape from the restricted directory, e.g. $filepath does not have components like ../. I can easily imagine an attack where a malicious user gets to place a custom php script in /tmp and then (if you have some other vulnerability in your other code) manages to call new Config('/tmp/attack.php'). Then you have an arbitrary maclicous script executing with the priviliges of your website. Ouch.


Sum pairs of numbers in array This code takes a list of ints and sums their pairs, storing the summed pairs in an array sumPairs, then returning that array. If the supplied array toSum has an odd number of elements, it returns the sumPairs with the last element of the toSum as the last element of sumPairs. Any suggestions for improving my code, or accomplishing this task in a better way, would be most appreciated!

public static int[] collapse(int[] toSum) {
    int[] sumPairs = new int[toSum.length / 2 + toSum.length % 2];
    for (int i = 0, j = 0; i < toSum.length - 1; i+=2, j++) {
        sumPairs[j] = toSum[i] + toSum[i + 1];
    }
    if (toSum.length % 2 == 1 && sumPairs[sumPairs.length - 1] == 0) {
        sumPairs[sumPairs.length - 1] = toSum[toSum.length - 1];
    }
    return sumPairs;
}

Looks good! :)

The only thing I can spot is that the second condition for your if statement is unnecessary, as the first condition is all that is required. Therefore:

if (toSum.length % 2 == 1) {
    sumPairs[sumPairs.length - 1] = toSum[toSum.length - 1];
}


Changing php.ini with code in a LEMP stack on Debian systems This is how I establish a Debian LEMP and use code to change php.ini directives, as part of php-fpm.

Please review the way I change php.ini. I believe my approach here is bad because I edit a conf file directly instead overriding somewhere.

apt-get upgrade nginx mysql-server php-fpm php-mysql php-mbstring php-mcrypt -y
sed -i "s/post_max_size = .M/post_max_size = 200M/ ; s/upload_max_filesize = .M/upload_max_filesize = 200M/" /etc/php/*/fpm/php.ini
sed -i "s/;cgi.fix_pathinfo=1/cgi.fix_pathinfo=0/" /etc/php/*/fpm/php.ini

By doing ad hoc editing like that, you're making certain assumptions about what the php.ini configuration currently looks like.  For example, you have assumed that the size limits end with M, and that those lines are not commented out.  You have also assumed that the cgi.fix_pathinfo line is commented out.

There are two better ways that I prefer.

The simple solution is to put all of the local overrides together in one file.  From the PHP manual:

  Within each directory, PHP will scan all files ending in .ini in alphabetical order.

So, instead of editing, simply create a file with a name like /etc/php/7.0/fpm/zz_overrides.ini containing four lines:

[PHP]
post_max_size = 200M
upload_max_filesize = 200M
cgi.fix_pathinfo = 0

This has the benefit that you leave all of the distro-provided configuration files in their pristine state, and won't need to merge your changes when upgrading.  The disadvantage is that you will need to keep in mind where to look for the effective settings — but that's not hard if you make it a habit.
Alternatively, use augtool, which is a configuration file editor, as in this example.


PHP test isset and instantiate in one shot. Possible in PHP? How to improve this kind of redundancy?

$nid = isset( $_GET['nid'] ) ? $_GET['nid'] : 0;

I want to avoid to repeat $_GET['nid']

Example of useful use case:

$nid = isset( $node->field_mymodule_extended_nid['und'][0]['target_id'] ) ? $node->field_mymodule_extended_nid['und'][0]['target_id'] : 0;

By reading the manual:

  Since PHP 5.3, it is possible to leave out the middle part of the ternary operator. Expression expr1 ?: expr3 returns expr1 if expr1 evaluates to TRUE, and expr3 otherwise.

So in short:

$nid = $_GET['nid'] ?: 0;

Would work, but PHP will trigger a notice if $_GET['nid'] is not set. The logic || operator won't help, either. The closest you can get is:

(($nid = $_GET['nid']) || ($nid = o));

But that's not exactly shorter, nor does it solve the Notice issue.
The "best" as in shortest, moste un-maintainable and horrible looking code I can think of is this:

$nid = 0;//default
foreach($_GET as $name => $val)
{
    $$name = $val;
}
var_dump($nid);

This turns all keys that were present in the $_GET array into var names. If you simply declare the vars you're after (names must be identical to keys, though) and assign them a default value, this loop takes care of the rest.
To map certain keys to another variable name, you could use another array:

$nid = 0;
$map = array('new_id' => 'nid');
foreach($_GET as $k => $v)
{
    if (isset($map[$k]))
    {
        $k = $map[$k];
    }
    $$k = $v;
}

But honestly, maintaining code like this is going to be a nightmare. Though request objects add a lot of overhead for a task as simple as this, their get methods make it all worth while:

public function get($name, $default = null)
{
    if (!isset($_GET[$name]))
    {
        return $default;
    }
    return $_GET[$name];
}

This is easy to read/understand and easy to use:

$nid = $getInstance->get('nid', 0);

Assignes the GET param, or 0 to $nid... which makes your code tidier, easier to maintain and a lot easier to understand. Based on the use-case you posted, I take it you're not using the ternary on just request variables. Well, in that case: use objects with getters/setters that define a default return value (or allow for one to be specified @call-time). If needs must, implement the ArrayAccess or some Traversable interface, so you can use it as an array, too.
Granted, it'll take some time/effort to do this, but once you've done that, any new code you write will be cleaner (no ternaries), debugging will be a doddle and type-hinting will become your best friend. check this answer for some examples and considerations on getters and setters, along with this answer on the importance of pre-declaring properties (and thus implementing getters/setters).

If you're still up for it after that, refer to this answer to see why and how you should avoid object overloading (in PHP, at least) as much as possible.


Assigning array keys based on the values of another array An optimisation question:

Currently I have entities stored in text files in the following format:

Attribute1,Attribute2,Attribute3
Value1-1,Value1-2,Value1-3
Value2-1,Value2-2,Value2-3
Value3-1,Value3-2,Value3-3

And convert it into a 2D array in the following format:

$array = array(array('Attribute1' => 'Value1-1',
                     'Attribute2', => 'Value1-2',
                     'Attribute3', => 'Value1-3'),
               array('Attribute1' => 'Value2-1',
                     'Attribute2', => 'Value2-2',
                     'Attribute3', => 'Value2-3'),
               array('Attribute1' => 'Value3-1',
                     'Attribute2', => 'Value3-2',
                     'Attribute3', => 'Value3-3'));

Using the following PHP code:

$lines = array();
$dump = normaliseNewLine($dump);
$dump = explode("\n", $dump);
$attributes = explode(',', array_shift($dump));

for($i = 0; $i < count($dump); $i++) {
    $tmp = explode(',', $dump[$i]);
    for ($j = 0; $j < count($tmp); $j++) {
        $lines[$i][$attributes[$j]] = $tmp[$j];
    }
}

I was wondering if there was a more efficient method of completing this task?

PHP provides functions to parse CSV files and strings line-by-line.

$file = ...
$headers = fgetcsv($file);
$lines = array();
while (($line = fgetcsv($file)) !== false) {
    $lines[] = array_combine($headers, $line);
}