Managing "active" and "rest" state for class name-based CSS animations Started working with CSS animations and I'm wondering if there's a more elegant way to manage (with class names) when an animation is on or off. The general pattern I have is like this:

I really dislike the idea of having a css class for base-class-rest that is different than just the base-class. I came up with this solution because I needed a css state that animates from the end of the base-class-active state back to a state of "rest". base-class alone cannot achieve this because there is no animation defined in its css.

tl;dr: I'm toggling a color on and off but I've written too much code for a task this simple. How can I write less code?

Here's a pen: http://codepen.io/anon/pen/GpaJQa

html

css

@keyframes box-active {
    0% { background-color: black; }
    100% { background-color: green; }
}

@keyframes box-rest {
    0% { background-color: green; }
    100% { background-color: currentColor; }
}

.box {
    height: 50px;
    width: 50px;
    background-color: black;
    display: inline-block;
    margin: 5px;
}

.box-active {
    animation: box-active 1s 1 forwards;
}

.box-rest {
    animation: box-rest 1s 1 forwards;
}

js

Array.prototype.slice.call(document.querySelectorAll(".box")).forEach(function(el) {
    el.addEventListener("click", click("box", "box-rest", "box-active"));
});

function click(none, rest, active) {
    return function(e) {
        var classname = e.target.className;
        var arr = classname.split(" ");
        var index = [arr.indexOf(none), arr.indexOf(rest), arr.indexOf(active)];
        //no state
        if (index[1] < 0 && index[2] < 0) {
            arr.push(active);
            return e.target.className = arr.join(" ");
        }
        //at rest
        if (index[2] > -1) {
            arr.splice(index[1], 1);
            arr.push(rest);
        }
        //active
        else {
            arr.splice(index[2], 1);
            arr.push(active);
        }
        e.target.className = arr.join(" ");
    };
}

You can use the toggle method from Element.classList and the animationend event in your example:

<script>
    Array.prototype.slice.call(document.querySelectorAll(".box")).forEach(function(el) {
        el.addEventListener('click', click());
        el.addEventListener('animationend', resetBox());
    });

    function click() {
        return function(e) {
            var target = e.target || e.srcElement;

            if (target.classList.contains('box')) {
                if (target.classList.contains('box-active')) {
                    target.classList.toggle('box-rest');
                } else {
                    target.classList.toggle('box-active');
                }
            }
        };
    }

    function resetBox() {
        return function(e) {
            var target = e.target || e.srcElement;

            if (target.classList.contains('box-rest')) {
                target.className = 'box';
            }
        };
    }
</script>


iOS Core Data Model with Swifty JSON I have been using SwiftyJSON to create my models of Object in Swift.
But recently, I moved to CoreData for saving the objects locally.
I am new to CoreData and want to know the best practices I should follow for creating the model.

Here's what I have created to update and get User's data (after getting the JSON from webservice)

Core Data Model

import Foundation
import CoreData
import SwiftyJSON

@objc(User)
public class User: NSManagedObject {

var authToken : String!

class func addUser(fromJSON json: JSON?, inManagedObjectContext managedObjectContext: NSManagedObjectContext?) -> User? {

    if var json = json { //unwrapping
        let entityDescription = NSEntityDescription.entity(forEntityName: "User", in: managedObjectContext!)
        let newUser = NSManagedObject(entity: entityDescription!, insertInto: managedObjectContext) as! User

        json = json["user"]
        newUser.authToken = json["auth_token"].stringValue
        newUser.email = json["email"].stringValue

        managedObjectContext?.perform({
            _ = try? newUser.managedObjectContext!.save()
        })
        return newUser
    }
    return nil
}

class func getUser(inManagedObjectContext managedObjectContext: NSManagedObjectContext) -> User? {
    // Initialize Fetch Request
    let fetchRequest = NSFetchRequest<NSFetchRequestResult>()

    // Create Entity Description
    let entityDescription = NSEntityDescription.entity(forEntityName: "User", in: managedObjectContext)

    // Configure Fetch Request
    fetchRequest.entity = entityDescription

    if let result = try? managedObjectContext.fetch(fetchRequest) {
        return result.first as? User
    } else {
        return nil
    }
}

class func updateUser(fromJson json : JSON?, inManagedObjectContext managedObjectContext: NSManagedObjectContext) {
    let user = getUser(inManagedObjectContext: managedObjectContext)

    if var json = json {
        json = json["user"]
        user?.name = json["name"].stringValue
        user?.email = json["email"].stringValue
    }

    managedObjectContext.perform({
        _ = try? managedObjectContext.save()
    })
}

}

Here's how I used to create model before I was using Core Data:

Previous Model

import Foundation
import SwiftyJSON

class User : NSObject, NSCoding{

var email : String!
var name : String!

/**
 * Instantiate the instance using the passed json values to set the properties values
 */
init(fromJson json: JSON!){
    if json == nil{
        return
    }
    email = json["email"].stringValue
    name = json["name"].stringValue
}
}

Although the code is working, I am not sure if this is the right way to do it.
How should I actually create the model with CoreData? And how should I call them from ViewControllers? I tried reading tutorials, but they help on getting started but not on how to use them in bigger projects.

Based on the code that you've supplied, here's my take on some small changes in responsibility.

First of all, the NSManagedObject subclass.

You can write this by hand (as I think you have done) but I prefer to have Xcode generate this subclass automatically from the xcdatamodeld file (this option is found in the 'Editor' menu).

Note that in the latest version of Xcode, this file isn't added to your project, but actually added to the 'DerivedData' folder so unless you go looking for it you'll probably never see it.

If you find it and open it up, it will probably look something like this this:

import Foundation
import CoreData

@objc(User)
public class User: NSManagedObject {

    @NSManaged public var name: String?
    @NSManaged public var email: String?
    @NSManaged public var authToken: String?
}

Second, an extension that contains some methods that are specific to working with JSON objects.

Instead of a class function to create a User object, I'd use a failiable initializer init?() that returns nil if an instance could not be created.

I'd also create some way to reference your string keys so that you're not typing them by hand each time. This could be a struct, or just a bunch of static let statements. I prefer the struct because it puts all the keys in a similar namespace that helps organize your code, but also helps with auto completion when you're typing out the keys.

import SwiftyJSON

extension User {

    // prevent accidental typos when working with string keys
    struct Keys {
        static let name = "name"
        static let email = "email"
        static let authToken = "auth_token"
        static let user = "user"
    }

    // convenience initalizer for making `User` from a JSON object
    convenience init?(in context: NSManagedObjectContext, with json: JSON?) {
        guard let json = json else { return nil }

        email = json[Keys.email].stringValue
        authToken = json[Keys.authToken].stringValue
    }

    // updating existing instance with data from a JSON object
    func update(from json: JSON?) {
        guard let json = json[Keys.user] else { return }

        name = json[Keys.name].stringValue
        email = json[Keys.email].stringValue

    }
}

Finally, I'd remove any code that calls save() on your context from within the User object, and have this in the controller. I'd probably also remove the code that fetches a User and also have this moved into  the view controller.

Personally, I'd also not call save() on the object when it's newly created, and instead defer that until some later time (such as when the view controller is exiting).

import UIKit

class ViewController: UIViewController {

    var context: NSManagedObjectContext?

    func someMethodThatCreatesUser() {

        guard let context = context else { return }

        let json: JSON?  // ... grab json
        guard let user = User(in: context, with: json) else { return }

        // do something with the user...
    }

    func someMethodThatGetsExistingUser() -> User? {

        guard let context = context else { return nil }

        let request: NSFetchRequest<User> = User.fetchRequest()
        // `fetchRequest()` is auto generated by Xcode, if you create
        // your own managed object subclasses you'll need to do some
        // additional work here.

        let users = try? context.fetch(request)
        return users?.first

    }

    deinit {
        guard let context = context else { return }
        context.perform {
            try? context.save()
        }
    }

}


Filter array of objects by comparing nested object properties I recently had a problem whereby I wanted to filter objects in an array by comparing the values of nested properties of each object to see if there were duplicates.

I have tried to explain as clearly as possible the scenario below and the solution I came up with. I wondering if it's possible to improve this solution and if so, how?

Example:

let articles = [{dateAdded: "31-5-1989", readId: 123, article: { id: 1}}, {dateAdded: "31-5-1989", readId: 124, article: { id: 2}}, {dateAdded: "31-5-1989", readId: 125, article: { id: 1}}]

In this array, you can see each object has a readId and an articleId. The first and last objects in the array have a different readId but a matching articleId. We want to return every object in this array unless we have already returned an object with the same articleId.

Here is my solution:

const removeDuplicatesbyArticleId = articles => {
  let articleIdsArray = [];
  let finalArray = [];

  articleResponse.reading.map(element => {
    if (!articleIdsArray.includes(element.article.id)) {
      articleIdsArray.push(element.article.id);
      finalArray.push(element);
    }
  });

  let filteredArticleResponse = articles;
  filteredArticleResponse.reading = finalArray;
  return filteredArticleResponse;
};

All I'm doing here is looping through the array, checking if the articleId exists in the articleIdsArray and if not, pushing this onto finalArray and then repeating before returning finalArray at the end of the loop.

This solution feels a little crude, I'd be really grateful if anyone could suggest an alternative(s) solution(s) that is easier to read, better for performance, or both!

I apologise if this is a repeat, from much searching I couldn't find a question and answer that matched my specific case.

We want to return every object in this array unless we have already returned an object with the same articleId.

I would use a Set object to keep track of which articleId values we've already used and then use .filter() to return only the items that pass the test.  Here's a runnable implementation:

let articles = [
  {dateAdded: "31-5-1989", readId: 123, article: { id: 1}},
  {dateAdded: "31-5-1989", readId: 124, article: { id: 2}},
  {dateAdded: "31-5-1989", readId: 125, article: { id: 1}}
];

function getUniqueArticles(array) {
    let ids = new Set();
    return array.filter(function(item) {
        if (!ids.has(item.article.id)) {
            ids.add(item.article.id);
            return true;
        }
        return false;
    });
}

console.log(getUniqueArticles(articles));

Note: Your articles array does not look valid as there seems to be a missing comma here in each array element and you probably mean to have quotes around the date to make it into a string:

{dateAdded: "31-5-1989", readId: 123, article: { id: 1}}
           ^^^       ^^^           ^^^


Function to find the kth match I've created a UDF some time ago and used it pretty often till now in different ways (but mainly to compare a "history" of data like having different filters at the same time). While there is no need to change anything at this state, I'd like get some suggestions of how to improve it without the loss of any functionality.

To make it short, it is a LOOKUP-function which goes for the xth match and returns a ref.

Normally there are comments, but I deleted them to test if it is still easy to get what it does. Also i like to have it in a way i don't need them.

Public Function VLOOKUPK(lookup_value As String, lookup_vector As Range, Optional result_vector As Range, _
                                  Optional count_num As Long = 1, Optional case_sens As Boolean) As Variant
  Application.Volatile

  Dim i(0 To 1) As Long, j As Long, lookup_vector_values As Variant
  VLOOKUPK = CVErr(2023)
  If count_num - 1 <> Abs(Round(count_num - 1)) Or (i(0) * i(1)) = 1 Then Exit Function

  Set lookup_vector = Intersect(lookup_vector.Parent.UsedRange, lookup_vector)
  If lookup_vector.Areas.Count > 1 Then Set lookup_vector = lookup_vector.Areas(1)
  i(0) = lookup_vector.Rows.Count
  i(1) = lookup_vector.Columns.Count

  If i(0) > 2 And i(1) > 2 Then Exit Function

  If result_vector Is Nothing Then
    If i(0) = 1 Or i(1) = 1 Then
      Set result_vector = lookup_vector

    ElseIf i(0) > 2 And i(1) = 2 Then
      Set result_vector = lookup_vector.Columns(2)
      Set lookup_vector = lookup_vector.Columns(1)

    ElseIf i(0) = 2 And i(1) > 2 Then
      Set result_vector = lookup_vector.Rows(2)
      Set lookup_vector = lookup_vector.Rows(1)

    Else
      Exit Function

    End If
  Else
    If result_vector.Columns.Count > 1 And result_vector.Rows.Count > 1 Then Exit Function
    Set result_vector = Intersect(result_vector.Parent.UsedRange, result_vector)

  End If

  If lookup_vector.Columns.Count > 1 And lookup_vector.Rows.Count > 1 Then Exit Function

  If Not case_sens Then lookup_value = LCase(lookup_value)
  If lookup_vector.Columns.Count > 1 Then j = 1
  lookup_vector_values = lookup_vector.Value
  i(0) = 1: i(1) = 1

  For i(j) = 1 To UBound(lookup_vector_values, j + 1)
    If lookup_value Like IIf(case_sens, lookup_vector_values(i(0), i(1)), LCase(lookup_vector_values(i(0), i(1)))) Then
      If count_num = 1 Then
        Set VLOOKUPK = result_vector.Cells(i(0), i(1))
        Exit Function

      Else
        count_num = count_num - 1

      End If
    End If
  Next
End Function

How to use it:

VLOOKUPK(lookup_value,lookup_vector,[result_vector],[count_num],[case_sens])

lookup_value (required)

The value to search for.
lookup_value can be used with wildcards like *.

lookup_vector (required)

The Range to look in.
It needs to contain only one row or column if the result_vector is submitted.
If no result_vector is set and the lookup_vector contains only 1 row/column, the result_vector will be set to the lookup_vector.
If no result_vector is set, and the lookup_vector contains 2 columns while holding more then 2 rows, the first column will be set as lookup_vector while the second column will become the result_vector. (and vice versa)

Having a range of 2x2 will result in an error.

result_vector (optional)

The range where the value to output is located.
Needs to contain only one row or column.

count_num (optional)

Indicates the kth match to work with.
if not submitted, the first occurrence will be submitted.

case_sens (optional)

if set to TRUE the search will be case sensitive.

Volatile

Application.Volatile

I'm walking down the code as I'm writing this, so I haven't really seen the code below that line. The Application.Volatile method call tells Excel to recalculate your function every time any cell anywhere on the worksheet is modified. The default behavior is to recalculate only when the input variables change.
That method call on the Application object tells me that you've perhaps tested it without at first, and concluded that it was required in order for the function to properly recalculate.
A comment explaining why Application.Volatile is needed here, might be judicious.

Naming notes...
I like the consistency you have with your naming style, it's nice to see. The style matches that of native Excel worksheet functions, like SUM:

SUMIF(range, criteria, [sum_range])

I think it's fine to expose that style to outside code (and/or Excel), but I don't see a reason for the internals to use any snake_case, convention is to use camelCase for locals (and usually, parameters too), and PascalCase for member names (procedures, functions, properties, ..).

Dim i(0 To 1) As Long, j As Long, lookup_vector_values As Variant

Hmm. 3 variables declared in the same instruction, of 3 different types and meanings. For readability's and maintainability's sake, a few points:

Declare arrays on their own line. As Long, j As Long is pretty much hiding the identifier.
If you're going to declare more than one variable in one instruction, do so but with variables of the same type.
Use meaningful identifier names: i is usually used in a For..Next loop, to count iterations. An array of Int32's isn't exactly trivially that. We don't know anything about that variable until we stumble on a usage of it, which hopefully isn't too far. The first usage is actually 2 lines below, which isn't too bad, but... let's face it, we all hate it when it's down to i and l and 1.
j is also typically used for iterating Foo..Next loops, usually when i is already taken. I'll have to scroll further down to see where the loop is. An interesting loop, I'll get back to it.
Declare variables as close as possible to their first usage.

I'd get rid of i, and rename lookup_vector to lookup_range or, as VLOOKUP has it, table_array (although, ...)
I said I was going to get back to the loop:

For i(j) = 1 To UBound(lookup_vector_values, j + 1)

Why make that operation a side-effect of iterating the loop? It's non-standard; people usually use a variable in a For loop, not an expression - one must read it carefully. If it can't be refactored, then it should be commented.


Approximation search source reconstruction localization algorithm Goal
To determine the coordinates of some signal source in a 3D space, given the coordinates of four observers and the time at which each saw the signal, as well as the velocity of the signal.
Algorithm
The algorithm is more-or-less comprised of two pieces: the approximation search algorithm, and the localization algorithm. Approximation search can be described simply as follows:
Suppose we have some function, f(x), and we want to find an x such that f(x) = y. We begin by choosing some search interval, [xi, xf], and we probe some evenly-spaced points along this interval with some step, da. We remember the point with the least error, and feed that error back into the algorithm. We recursively increase accuracy, reducing da and restricting the search interval to around each progressively better solution. We stop when we've reached the maximum number of iterations (configurable).
The localization bit is simply the computation of the error. I could explain it, however I find that the code does so itself far better than I could hope to in words.
Issues
In my opinion, the code is a bit messy and slightly more difficult to read than I'd like. Similarly, I'm not sure how I feel about the nested loops. Of course, I don't want to overcomplicate things simply to get rid of nested loops, however if there's a nicer way to do this, I'm all ears.
It's also slow. This is Python, of course, so I shouldn't expect it to perform like a similarly implemented algorithm in, say, C/C++, however I'm certainly interested in increasing search speed.
Lastly, it's been a while since I've done anything in Python, so I'm interested in a critique of my style. The goal is readability, and not necessarily 'standards' compliance.
Tear it apart. Don't hold back!
import math
from dataclasses import dataclass

@dataclass
class Approximate:

    ai: float;  af: float
    da: float;   n : int

    # Default values
    aa: float = 0
    ei: float = -1
    a : float = 0
    e : float = 0
    i : int   = 0

    done: bool = False
    stop: bool = False

    def step(self):

        if (self.ei < 0) or (self.ei > self.e):

            self.ei = self.e
            self.aa = self.a

        if (self.stop):

            self.i += 1

            if (self.i >= self.n):

                self.done = True
                self.a = self.aa

                return  # Max iter

            # Restrict to window around 'solution'
            self.ai = self.aa - self.da
            self.af = self.aa + self.da

            self.a = self.ai

            # Increase accuracy
            self.da *= 0.1
            self.ai += self.da
            self.af -= self.da

            self.stop = False

        else:

            # Probe some points in [ai,af] with step da
            self.a = self.a + self.da

            if (self.a > self.af):

                self.a = self.af
                self.stop = True

def localize(recv):

    ax = Approximate(0, 5000, 32, 10)
    ay = Approximate(0, 5000, 32, 10)
    az = Approximate(0, 5000, 32, 10)

    dt = [0, 0, 0, 0]

    while not ax.done:
        ay = Approximate(0, 5000, 32, 10, 0, 0)

        while not ay.done:
            az = Approximate(0, 5000, 32, 10, 0, 0)

            while not az.done:

                for i in range(4):
                    x = recv[i][0] - ax.a
                    y = recv[i][1] - ay.a
                    z = recv[i][2] - az.a

                    baseline = math.sqrt((x * x) + (y * y) + (z * z))

                    dt[i] = baseline / 299800000

                    # Normalize times into deltas from zero
                baseline = min(dt[0], dt[1], dt[2], dt[3])

                for j in range(4):
                    dt[j] -= baseline

                error = 0

                for k in range(4):
                    error += math.fabs(recv[k][2] - dt[k])
                    ay.e = error
                    ax.e = error

                az.step()

            ay.step()

        ax.step()

    # Found solution
    print(ax.aa)
    print(ay.aa)
    print(az.aa)
```

Remove empty lines after if/else. Remove empty lines in general, this has too many. Use the new math.dist. Add comments about how the approximation works. Add docstrings.
If you want to make it more readable, pull out sub-loops into functions. That reduces nesting depth, which is just too high here--it's a problem you don't run into often. Also, don't use names like 'ei'.
One approach to make it more readable (but maybe slower), rewrite 'approximate' as a function taking in: a function to compute, an interval to search, and a maximum number of iterations. Also, you could use scipy or another specialized package which has this built-in.
If you want to make it more readable AND fast, don't use this solution. Use some other maximization method, for example gradient descent. These are also available as prebuilt tools, so it should be readable.
Last, in this case, I think there is probably a very direct answer specific to the problem. Look up GPS algorithms, because this is more or less how they work (although the speed is always the speed of light)


Simple run-length encoding decoder For practice, I decided to write a run-length decoder. For example:

basic_run_length_decode("1A2B3C")
=> 'ABBCCC'

basic_run_length_decode("5A10Z10J")
=> 'AAAAAZZZZZZZZZZJJJJJJJJJJ'

It ended up getting long, but that's mostly because I wanted try incorporating the partition-by function from Clojure. It seemed like it would work nicely here, and it did. It required me to write that function though since Python doesn't seem to have anything equivalent. I also "stole" grouper from the "recipe" section of the itertools docs. Clojure has both of these functions as part of its standard library, so that's what I thought of while writing this.

Basically, it works by cutting the string into groups of digits and non-digits, pairing them into [digits, word] pairs, then parsing the digits and doing some string multiplication.

What I'd like commented on mainly:

partition_by. I tried to see if there was a sane way of using something like takewhile + drop_while, but both of those functions consume an extra element after the predicate flips, which caused problems. I opted to just do manual iteration, but it's quite verbose.
Anything else that I may be doing the hard way.

Note, it actually allows for multiple characters per number.

basic_run_length_decode("10AB10CD")
=> 'ABABABABABABABABABABCDCDCDCDCDCDCDCDCDCD'

I thought about raising an error, but decided to just allow it. This is apparently a type of possible encoding, and it cost me nothing to allow it.

from itertools import zip_longest
from typing import Callable, Any, Iterable, TypeVar, Generator, List

T = TypeVar("T")
# Emulating https://clojuredocs.org/clojure.core/partition-by
def _partition_by(f: Callable[[T], Any], iterable: Iterable[T]) -> Generator[List[T], None, None]:
    """Splits off a new chunk everytime f returns a new value.
    list(partition_by(lambda n: n < 5, [1, 2, 3, 6, 3, 2, 1])) => [[1, 2, 3], [6], [3, 2, 1]]
    """
    last_value = object()  # Dummy object that will never be equal to anything else
    chunk = []

    for x in iterable:
        returned = f(x)

        if returned == last_value:
            chunk.append(x)

        else:
            if chunk:
                yield chunk

            chunk = [x]

        last_value = returned

    if chunk:
        yield chunk

# Recipe from https://docs.python.org/3.8/library/itertools.html#itertools.takewhile
def _grouper(iterable, n, fillvalue=None):
    """Collect data into fixed-length chunks or blocks"""
    args = [iter(iterable)] * n
    return zip_longest(*args, fillvalue=fillvalue)

def _invalid_encoded_error(message: str) -> ValueError:
    return ValueError(f"Invalid encoded string. {message}")

def basic_run_length_decode(encoded_string: str) -> str:
    """Decodes a string in the form of <number><chars><number><chars>. . .
    basic_run_length_decode("1A2B3C") => 'ABBCCC'"""
    if encoded_string and not encoded_string[0].isdigit():
        raise _invalid_encoded_error("First character must be a number.")

    chunks = _partition_by(lambda c: c.isdigit(), encoded_string)

    acc = []
    for raw_n, raw_word in _grouper(chunks, 2):
        if not raw_word:
            raise _invalid_encoded_error("Trailing numbers without character at end of encoded string.")

        n = int("".join(raw_n))  # Should never throw as long as the very first char is a digit?
        word = "".join(raw_word)

        acc.append(n * word)

    return "".join(acc)

The code looks good to me, it is well documented and I do not have much to say.

Suggestion for _partition_by

In:

    if returned == last_value:
        chunk.append(x)
    else:
        if chunk:
            yield chunk
        chunk = [x]

I'd tried to write chunk = [x] as chunk = [] ; chunk.append(x). This allows some rewriting as you can extract the common line out of the if / else.

    if returned == last_value:
        pass
    else:
        if chunk:
            yield chunk
        chunk = []
    chunk.append(x)

Then revert condition

    if returned != last_value:
        if chunk:
            yield chunk
        chunk = []
    chunk.append(x)

Then group condition as there is nothing to do when chunk is already empty.

    if chunk and returned != last_value:
        yield chunk
        chunk = []
    chunk.append(x)

There is nothing wrong with the way things were done, this is a pure personal preference.

Details about docstring

The docstrings are slightly inconsistent as we have two different forms for the verb: with and without the final 's'. If that can be of any help, there are Docstring conventions for Python in PEP 257.

It suggests:

  The docstring is a phrase ending in a period. It prescribes the
  function or method's effect as a command ("Do this", "Return that"),
  not as a description; e.g. don't write "Returns the pathname ...".


Simple PDO database class in PHP I'm writing a simple database class so I could use it in my future projects. It is based on some Codeigniter database methods, but my implementation, so if you could review this, that would be cool. It is not finished yet, but I want to remove all bad code early on.

<?php

class Database {

    private static $instance = null;
    private $pdo;
    private $query;
    private $results;
    private $count = 0;
    private $error = false;

    private $query_string = "";
    private $bindValues = array();
    private $lastId;

    private function __construct() {

        try {
            // Put your database information
            $this->pdo = new PDO("mysql:host=127.0.0.1;dbname=login","root","");
        } catch (PDOException $e) {
            die($e->getMessage());
        }

    }

    public static function getInstance() {
        if (is_null(self::$instance)) {
            self::$instance = new Database();
        }
        return self::$instance;
    }

    public function query($sql, $parameters = array()) {
        $this->error = false;

        if ($this->query = $this->pdo->prepare($sql)) {
            $i = 1;
            foreach ($parameters as $param) {
                $this->query->bindValue($i, $param);
                $i++;
            }
            if ($this->query->execute()) {
                // You can PDO::FETCH_OBJ instad of assoc, or whatever you like
                $this->results = $this->query->fetchAll(PDO::FETCH_ASSOC);
                $this->count = $this->query->rowCount();
                $this->lastId = $this->query->lastInsertId();
            } else {
                $this->error = true;
            }
        }
        return $this;
    }

    public function select($fields = "*") {
        $action = "";
    $this->query_string = "";
        if (is_array($fields)) {
            $action = "SELECT ";
            for ($i = 0; $i < count($fields); $i++) {
                $action .= $fields[$i];
                if ($i != count($fields) - 1)
                    $action .= ', ';
            }
        } else {
            $action = "SELECT * ";
        }
        $this->query_string .= $action;
        return $this;
    }

    public function from($table) {
        $this->query_string .= " FROM {$table} ";
        return $this;
    }

    public function where($where = array()) {
        $keys = array_keys($where);
        $action = " WHERE ";
        for ($i = 0; $i < count($keys); $i++) {
            $action .= $keys[$i] . ' = ?';
            if ($i < count($keys) - 1)
                $action .= ' AND ';
            $this->bindValues[] = $where[$keys[$i]];
        }
        $this->query_string .= $action;
        return $this;
    }

    public function execute() {
        if (!empty($this->query_string))
            $this->query($this->query_string, $this->bindValues);
        $this->bindValues = array();
    }

    public function getQueryString() {
        return $this->query_string;
    }

    public function results() {
        return $this->results;
    }
    public function first() {
        return $this->results[0];
    }
    public function last() {
        return $this->results[$this->count-1];
    }
    public function row($id) {
        return $this->results[$id];
    }
    public function error() {
        return $this->error();
    }
    public function count() {
        return $this->count;
    }
    public function lastId() {
        return $this->lastId;
    }
}

My suggestion is to not do what you are trying to do.  Why do you want to wrap PDO?

This is a common type of code review that I see here and in most cases the attempt to "wrap" a perfectly good abstraction library like PDO is misguided.

Please look at the discussion on these threads:

Class for reducing development time
PHP PDO Wrapper

Given that, let me give you some thoughts on what you do have.

Don't mix snake case and camel case in your codebase.  I know PHP is pretty bad about mixing these two, but that doesn't mean you should be.

You have a class that is doing two things - acting as a singleton and acting like a DB wrapper.  If you truly want to move forward with having a wrapper, you should at least consider moving the singleton behavior into it's own class, with the singleton managing the creation or retrieval of the Database object (or PDO object if you drop the wrapper idea).  Name that class appropriately like DatabaseSingleton, PDOSingleton, or similar.

private function __construct() {

    try {
        // Put your database information
        $this->pdo = new PDO("mysql:host=127.0.0.1;dbname=login","root","");
    } catch (PDOException $e) {
        die($e->getMessage());
    }

}

Don't just assume that PDO instantiation works.  You could be assigning a bad value to $this->pdo.
Don't put your DB credentials here in this class.  They should be in app config.
Don't use "root" for this application. Create a user in MySQL for this application, with appropriate permissions.  Don't use "" as your password, especially in combination with "root". Your MySQL server could be vulnerable.
Don't echo out application error messages to standard output.
You try-catch block is essentially useless here as it is not really doing anything and is, in fact, hiding the error from calling code which is likely in better position to determine how to recover from the failure and/or message the end user.

public function query($sql, $parameters = array()) {

This method is poorly named. Perhaps something like prepare_and_execute is more descriptive of what is happening here.
You are performing no validation whatsoever for the data passed.  You need to verify that $sql is (at a minimum) a non-zero length string and that you have an array passed for $parameters.  Your code could break if those conditions aren't met, so you need to fail out of the method (possibly with INvalidArgumentException being thrown) before you ever get to the point of trying to prepare a statement. You should have parameter type-hinting and/or validation as the first thing that happens in ANY publicly accessible method.
This method has removed user's ability to do any parameter type enforcement in their bindings.
This method rally only considers happy path.  What if prepare fails (right now you silently just return the object with no indication to caller that something went wrong). Same with bindValue and execute().  To reinforce the point about pointlessness of this wrapper.  You have now written a number of lines of code here to make working with PDO less flexible, more fragile, and less transparent to the caller.

        $this->results = $this->query->fetchAll(PDO::FETCH_ASSOC);
        $this->count = $this->query->rowCount();
        $this->lastId = $this->query->lastInsertId();

This is just odd.  In what scenario would a single query have both a result set and an insert ID?

I know that people may like the natural language interfaces like you are trying to achieve with select(), where(), etc. methods. There are a number of good libraries out there that can give you this functionality, and I guarantee you those methods in those libraries contain a lot more code in them to address all the data validation and edge cases you are missing (like what if I want to OR my WHERE filters).  How much benefit are you really getting from this functionality?

Is this:

$result = $database->select('*')->from('table')->where(["column = 'abc'"])

Really easier to read and understand than:

$sql = "SELECT * FROM table WHERE column = 'abc'";

What happens when you need to perform join, order by, limit, etc.?  With your current class, you just end up having to revert to using raw SQL anyway because your current implementation is very basic in terms of functionality.

By the way defaulting to (or even allowing) SELECT * is something you ought to think twice about.  SELECT * is bad.  SELECT * can quietly break your applications when there are schema changes. SELECT * can suck up a lot more bandwidth in transmission than is required for any given piece of logic to execute. SELECT * makes it difficult for someone only looking at code to understand what sort of data is being made available to them.

The last(), first(), row() methods are ill-conceived.  These concerns should really be addressed by LIMIT clauses in SQL.  Why do you need to retrieve and store every row from a table if you are only interested in row X?


Testing a probabilistic prime-testing algorithm that may fail I have implemented Pollard's Rho algorithm in Java. Due to the nature of it there is a small chance for it to fail (have not seen it happening yet).

Since I do not want to see my unit tests failing due to this "too often", I set a retry, with which I am not totally satisfied, as you can see here:

@Test
void primeFactorsCalculatedWithPollardsRhoAreCorrect() {
    Operations op = new Operations();
    int remainingAttempts = 5;

    while (true) {
        try {
            assertEquals(5, op.primeFactorsPollardRho(13195));
            break;
        } catch (UnsupportedOperationException e) {
            remainingAttempts -= 1;

            if (remainingAttempts == 0) {
                throw e;
            }
        }
    }
}

I am using jUnit.

Is there a better, more pleasing to the eye, way of doing this?

Your test is missing the central point of unit testing which is to prove the code under test works in the specific circumstances in question, these should be deterministic.  So for each parameter you can know in advance what the result should be.  Create multiple unit tests for the known factors that should work and a number for the parameters expected to fail.

Secondly, I suggest you create your own Checked Exception for the failing circumstance rather than reusing the runtime UnsupportedOperationException, this will ensure a client using your function/library is forced to handled the exception.  When the error can be handled use a checked exception, only use the unchecked runtime exceptions when the error cannot be handled by the code.  If the program can, it should still fail gracefully with either.  I can see that is what you are trying to achieve for your live code but visibility of the issue is more important in unit tests.

The client code handling exception could skip the value and proceed to the next value.  Simply retrying with the same value should produce the same result, so the retry with the same value is actually pointless.

You can verify that the test produces the result you expect by catching the expected failure with the expected Exception clause of the @Test annotation.

@Test(expected = BirthdayParadoxException.class)
public void testBirthdayParadox() {
    op.primeFactorsPollardRho(BIRTHDAY_PARADOX_VALUE);
}

@Test
public void testPassingValue() {
    assertEquals(EXPECTED_RESULT, op.primeFactorsPollardRho(FACTORABLE_VALUE);
}

Create multiple tests for each specific situation you can predict and verify the expected behaviour, catching failures should be considered expected behaviour for unit testing purposes.

Also see : https://github.com/junit-team/junit4/wiki/Exception-testing


Inefficient binary search? Hackerrank - Climbing the Leaderboard I'm learning Golang and have been trying to get 100% on the following Hackerrank practice challenge: Climbing the Leaderboard

The code passes ~8 of 11 tests. The Hackerrank tests are time-bound and my code is not efficient enough to past the most intensive tests.

I'm removing duplicates from the scores using a map. Then work through each of Alice's scores to find the index of each one by doing a binary search. Given that Alice's scored are sorted, I re-use the index for the next search.

Any ideas?

package main

import (
    "fmt"
)

func main() {

    // example 1
    scores := []int32{100, 100, 50, 40, 40, 20, 10}
    alice := []int32{5, 25, 50, 120}
    r := climbingLeaderboard(scores, alice)
    for a, v := range r {
        fmt.Printf("Position: %d alice[a]: %d\n", v, alice[a])
    }
    fmt.Printf("\n")
}

func removeDuplicates(a []int32) []int32 {
    r := []int32{}
    seen := map[int32]int32{}
    for _, val := range a {
        if _, ok := seen[val]; !ok {
            r = append(r, val)
            seen[val] = val
        }
    }
    return r
}

func getMidPoint(start, end int) int {
    i := end - start
    if i%2 == 0 {
        return i/2 + start
    }
    return (i+1)/2 + start
}

// 100 100 50 40 40 20 10
// 100 50 40 20 10
// 0   1  2  3  4
func binarySearch(scores []int32, v int32, i int) int {
    var mid int
    start := 0
    end := i

    for true {
        if end-start == 1 {
            //fmt.Printf("1 - v: %d start: %d end %d\n", v, start, end)
            if v < scores[end] {
                return end + 2
            } else if v < scores[start] {
                return start + 2
            }
            return 1
        }
        if end == 0 {
            return 1
        }

        mid = getMidPoint(start, end)

        if v > scores[mid] {
            end = mid
        } else if v < scores[mid] {
            start = mid
        } else {
            // v == scores[mid]
            //fmt.Printf("2 - v: %d start: %d end %d\n", v, start, end)
            return mid + 1
        }
    }
    return end
}

// attempt 3 - binary search
func climbingLeaderboard(originalScores []int32, alice []int32) []int32 {

    r := make([]int32, len(alice))

    //create scores and remove dups
    scores := removeDuplicates(originalScores)
    i := len(scores)

    for a, v := range alice {
        // fmt.Printf("1 - i: %d a: %d alice[a]: %d\n", i, a, alice[a])
        i = binarySearch(scores, v, i-1)
        r[a] = int32(i)
    }

    return r
}

As @hoffmale correctly pointed out, the Hackerrank code to read in the input files on the larger tests was causing a problem. More specifically it didn't have enough memory to read in the array. So I increased the size of the reader  and the writer (just in case). I also made a change to my binary search to ensure end would not be out of bounds on scores.

reader := bufio.NewReaderSize(os.Stdin, 1024*1024*3)

package main

import (
    "bufio"
    "fmt"
    "io"
    "os"
    "strconv"
    "strings"
)

func removeDuplicates(a []int) []int {
    r := []int{}
    seen := map[int]int{}
    for _, val := range a {
        if _, ok := seen[val]; !ok {
            r = append(r, val)
            seen[val] = val
        }
    }
    return r
}

func getMidPoint(start, end int) int {
    i := end - start
    if i%2 == 0 {
        return i/2 + start
    }
    return (i+1)/2 + start
}

func binarySearch(scores []int, v int, i int) int {
    var mid int
    var end int

    start := 0
    if i >= len(scores) {
        end = len(scores) - 1
    } else {
        end = i
    }

    for true {
        if end-start == 1 {
            // fmt.Printf("1 - v: %d start: %d end %d\n", v, start, end)
            if v < scores[end] {
                return end + 2
            } else if v < scores[start] {
                return start + 2
            }
            return 1
        }
        if end == 0 {
            return 1
        }

        mid = getMidPoint(start, end)

        if v > scores[mid] {
            end = mid
        } else if v < scores[mid] {
            start = mid
        } else {
            // v == scores[mid]
            // fmt.Printf("2 - v: %d start: %d end %d\n", v, start, end)
            return mid + 1
        }
    }
    return end
}

// attempt 3 - binary search
func climbingLeaderboard(originalScores []int, alice []int) []int {

    r := make([]int, len(alice))

    //create scores and remove dups
    scores := removeDuplicates(originalScores)
    i := len(scores)

    for a, v := range alice {
        // fmt.Printf("1 - i: %d a: %d alice[a]: %d\n", i, a, alice[a])
        i = binarySearch(scores, v, i-1)
        r[a] = i
    }

    return r
}

func main() {
    reader := bufio.NewReaderSize(os.Stdin, 1024 * 1024*3)

    stdout, err := os.Create(os.Getenv("OUTPUT_PATH"))
    checkError(err)

    defer stdout.Close()

    writer := bufio.NewWriterSize(stdout, 1024 * 1024*3)

    scoresCount, err := strconv.ParseInt(readLine(reader), 10, 64)
    checkError(err)

    scoresTemp := strings.Split(readLine(reader), " ")

    var scores []int

    for i := 0; i < int(scoresCount); i++ {
        scoresItemTemp, err := strconv.ParseInt(scoresTemp[i], 10, 64)
        checkError(err)
        scoresItem := int(scoresItemTemp)
        scores = append(scores, scoresItem)
    }

    aliceCount, err := strconv.ParseInt(readLine(reader), 10, 64)
    checkError(err)

    aliceTemp := strings.Split(readLine(reader), " ")

    var alice []int

    for i := 0; i < int(aliceCount); i++ {
        aliceItemTemp, err := strconv.ParseInt(aliceTemp[i], 10, 64)
        checkError(err)
        aliceItem := int(aliceItemTemp)
        alice = append(alice, aliceItem)
    }

    result := climbingLeaderboard(scores, alice)

    for i, resultItem := range result {
        fmt.Fprintf(writer, "%d", resultItem)

        if i != len(result) - 1 {
            fmt.Fprintf(writer, "\n")
        }
    }

    fmt.Fprintf(writer, "\n")

    writer.Flush()
}

func readLine(reader *bufio.Reader) string {
    str, _, err := reader.ReadLine()
    if err == io.EOF {
        return ""
    }

    return strings.TrimRight(string(str), "\r\n")
}

func checkError(err error) {
    if err != nil {
        // fmt.Println("Error: " + err.Error())
        panic(err)
    }
}


Search for User by first and/or last name, efficiently I am refactoring a user search that just feels dirty. (Users#search)

I need to allow a blank param to search on partial or only a first or last, but don't want to return all the records in the database (which is what seems to be happening when I search with one value nil).

That said, what's the best way to handle one blank param / nil value?
What's the most succinct, efficient way to do this?

Here is where I started:

    def user_search ## this is my before_action
      if params[:fname] || params[:lname]
        @users = User.where("first_name LIKE ? OR last_name LIKE ?", "%#{params[:fname]}%", "%#{params[:lname]}%")
      elsif params[:email_search]
        @users = User.where("email LIKE ?", "%#{params[:email_search]}%")
      end
      head 404 if @users.blank?
    end

Here is where I am:

    def user_search
      if params[:fname].presence && params[:lname].presence
        @users = User.where("first_name LIKE ? AND last_name LIKE ?", "%#{params[:fname]}%", "%#{params[:lname]}%")
      elsif query = (params[:fname] || params[:lname])
        @users = User.where("first_name LIKE ? OR last_name LIKE ?", "%#{query}%", "%#{query}%")
      elsif params[:email_search]
        @users = User.where("email LIKE ?", "%#{params[:email_search]}%")
      end
      head 404 if @users.blank?
    end

The significance behind the SQL of OR versus AND seems to be a convenient way to query and return matches on one of the names, but is it really efficient? Looking for a first_name in the last_name column sounds kind of silly hackery. Unless I'm looking at this the wrong way.

Is there a more performant way to do this?

These are just some ideas based on some assumptions, so I may be way off.

  Looking for a first_name in the last_name column sounds kind of silly hackery. Unless I'm looking at this the wrong way.

The "hacky" feeling is maybe not so much because of the code itself, but elsewhere in the design. It sounds like a "hacky" user-facing UI or client-server API, rather than hacky code, is to blame.

I assume the user interface (if there is one) always shows both a first name and a last name input field of some kind. If that's the case, isn't it valid to only search for the value(s) the user provides?

I certainly wouldn't expect to get results for last name OR first name matches if I only provide a "last name" search string. Similar, I wouldn't expect to find both "Daniel Craig" and "Timothy Dalton" if I type in "Da" in the first name field.

Even if the client has no obvious or direct user input, I'd still say the API should be clear so that an fname param searches for first name, and only first name matches, while lname is last-name-only.

Overloading a user interface or API with non-obvious, special functionality is no different than overloading a method with strange behavior that triggers when pass it certain arguments.

In other words, I'm not sure the OR case makes sense, given input that's so specific and the (assumed to be) equally specific UI.

So, if we can eliminate the OR query (and I admit it's all assumptions here), I'd might do something like:

# get the search parameters, but only
# keep those that that are actually present
search_params = {
  first_name: params[:fname],
  last_name:  params[:lname]
}.keep_if { |_, value| value.present? }

# now do the search
@users = if search_params.any?
  # chaining `where` calls will implicitly add the `AND` in between
  search_params.inject(nil) do |memo, pair|
    column, string = pair
    (memo || User).where("#{column} LIKE ?", "%#{string}%")
  end
elsif params[:email_search].present?
  User.where("email LIKE ?", "%#{params[:email_search]}%")
end

head :not_found if @users.blank?

Haven't tested it, but it should work.

Alternatively, you can leave the "search for name" vs "search for email" decision to the client, and simply say the the API will return for whatever matches all the criteria you send it; if you don't want something matched, don't send it. In that case, the elsif isn't even necessary:

search_params = {
  first_name: params[:fname],
  last_name:  params[:lname],
  email:      params[:email_search]
}.keep_if { |_, value| value.present? }

@users = search_params.inject(nil) do |memo, pair|
  column, string = pair
  (memo || User).where("#{column} LIKE ?", "%#{string}%")
end

head :not_found if @users.blank?

The client can then implement its UI/code however it sees fit, and send whatever params it needs to. Yes, it's just moving the problem, but your API is a lot clearer.

Of course, another solution still would be to index your data in some fashion, so you could just search on any field in one go by just passing a q param or something. But that's another kettle of fish entirely.

Again, just ideas based on assumptions.


O(lg n) algorithm for a fibonacci number with c++ template I wrote a function to find a specific fibonacci number, which runs in O(lg n). Then I modified it with a template. Though it works, I'm new to template and want to know if I used template correctly. Moreover, I wonder whether someone who has a BigNumber class
can use my function without modify my source code.

#include <iomanip>
#include <iostream>
#include <cassert>
#include <cstdlib>
#include <cstring>

using namespace std;

const int initFibo[5] = {5,3,2,1,1};
template <class U, class V>
void fastFibo(U n, V *tuple) {
    assert(n >= 4);
    if(n <= 5) {
        for(int i = 0; i < 4; i++)
            tuple[i] = initFibo[i + (5 - n)];
        return;
    }
    V tmp[4];
    fastFibo<U, V>(n / 2 + 1, tmp);
    if(n % 2 == 0) { // even
        tuple[0] = tmp[0] * tmp[1] + tmp[1] * tmp[2];
        tuple[1] = tmp[1] * tmp[1] + tmp[2] * tmp[2];
        tuple[2] = tmp[1] * tmp[2] + tmp[2] * tmp[3];
        tuple[3] = tmp[2] * tmp[2] + tmp[3] * tmp[3];
    } else {
        tuple[0] = tmp[0] * tmp[0] + tmp[1] * tmp[1];
        tuple[1] = tmp[0] * tmp[1] + tmp[1] * tmp[2];
        tuple[2] = tmp[1] * tmp[1] + tmp[2] * tmp[2];
        tuple[3] = tmp[1] * tmp[2] + tmp[2] * tmp[3];
    }
}

int main(){
    unsigned long long tuple[4];
    for(int i = 4; i <= 93; i++){
        fastFibo<int, unsigned long long >(i, tuple);
        cout << "Fibo(" << setw(2) << i << ")=" << setw(20) << tuple[0] << endl;
    }
    system("pause");
    return 0;
}

@Yuushi:

"Why does n have to be >= 4?... passing a size and a pointer..." Required by my algorithm, n must be >= 4 and tuple is always an array of size 4 (not n) (After invocation tuple will contains 4 fibonacci numbers: the nth, n-1th, n-2th and n-3th). I thought up an idea to create a method like this:

V fastFibo2(U n){
    if( n < 4 )
        return initFibo[5 - n];
    else{
        V buf[4];
        fastFibo(n, buf)
        return buf[0];
    }
}

However if I create it I don't want user to access the ordinary fastFibo. Is it possible or is there a better solution?

As always, using namespace std; is dangerous. In fact, there is a tuple class in  C++11, std::tuple. You could very easily have a naming collision here thanks to that.

Why bother using a template for the size if you compare it to an int anyway? It's overkill here. If you do decide to keep it, you should give your template parameters more descriptive names here. T is used when there really is no information about what the possible type could be, as in container classes. However, the value here is going to have to be some kind of integral constant - so call it as such.

template <class IntegralValue, class IntegralValue2>
void fastFibo(IntegralValue n, IntegralValue *tuple)

Why does n have to be >= 4? What happens if the user wants the value at 1? This shouldn't throw up an error (or in this case, call abort). This violates the principle of least surprise.

Finally, passing a size and a pointer as parameters is the C way of doing things. In C++, prefer to use the concept of Iterators:

template <typename Iterator>
void fastFibo(Iterator begin, Iterator end)

This makes the code below a little bit messier, but doesn't force the user to utilize an array. They could use a std::vector (preferable), or even a list, set, or their own custom data structure to store the results. You can get the distance between two iterators using std::distance.

Edit: There are a couple of solutions to your problem. Using another function which forwards to fastFibo is a reasonable solution here, as it hides the user from some of the implementation details. With templates, it's a bit trickier to hide things (since they need to be in header files). One way is using a namespace like detail (which generally indicates that the things inside it are internal and shouldn't be used directly). Another possibility is to use a class or struct with static member functions: the call then looks very much like using a namespace, but you can utilize access protection modifiers. For example:

struct fib
{
private:
    const static int initFibo[5] = {5,3,2,1,1};

    template <class U, class V>
    static void fastFibo_impl(U n, V *tuple) {
        assert(n >= 4);
        if(n <= 5) {
            for(int i = 0; i < 4; i++)
                tuple[i] = initFibo[i + (5 - n)];
            return;
        }
        V tmp[4];
        fastFibo_impl<U, V>(n / 2 + 1, tmp);
        if(n % 2 == 0) { // even
            tuple[0] = tmp[0] * tmp[1] + tmp[1] * tmp[2];
            tuple[1] = tmp[1] * tmp[1] + tmp[2] * tmp[2];
            tuple[2] = tmp[1] * tmp[2] + tmp[2] * tmp[3];
            tuple[3] = tmp[2] * tmp[2] + tmp[3] * tmp[3];
        } else {
            tuple[0] = tmp[0] * tmp[0] + tmp[1] * tmp[1];
            tuple[1] = tmp[0] * tmp[1] + tmp[1] * tmp[2];
            tuple[2] = tmp[1] * tmp[1] + tmp[2] * tmp[2];
            tuple[3] = tmp[1] * tmp[2] + tmp[2] * tmp[3];
        }
    }

public:

    template <typename U, typename V>
    static V fastFibo(U n){
        if( n < 4 )
            return initFibo[5 - n];
        else {
            V buf[4];
            fastFibo_impl<U, V>(n, buf)
            return buf[0];
        }
    }
};

which is then called by fib::fastFibo<...>(...).

I also noticed I didn't answer one of your questions. If someone wishes to use this with their own BigNumber class, then their class would need to support operator*, operator+, and operator= (and have those functions perform integer multiplication, integer addition, and integer assignment). This falls under the idea of Concepts, which were slated for inclusion into C++11, but were dropped due to not being ready. Suffice to say, as long as the user has a BigNumber class that "acts like" an integer with respect to *, + and =, is DefaultConstructable (since you call the default constructor for V in V buf[4]), and has an overload of operator+ taking a BigNumber and an int, they could use it in your template without having to touch anything.


Show a diamond shape with numbers After seeing this post on StackOverflow, I thought I'd try it out. What I have now is a diamond shape with numbers like this:

    1
   212
  32123
 4321234
543212345
 4321234
  32123
   212
    1

My code looks like this:

int i, j, k, n = 6, l = 3, o = 2;
for (i = 1; i <= 5; i++)
{
    for (k = n; k >= 1; k--)
    {
        Console.Write(" ");
    }
    n--;
    for (j = i; j >= 1; j--)
    {
        Console.Write(j);
    }
    o = 2;
    for (j = 1; j < i; j++)
    {
        Console.Write(o);
        o++;
    }
    Console.WriteLine();
}
for (i = 4; i >= 1; i--)
{
    for (k = l; k >= 1; k--)
    {
        Console.Write(" ");
    }
    l++;
    for (j = i; j >= 1; j--)
    {
        Console.Write(j);
    }
    o = 2;
    for (j = i; j > 1; j--)
    {
        Console.Write(o);
        o++;
    }
    Console.WriteLine();
}
Console.Read();

I don't think this is a nice solution for this task. Is there any nicer and shorter way to solve this without using that many loops?

Code issues that decrease readability and maintainability

Plenty of loops.
Non-meaningful variable names: n, l, o.

We can declare the following helper methods:

private static string CreateDigitsString(int length)
{
    char[] buffer = new char[length];
    for (int i = 0; i < buffer.Length; i++)
    {
        buffer[i] = (char)('1' + Math.Abs(buffer.Length / 2 - i));
    }
    return new string(buffer);
}

private static string CreateLine(int length, int spaces)
{
    return new string(' ', spaces) + CreateDigitsString(length - 2 * spaces) + new string(' ', spaces);
}

Then use it like:

const int N = 5;
const int TotalRows = N * 2 - 1;

for (int row = 0; row < TotalRows; row++)
{
    Console.WriteLine(CreateLine(TotalRows, Math.Abs(N - row - 1)));
}

Another approach
In the 2-dimensional loop (i, j) we can calculate the distance between the current cell and the center of the diamond as follows:

int rowDistance = Math.Abs(row - N + 1);
int colDistance = Math.Abs(col - N + 1);

Let's output the colDistance for cells that are within rowDistance + colDistance <= N - 1, and space - otherwise.

const int N = 5;
const int TotalSize = N * 2 - 1;

for (int row = 0; row < TotalSize; row++)
{
    for (int col = 0; col < TotalSize; col++)
    {
        int rowDistance = Math.Abs(row - N + 1);
        int colDistance = Math.Abs(col - N + 1);
        Console.Write(rowDistance + colDistance <= N - 1 ? (char)('1' + colDistance) : ' ');
    }
    Console.WriteLine();
}


Generalized PHP function for editing data in a table I've got some legacy code which I need to maintain and its got this function which works perfectly fine, but I'm trying to understand if it is working using acceptable coding practices or not... I am trying to understand if it is safe and secure to operate the site with the function as-it-is.

The function would be called as:

$this->editData('users',"activation_key",$_GET['key']);

The actual function (unedited):

function editData($table_name,$fldname,$fldval,$other='',$lang_flag=0)
{

    $link = $this->my_connect();
    $this->my_select_db($this->DATABASE_NAME,$link);
$arr_types =  array("TR_", "TN_", "TREF_", "PHR_", "PHN_", "IR_", "IN_", "MR_", "MN_", "TNEF_", "TRC_", "TNC_", "TRFN_", "TNFN_","TNURL_","TRURL_");
    if (!empty($GLOBALS["HTTP_POST_VARS"])) {
        reset($GLOBALS["HTTP_POST_VARS"]);
        while (list($k,$v)=each($GLOBALS["HTTP_POST_VARS"]))
        {

            for($p=0;$p<count($arr_types);$p++)
            {
                if(stristr($k,$arr_types[$p])!="")
                {
                    $k = str_replace($arr_types[$p],"",$k);
                }
            }
            ${strtolower($k)}=$v;
            //echo "
 k =$k -- v = $v";
        }
    }
    if (!empty($_POST)) {
        reset($_POST);
        while (list($k,$v)=each($_POST))
        {

            for($p=0;$p < count($arr_types);$p++)
            {
                if(stristr($k,$arr_types[$p])!="")
                {
                    $k = str_replace($arr_types[$p],"",$k);
                }
            }
            ${strtolower($k)}=$v;
            //echo "
 k =$k -- v = $v";
        }
    }
    if (!empty($GLOBALS["HTTP_GET_VARS"])) {
        reset($GLOBALS["HTTP_GET_VARS"]);
        while (list($k,$v)=each($GLOBALS["HTTP_GET_VARS"]))
        {

            for($p=0;$p < count($arr_types);$p++)
            {
                if(stristr($k,$arr_types[$p])!="")
                {
                    $k = str_replace($arr_types[$p],"",$k);
                }
            }
            ${strtolower($k)}=$v;
            //echo "
 k =$k -- v = $v";
        }
    }
    if (!empty($_GET)) {
        reset($_GET);
        while (list($k,$v)=each($_GET))
        {

            for($p=0;$p < count($arr_types);$p++)
            {
                if(stristr($k,$arr_types[$p])!="")
                {
                    $k = str_replace($arr_types[$p],"",$k);
                }
            }
            ${strtolower($k)}=$v;
            //echo "
 k =$k -- v = $v";
        }
    }
     $result=$this->my_query("show fields from $table_name",$link);
    $query="update $table_name SET   ";
     while ($a_row = $this->my_fetch_array($result)) {
        $field="$a_row[Field]";

            if($field!=$fldname)
            {
                if(isset($$field))
                {
                       $query.="`".$field."`=";

                       $query.="'".$this->removeQuotes($$field,$lang_flag)."' , ";
                }
                else
                {
                    if(isset($GLOBALS["$field"]))
                    {
                        //echo "
 var ".$GLOBALS["$field"];
                        $query.="`".$field."`=";
                        $query.="'".$this->removeQuotes($GLOBALS["$field"],$lang_flag)."' , ";

                    }
                }
            }
     }
     $query = substr($query,0,-2);
    $query.=" where `$fldname`='$fldval' $other";
    $query  ;
    //echo $query;
     $result=$this->my_query($query,$link);
     $this->my_free_result($result);
     $this->my_close($link);
     return $result;
}

Simply put, this code fails a lot best practices also contains massive security issues.
Suggestion to improve for best practices:

  $GLOBALS["HTTP_POST_VARS"] is deprecated in favor of $_POST

So you looping over $GLOBALS["HTTP_POST_VARS"] and $_POST is redundant and bad mojo. Just looping over $_POST will accomplish the same thing. Same goes for $GLOBALS["HTTP_GET_VARS"] and $_GET.

  You should be passing in the $link object as parameter.

Since there is no code intended to change the database connection conditionally (the connect parameters are always the same). This prevents your scripts creating multiple database connection resources and destroying them (more memory and time consuming). Also passing in the database resource as parameter to the function is a type of dependency injection which makes your code testable should you want to in the future. Since this also appears to be in a class, consider creating the database connection in the __construct and use that single instance.

  Using sessions in favor of $GLOBALS

Pretty much any use of $GLOBALS is considered bad practice. I'm sure there are a few exceptions to the rule. But there you should definitely not be using is for storing data in a scopeless fashion. This is what sessions are for.

  removeQuotes() is never called on $fldname or $fldval

As you can imagine this a serious security issue leaving your script open to SQL Injection attacks.

  Use mysql_real_escape_string in favor of home made sanitising functions i.e. removeQuotes()

There is a lot more to safe escaping query parameters that remove slashes. And in my experience home made sanitising functions often quietly fail when you have mixed string and number parameters.

As a matter of readability and style I would make to following suggestions, I know these are subject matters but there are common ways of doing things.

  Substitute the use of string concatenation

$query.="`".$field."`=";
$query.="'".$this->removeQuotes($$field,$lang_flag)."' , ";
... more concatenation ...
$query = substr($query,0,-2);

for array joining

$queryParams[] = "`".$field."`='".$this->removeQuotes($$field,$lang_flag)."'";
... more array filling ...
$query = join(',', $queryParams);

General WTFs

  This is doing nothing at all $query  ;

And code that does nothing is very bad mojo.

  Call count() once.

This is not really an issue, doing something like this

for($p=0;$p

In actual fact calls count() on every iteration of the loop, even though the return value will never change. It causes the interpreter to do more work than necessary, although I suspect during compilation to byte code this would be optimised automatically. This could be substituted for:

// only call count() once
$count = count($arr_types);
for($p=0;$p<$count;$p++)

  Don't nest unnecessarily.

Don't nest conditionals (if/else/switch) and loop(while/for) unnecessarily. It makes for messier code, code that's harder to read and follow mentally, and importantly difficult to test.

if(isset($$field))
{
     $query.="`".$field."`=";

     $query.="'".$this->removeQuotes($$field,$lang_flag)."' , ";
 }
 else
 {
     if(isset($GLOBALS["$field"]))
     {
         $query.="`".$field."`=";
         $query.="'".$this->removeQuotes($GLOBALS["$field"],$lang_flag)."' , ";
     }
 }

The above code is exactly equal to the following, notice less nesting.

if(isset($$field))
{
     $query.="`".$field."`=";

     $query.="'".$this->removeQuotes($$field,$lang_flag)."' , ";
 }
 else if(isset($GLOBALS["$field"]))
 {
         $query.="`".$field."`=";
         $query.="'".$this->removeQuotes($GLOBALS["$field"],$lang_flag)."' , ";
 }

Personal preference

  This is completely subjective, but something to think about.

if(stristr($k,$arr_types[$p])!="")

equal to

if(stristr($k,$arr_types[$p]) == true)

equal to

if(stristr($k,$arr_types[$p]))

  Considering the above, this:

while (list($k,$v)=each($_GET))
{
    for($p=0;$p < count($arr_types);$p++)
    {
        if(stristr($k,$arr_types[$p])!="")
        {
            $k = str_replace($arr_types[$p],"",$k);
        }
    }
    ${strtolower($k)}=$v;
}

Is exactly the same as:

while (list($k,$v)=each($_GET))
{
    $count = count($arr_types);
    for($p=0; $p<$count; $p++);
    {
        $k = str_replace($arr_types[$p],"",$k);
    }
    ${strtolower($k)}=$v;
}

Which would you rather test and/or work with?


Implementation of merge sort and bubble sort I'm new to Go and looking for a review of my merge sort and bubble sort implementations. What can be done better? Can my code be cleaner/clearer?

// algos.go
// (C) 2014 splashinn
// Implement two types of sorting algorithms; merge sort and bubble sort

package main

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

func main() {

    var n int
    fmt.Printf("How long of an array should we sort? (≤1000) ")
    _, err := fmt.Scanf("%d", &n)
    if err != nil {
        fmt.Println("Error:", err)
        return
    }
    if n > 1000 {
        fmt.Println("Error:", n, "> 1000 - too long!")
        return
    } else if n < 1 {
        fmt.Println("Error:", n, "< 1")
        return
    }

    array := make([]int, n)
    rand.Seed(time.Now().UTC().UnixNano())

    fmt.Println("Original Array:")
    for i := range array {
        array[i] = rand.Intn(10000)
        fmt.Printf("%d\t", array[i])
    }
    fmt.Println()

    bubbleSortedArray := BubbleSort(array)

    fmt.Println("Bubble Sorted Array: ")
    for i := range bubbleSortedArray {
        fmt.Printf("%d\t", bubbleSortedArray[i])
    }
    fmt.Println()

    mergeSortedArray := MergeSort(array)

    fmt.Println("Merge Sorted Array: ")
    for i := range mergeSortedArray {
        fmt.Printf("%d\t", mergeSortedArray[i])
    }
    fmt.Println()
}

// Implementing bubble sort
func BubbleSort(inputArray []int) (sortedArray []int) {
    array := make([]int, len(inputArray))
    copy(array, inputArray)
    for i := 0; i < len(array)-1; i++ {
        for j := 0; j < len(array)-1; j++ {
            if array[j] > array[j+1] {
                // Idiomatic Go swap
                array[j], array[j+1] = array[j+1], array[j]
            }
        }
    }
    return array
}

// Implementing a top-down merge sort
// Adapted from the Merge Sort C code from Wikipedia
func MergeSort(array []int) (sortedArray []int) {
    sortedArray = make([]int, len(array), cap(array))
    splitMerge(array, sortedArray)
    return array
}

func splitMerge(A []int, B []int) {
    if(len(A) < 2) {   // if run size == 1, consider it sorted
        return
    }
    // recursively split runs into two halves until run size == 1,
    // then merge them and return back up the call chain
    iMiddle := len(A) / 2              // iMiddle = mid point
    splitMerge(A[:iMiddle], B[:iMiddle])  // split / merge left  half
    splitMerge(A[iMiddle:], B[iMiddle:])  // split / merge right half
    merge(A, B, iMiddle)  // merge the two half runs
    copy(A, B)             // copy the merged runs back to A
}

func merge(A []int, B []int, iMiddle int) {
    i0 := 0
    i1 := len(A) / 2
    iEnd := len(A)

    for j := range A {
        // If left run head exists and is <= existing right run head.
        if (i0 < iMiddle && (i1 >= iEnd || A[i0] <= A[i1])) {
            B[j] = A[i0]
            i0++  // Increment i0 after using it as an index.
        } else {
            B[j] = A[i1]
            i1++  // Increment i1 after using it as an index.
        }
    }
}

I don't see any major problems but there are a number of (really very minor) little nits.

First, the trend seems to be for people to like very few blank lines.  I think this trend is driven by the prevalence of syntax highlighting.  I recently I had the occasion to program on a machine without syntax highlighting and found that I wanted more blank lines again for readability.  Anecdotes aside though, try using fewer blank lines and see if you like it after you get used to it.

In your first if statement, you could use the "simple statement" form

if _, err := fmt.Scanf("%d", &n); err != nil {
    fmt.Println("Error:", err)
    return
}

Most Go programmers like that this form more clearly associates the if statement with the code that is being tested and that the scope of err here is limited to the if body.  It also saves a line of screen real estate.

Consider replacing sequences of if statements with a single switch statement.

switch _, err := fmt.Scanf("%d", &n); {
case err != nil:
    fmt.Println("Error:", err)
    return
case n > 1000:
    fmt.Println("Error:", n, "> 1000 - too long!")
    return
case n < 1:
    fmt.Println("Error:", n, "< 1")
    return
}

The usual arguments for preferring switch over a chain of ifs are that the logic is easier to follow and the code is more readable.  As with the simple statement form of if, this form also restricts variable scope and tends to need few lines on the screen.

array := make([]int, n) presents some issues.

The word "array" has a general meaning in computing, but has a specific meaning in Go.  Some Go programmers are bothered when Go slices are incorrectly called arrays.  It's disturbing in the way something like var integer float64 would be.  You might still argue that array is appropriate here because it identifies the conceptual thing you have obtained from the user and retains the language of the user interface.  Or in the interest of code readability, you might consider other names, or consider different terminology in the user interface.

In BubbleSort, you can replace

array := make([]int, len(inputArray))
copy(array, inputArray)

with

array := append([]int{}, inputArray...)

if you like.  The first form is easy to understand and is arguably more readable.  The second form is a little tricky but is listed on Slice Tricks on the Go Wiki and I think is pretty common.

It looks like your for loops in BubbleSort could use the range clause form.

And then you have some if statements with unnecessary parentheses.  Otherwise looks great.


Updating multiple svn repositories using powershell I wrote a script to perform svn update on all the SVN repositories inside a folder. It would have been great if the folder containing all these repositories itself was a repository, but it was not setup like that, which is why I opted to write this script.

$timestamp = get-date -f "yyyy-M-d-h-m-s"
$SvnLogPath="C:\out\svnlogs\"

if(test-path -path $SvnLogPath)
{
    write-host "SVN Log path doesn't exist. So I am creating it . . ."
    new-item -type directory $SvnLogPath | out-null
}

$logfile = "${SvnLogPath}svn_update_${timestamp}.log"
new-item -type file $logfile | out-null

$BaseDir="C:\TRUNK\"

$RepositoryList = "Alpha","Bravo","Charlie","Delta","Echo","Foxtrot","Golf","Hotel"

# BUILDING FULL PATHS
$FullPaths = $RepositoryList | %{ $BaseDir+$_}

# ISSUING SVN UPDATE TO EACH DIR
$FullPaths | % {svn update $_ 2>&1 | ft -AutoSize -Wrap | Out-File -Append $logfile }

# OPEN LOG FILE AFTER THE COMPLETION
notepad $logfile

Can this script be further simplified and written more idiomatically?

PS: I am using only svn update without credentials as they are stored in the shell already.

Reusable Commands

One of the most important ideas in PowerShell is the Get/Update/Set pattern. A clear delineation between resources (nouns) and commands (verbs) helps to write code that is useful not only for scripting, but also in the interactive shell.

We only need a couple of new commands. First we need to Find the working copies under some directory, then Update each working copy. The list of approved verbs for PowerShell is a great resource for help with naming your commands. Let's use these names.

Find-SvnWorkingCopy (Find looks for an object in a container)
Update-SvnWorkingCopy (Update brings a resource up-to-date)

Functions and Modules

Now you're convinced that we should create reusable commands with these great new names. What next? PowerShell v2 introduced the concept of a module, which is just a file that contains functions. (Of course, you could just create a new script for each command but it helps to keep things tidy by bundling all of the functions together in one file.)

What should our functions look like? Yes they will need to take some parameters. But more importantly, they should return objects to the pipeline, even if they aren't a "getter" function! This makes it easier to review what impact a function had.

Some actual code

Luckily for us, somebody else already found a way to find all the svn working copies underneath a directory. We can reuse that.

SvnTools.psm1

function Find-SvnWorkingCopy
{
    Param (
        [Parameter(ValueFromPipeline=$true,Mandatory=$true)]
        $BasePath
    )
    process
    {
        $prev = "^$"
        Get-ChildItem -Recurse -Force -Include ".svn" -Path $BasePath |
            Where-Object {
                $_.PSIsContainer -and $_.Fullname.StartsWith($prev)-eq $false
            } |
            ForEach-Object {
                $prev=$_.Fullname.TrimEnd(".svn")
                $prev
            }
    }
}

I took some liberties with the command Update-SvnWorkingCopy. In addition to calling "svn update" like your original script, it also calls "svn info" and parses the results to find the current revision number and include that in the object returned to the pipeline.

SvnTools.psm1 (continued)

function Update-SvnWorkingCopy
{
    Param (
        [Parameter(ValueFromPipeline=$true,Mandatory=$true)]
        $WorkingCopy
    )
    process {
        svn update $WorkingCopy > $null
        [String[]] $result = svn info $WorkingCopy
        [String] $revStr = $result -like "Revision:*"
        [int] $revNumber = $revStr.Replace("Revision: ", "")
        New-Object -TypeName PSObject -Prop @{
            WorkingCopy=$WorkingCopy;
            Revision=$revNumber;
        } | Write-Output
    }
}

Now we can reap the benefits from creating these commands. Here's your original script, rewritten to take advantage of our new module using just one line.

UpdateMyWorkingCopies.ps1

Import-Module C:\path\to\module\SvnTools.psm1
Find-SvnWorkingCopy -BasePath "C:\TRUNK\" | Update-SvnWorkingCopy

You could easily redirect the output to a file. If you don't, it will show up on screen looking something like this.

WorkingCopy         Revision
-----------         --------
C:\TRUNK\Alpha      31
C:\TRUNK\Bravo      42
C:\TRUNK\Charlie    16

References

The Scripting Guy! blog on TechNet has a really excellent series about building your own PowerShell cmdlet that I recommend reading.

Coding Style

https://stackoverflow.com/a/2031927/190298
https://stackoverflow.com/questions/5260125/whats-the-better-cleaner-way-to-ignore-output-in-powershell
http://www.powershellcommunity.org/Wikis/BestPractices/tabid/79/topic/Naming/Default.aspx

Bonus Points

For bonus points, install the module into one of the module path folders ($env:PSModulePath) so that you can import it from the shell more easily.

And if you want to get even more fancy, implement another command called Get-SvnWorkingCopy which parses all of the results from svn info. It should integrate nicely with the two commands we already created here.


Code to take a list, convert to string and insert comma between the 'elements' - From Automate The Boring Stuff I have been working on this question in particular. I'm a bit of a python beginner myself, having re-learnt functions just a few days back.

Write a function that takes a list value as an argument and returns a string with all the items separated by a comma and a space, with and inserted before the last item. For example, passing the previous spam list to the function would return 'apples, bananas, tofu, and cats'. But your function should be able to work with any list value passed to it. Be sure to test the case where an empty list [] is passed to your function.

My idea for this was to have my function run a for loop on the range of the number of elements of the list, add the elements of the list to a comma. If the element of the list happens to be the last element, 'and' and that specific element are added together.
Here is the code:
def listsmash(listu):
    y = ''
    for x in range(len(listu)):
        if x == len(listu) -1 :
            y+= 'and '+ str(listu[x])
        else:
            y+= str(listu[x]) + ','
    return y
print(listsmash([1,345,34656,456456,456454564]))

Is this an efficient piece of code? Any way for me to make it more efficient than it already is?

Nice solution, these are my suggestions:

Indentation: the first line is not indented correctly, there is an extra tab or spaces before def listsmash(listu):.
Code formatting: follow PEP 8 to format the code. You can use pep8online or just use the auto-formatting feature of many IDEs.
Bug: the output should be apples, bananas, tofu, and cats but the code outputs apples,bananas,tofu,and cats. There should be a space after the comma.
Edge case: if the input list contains only apples the code outputs and apples. This is an edge case to handle, probably at the beginning of the method.
Performance: there is no much to worry about performances, as it runs linearly to the input size. However, the first if condition is executed everytime:
for x in range(len(listu)):
    if x == len(listu) -1 :
        y+= 'and '+ str(listu[x])
    else:
        y+= str(listu[x]) + ','

Limit the number of iterations to len(listy) - 1 and add the last element outside the for-loop:
def listsmash(listu):
    y = ''
    for x in range(len(listu) - 1):
        y += str(listu[x]) + ', '
    y += 'and ' + str(listu[-1])
    return y

Use str.join: str.join is a handy function in this case:
def listsmash(listu):
    y = ', '.join(str(value) for value in listu[:-1])
    return y + ', and ' + str(listu[-1])

It joins all elements of listu (except the last one) with ,  and then we can append the last element at the end. Additionally, the function .join() is faster than +=.
Naming: I am not sure what listu means, maybe you can consider a better name like items following from the problem description.

Runtime for 1 million items:
Original: 4.321 s
Improved: 0.418 s

Full code:
from random import randint
from time import perf_counter

def original(listu):
    y = ''
    for x in range(len(listu)):
        if x == len(listu) - 1:
            y += 'and ' + str(listu[x])
        else:
            y += str(listu[x]) + ', '
    return y

def improved(items):
    result = ', '.join(str(item) for item in items[:-1])
    return result + ', and ' + str(items[-1])

# Correctness
items = ['apples', 'bananas', 'tofu']
assert original(items) == improved(items)

# Benchmark
n = 1_000_000
items = [randint(0, n) for _ in range(n)]

t0 = perf_counter()
original(items)
total_time = perf_counter() - t0
print(f'Original: {round(total_time, 3)} s')

t0 = perf_counter()
improved(items)
total_time = perf_counter() - t0
print(f'Improved: {round(total_time, 3)} s')


Manage the sale of hamburgers Can I improve my code? There are 3 types of hamburgers.

The basic hamburger will have these properties:

prices
type of bread
type of meat
maximum 4 supplements

Then, create a  Bio burger  that will be gray bread, and which can receive in addition 2 elements supplement to the basic either 6 supplements max.

After, create a Deluxe Hamburger : that will be provided with fries and a drink, but it will not be possible to add supplements.

Class Hamburger

public class Hamburger {

  // Attributes

  double price;
  private String typeBread;
  private String typeMeat;
  int counter;

  public Hamburger(double price, String typeBread, String typeMeat)
  {
    this.price = price;
    this.typeBread = typeBread;
    this.typeMeat = typeMeat;
    this.counter = 0;

    System.out.println("Hamburger " + this.getTypeBread() + " - " + this.typeMeat + " - " + this.getPrice() + " euros");
  }

  // add supplement 4 maximum

  public void addSupplement(String type, double amount)
  {
    counter++;
    if(counter <= 4)
    {
      System.out.println("Supplement " + counter + " : " + type + " - " + amount + " euros");
      this.price = this.price + amount;
    }
    else
    {
      System.out.println("Error supplement maximum is 4 ");
    }
  }

  public void total()
  {
    System.out.println("Total => " + this.getPrice() + " euros");
  }

  // les getters

  public double getPrice()
  {
    return price;
  }

  public String getTypeBread()
  {
    return typeBread;
  }

  public String getTypeMeat()
  {
    return typeMeat;
  }

  public int getCounter()
  {
    return counter;
  }

}

Class Bio

public class Bio extends Hamburger  {

  public Bio(double price, String typeMeat) {
        super(price, "gray bread", typeMeat);

    }

    public void addSupplement(String type, double amount) {
        counter++;
        if (counter<=6){
            System.out.println("Supplement " + counter + " : " + type + " - " + amount + " euros ");
            this.price = this.price+ amount;
        }
        else{
            System.out.println("Error supplement 6 maximum ");
        }
    }

}

Class Deluxe

public class Deluxe extends Hamburger {

   private String accompaniment;
   private String drink;

   // Builders

     public Deluxe () {
         super (7.99, "white bread", "veal");
         this.accompaniment = "fried";
         this.drink = "drink";

         System.out.println ("Included in the DELUXE formula:" + this.getDrink () + "-" + this.getAccompaniment ());
     }

     public String getAccompaniment () {
         return accompaniment;
     }

     public String getDrink () {
         return drink;
     }

     public void addSupplement (String type, double price) {
         System.out.println ("No supplement allowed for this formula");
     }

     public void total () {
         super.total ();
     }
}

Class Main

public class Main {

    public static void main(String[] args) {

    System.out.println("BASIC");
    Hamburger myHamburger = new Hamburger(5.99, "white bread", "beef");
    myHamburger.addSupplement("Tomatos", 0.99);
    myHamburger.addSupplement("Bacons", 0.59);
    myHamburger.addSupplement("Onions", 0.49);
    myHamburger.addSupplement("Ketchup", 0.29);
    System.out.println("__________");
    myHamburger.total();
    System.out.println("-----------------------");

    System.out.println("BIO");
    Bio myBio = new Bio (6.99, "Veal");
    myBio.addSupplement ("Onions", 0.49);
    myBio.addSupplement("Tomatoes", 0.99);
    myBio.addSupplement ("Bacon", 0.59);
    myBio.addSupplement ("Cucumbers", 0.49);
    myBio.addSupplement("Ketchup", 0.29);
    myBio.addSupplement ("Mayo", 0.29);
    myBio.addSupplement ("Cocktail", 0.29);
    System.out.println ( "__________");
    myBio.total ();
    System.out.println("-----------------------");

    System.out.println("DELUXE");
    Deluxe myDeluxe = new Deluxe();
    myDeluxe.addSupplement("Tomatoes", 0.99);
    System.out.println("__________");
    myDeluxe.total();

}
}

Things that I would like to point out in your code:

Use of Strings

I see that you used Strings in defining the names of the parts for your burger like bread, and meat types. I suggest you use enums so as to regulate the possible types of bread, meat, and others that can be added to a meal, like so:

BreadType Enum

public enum BreadType {
    WHITE_BREAD("white bread");

    private String type;

    BreadType(String type) {
        this.type = type;
    }

    @Override
    public String toString() {
        return type;
    }
}

MeatType Enum

public enum MeatType {
    BEEF("beef"), VEAL("veal");

    private final String type;

    MeatType(String type) {
        this.type = type;
    }

    @Override
    public String toString() {
        return type;
    }
}

You can also use enums to store other properties too. This can be done for items like Supplements which have prices attached to them, drinks, and sides like so:

Supplement Enum

public enum Supplement implements FoodItem {
    TOMATOES("tomato", 0.99), BACON("bacon", 0.59), ONIONS("onion", 0.49),
    KETCHUP("ketchup", 0.29), COCKTAIL("cocktail", 0.29), CUCUMBERS("cucumbers", 0.29);

    private BigDecimal price;
    private String type;

    Supplement(String type, double price) {
        this.type = type;
        this.price = new BigDecimal(price).setScale(2, BigDecimal.ROUND_HALF_DOWN);
    }

    @Override
    public String toString() {
        return type;
    }

    public BigDecimal getPrice() {
        return price;
    }
}

Drink Enum

public enum Drink implements FoodItem{
    APPLE_JUICE("apple juice", 3.49);

    private final BigDecimal price;
    private final String name;

    Drink(String name, double price) {
        this.name = name;
        this.price = new BigDecimal(price).setScale(2, BigDecimal.ROUND_HALF_DOWN);
    }

    public BigDecimal getPrice() {
        return price;
    }

    @Override
    public String toString() {
        return name;
    }
}

Side Enum

public enum Side implements FoodItem {
    FRIES("fries", 3.99);

    private final BigDecimal price;
    private final String name;

    Side(String name, double price) {
        this.name = name;
        this.price = new BigDecimal(price).setScale(2, BigDecimal.ROUND_HALF_DOWN);
    }

    public BigDecimal getPrice() {
        return price;
    }

    @Override
    public String toString() {
        return name;
    }
}

You may have noticed the use of BigDecimals in prices part instead of double. The reason for this is that BigDecimal gives us a way to exactly represent numbers. More on this reason over here.

The Hamburger Meals

I take it you want to create different hamburger meals -- each meal having different items compared to that of the other meals. Here's what I did:

FoodItem interface

public interface FoodItem {
    BigDecimal getPrice();
}

You mentioned four "food items" in the problem: Hamburger, Fries, Supplement, and Drink. Assuming that each "food item" has a price, these classes should implement FoodItem. The reason for this is to ease the computation of the price in a hamburger meal. Speaking of hamburger meal, I also created a HamburgerMeal class:

HamburgerMeal class

public class HamburgerMeal {
    private final List<FoodItem> foodItems;
    private final List<Supplement> supplements;
    private final HamburgerMealType hamburgerMealType;

    public HamburgerMeal(HamburgerMealType hamburgerMealType) {
        this.foodItems = new ArrayList<FoodItem>();
        this.supplements = new ArrayList<Supplement>();
        this.hamburgerMealType = hamburgerMealType;
    }

    public BigDecimal getPrice(){
        BigDecimal resultingPrice = BigDecimal.ZERO;
        for(FoodItem foodItem : foodItems){
            resultingPrice = resultingPrice.add(foodItem.getPrice());
        }
        for(Supplement supplement : supplements){
            resultingPrice = resultingPrice.add(supplement.getPrice());
        }
        return resultingPrice;
    }

    public void addItem(FoodItem foodItem){
        foodItems.add(foodItem);
    }

    public void addSupplement(Supplement supplement) {
        checkIfAddingSupplementsIsStillAllowed();
        supplements.add(supplement);
    }

    private void checkIfAddingSupplementsIsStillAllowed() {
        if(supplements.size() == hamburgerMealType.getMaxNumberOfSupplements())
            throw new AdditionOfItemNotAllowedException("No more supplements can be added to " + hamburgerMealType);
    }

    @Override
    public String toString() {
        return hamburgerMealType + " burger meal";
    }
}

You may have noticed the variable HamburgerMealType having a property called maxNumberOfSupplements. That's because, it's also an Enum!

HamburgerMealType Enum

public enum HamburgerMealType {
    BASIC("basic", 4), BIO("bio", 6), DELUXE("deluxe", 0);

    private final String name;
    private final int maxNumberOfSupplements;

    HamburgerMealType(String name, int maxNumberOfSupplements) {
        this.name = name;
        this.maxNumberOfSupplements = maxNumberOfSupplements;
    }

    @Override
    public String toString() {
        return name;
    }

    public int getMaxNumberOfSupplements() {
        return maxNumberOfSupplements;
    }
}

This helps us keep track the main property of a MealType which is the maximum number of Supplements it can have.

You may have also noticed the use of AdditionOfItemNotAllowedException. Whenever you think something "illegal" can happen (which is adding a supplement into a meal that can't have supplements), it is best to catch them with Exceptions instead of just saying that it's "illegal". Here's the code for the said exception:

public class AdditionOfItemNotAllowedException extends IllegalArgumentException {
    public AdditionOfItemNotAllowedException(String message) {
        super(message);
    }
}

Lastly, we have the Hamburger class.

Hamburger class

public class Hamburger implements FoodItem {
    private final BigDecimal price;
    private final BreadType breadType;
    private final MeatType meatType;

    private Hamburger(Builder builder){
        this.price = builder.price;
        this.breadType = builder.breadType;
        this.meatType = builder.meatType;
    }

    public static class Builder{
        private final BigDecimal price;

        private BreadType breadType = BreadType.WHITE_BREAD;
        private MeatType meatType = MeatType.BEEF;

        public Builder(double price){
            this.price = new BigDecimal(price).setScale(2, BigDecimal.ROUND_HALF_DOWN);
        }

        public Builder breadType(BreadType breadType){
            this.breadType = breadType;
            return this;
        }

        public Builder meatType(MeatType meatType){
            this.meatType = meatType;
            return this;
        }

        public Hamburger build(){
            return new Hamburger(this);
        }
    }

    public BigDecimal getPrice(){
        return price;
    }

    public BreadType getBreadType() {
        return breadType;
    }

    public MeatType getMeatType() {
        return meatType;
    }
}

I used the static builder pattern for this one since the Hamburger specs you mentioned have defaults for BreadType and MeatType. This is also to avoid passing three parameters into the constructor which may get confusing when you don't know which parameter comes first.

I end this answer with the main code I wrote:

Main class

public class Main {
    public static void main(String[] args){
        HamburgerMeal standardBurgerMeal = new HamburgerMeal(HamburgerMealType.BASIC);
        Hamburger hamburger = new Hamburger.Builder(5.99).build();
        standardBurgerMeal.addItem(hamburger);
        standardBurgerMeal.addSupplement(Supplement.TOMATOES);
        standardBurgerMeal.addSupplement(Supplement.BACON);
        standardBurgerMeal.addSupplement(Supplement.ONIONS);
        standardBurgerMeal.addSupplement(Supplement.KETCHUP);
        System.out.println(standardBurgerMeal + " price: " + standardBurgerMeal.getPrice());

        HamburgerMeal bioBurgerMeal = new HamburgerMeal(HamburgerMealType.BIO);
        Hamburger bioBurger = new Hamburger.Builder(6.99).meatType(MeatType.VEAL).build();
        bioBurgerMeal.addItem(bioBurger);
        bioBurgerMeal.addSupplement(Supplement.TOMATOES);
        bioBurgerMeal.addSupplement(Supplement.BACON);
        bioBurgerMeal.addSupplement(Supplement.ONIONS);
        bioBurgerMeal.addSupplement(Supplement.KETCHUP);
        bioBurgerMeal.addSupplement(Supplement.COCKTAIL);
        bioBurgerMeal.addSupplement(Supplement.CUCUMBERS);
        System.out.println(bioBurgerMeal + " price: " + bioBurgerMeal.getPrice());

        HamburgerMeal deluxeBurgerMeal = new HamburgerMeal(HamburgerMealType.DELUXE);
        Hamburger deluxeBurger = new Hamburger.Builder(7.99).build();
        deluxeBurgerMeal.addItem(deluxeBurger);
        deluxeBurgerMeal.addItem(Drink.APPLE_JUICE);
        deluxeBurgerMeal.addItem(Side.FRIES);
        System.out.println(deluxeBurgerMeal + " price: " + deluxeBurgerMeal.getPrice());
    }
}

I apologize if my explanation is confusing. This is pretty much the first long answer I posted ever lol.


Retrieve value from item wrapped in a Future[Option[Item]] I have a function like this

def findById(id: UUID): Future[Option[Item]]

case class Item(subject: String)

Now I want to retrieve the subject if the item exists else I want the subject to be empty string.

val subject: Future[String] = repository
  .findById(someId)
  .map(_.map(i => i.subject))
  .map(_.getOrElse(""))

subject.foreach(doSomething)

Am I doing it right or is there a better way in Scala without frameworks like Scalaz.

It's better that you are staying in the Futures domain, no longer trying to unwrap to String.

It's not so easy to follow all of the underscore-based function expressions. I'd try something like...

val subject: Future[String] = {
  repository.findById(someId) map { mbItem =>
    mbItem match {
      case Some( item ) => item.subject
      case None         => ""
    }
  }
}


Towny chat formatter How can this be improved,to make it easier to maintain? There is so much boilerplate being thrown around, so is it possible to use annotations? I've seen one example from Stack Overflow, but it ended up using reflection in order to call the method.

I was already planning on somehow annotating these "variables" in order to automatically produce the documentation, to prevent the documentation going out of date.

GitHub

public class TownyChatFormatter {
private static StringReplaceManager<LocalTownyChatEvent> replacer = new StringReplaceManager<LocalTownyChatEvent>();

static {
    replacer.registerFormatReplacement(Pattern.quote("{worldname}"), new TownyChatReplacerCallable() {
        @Override
        public String call(String match, LocalTownyChatEvent event) throws Exception {
            return String.format(ChatSettings.getWorldTag(), event.getEvent().getPlayer().getWorld().getName());
        }
    });
    replacer.registerFormatReplacement(Pattern.quote("{town}"), new TownyChatReplacerCallable() {
        @Override
        public String call(String match, LocalTownyChatEvent event) throws Exception {
            return event.getResident().hasTown() ? event.getResident().getTown().getName() : "";
        }
    });

If you don't mind using a String switch, and can't use Java 8 yet (lambda expressions should greatly simplify your existing code), you could use something like the following (which replaces the use of inner classes with a single switch):

static {
    register("{worldname}", "{town}");
}

private static void register(final String... patterns) {
    for (final String pattern : patterns) {
        replacer.registerFormatReplacement(Pattern.quote(pattern), new TownyChatReplacerCallable() {
            @Override
            public String call(String match, LocalTownyChatEvent event) throws Exception {
                switch (pattern) {
                    case "{worldname}": return String.format(ChatSettings.getWorldTag(), event.getEvent().getPlayer().getWorld().getName());
                    case "{town}": return event.getResident().hasTown() ? event.getResident().getTown().getName() : "";
                    default: throw new IllegalArgumentException();
                }
            }
        });
    }
}

You would have to profile this to see whether the use of a switch made performance worse or better though (could be better, as the single registered function might be inlined and the switch should be fast).  You could also use constants variables instead of strings, but that would increase code size.