Int to Enum Extension Method As you know, converting an int to an enum is straightforward. Doing it generically with type/range checking isn't. I looked for a reusable example since this seems like a common situation but did not find a suitable one. If it exists, please let me know. Below is a C# extension method that encapsulates this behavior. Any feedback is welcome.

public static T ToEnum<T>(this int value)
{
    Type type = typeof(T);

    if (!type.IsEnum)
    {
        throw new ArgumentException($"{type} is not an enum.");
    }

    if (!type.IsEnumDefined(value))
    {
        throw new ArgumentException($"{value} is not a valid ordinal of type {type}.");
    }

    return (T)Enum.ToObject(type, value);
}

As Jason Tyler said in a comment, this has a problem that [Flags] enumerations won't be properly handled.

I ran this in C# interactive, and it threw an exception (as expected, but not desired) for a valid [Flags] value:

> public static T ToEnum<T>(this int value)
. {
.     Type type = typeof(T);
.
.     if (!type.IsEnum)
.     {
.         throw new ArgumentException($"{type} is not an enum.");
.
.     }
.
.     if (!type.IsEnumDefined(value))
.     {
.         throw new ArgumentException($"{value} is not a valid ordinal of type {type}.");
.     }
.
.     return (T)Enum.ToObject(type, value);
. }
> [Flags] public enum Test { Flag1 = 0x01, Flag2 = 0x02 }
> ToEnum<Test>(3);
3 is not a valid ordinal of type Submission#1+Test.
  + Submission#0.ToEnum<T>(int)

So, we might have to look at a different way of going about this.

There's an answer on Stack Overflow, but I don't like it. They use string comparison and validation, and it just feels like a hack.

So what can we do?

Well, we have Enum.GetValues, I bet we can come up with a solution there, assuming we interpret all the values as a numeric type.

The hard part is picking the type. Enums can be any numeric type, so we have to decide on one.

Fun fact: every numeric type can be cast to the others in C#, there just might be overflow issues. So if we look at all the numeric types, we can find that we should always be able to cast them to a long or ulong, depending on whether they're signed or not.

OK, so how do we figure out what number type it is?

There's another magic function in Enum: Enum.GetUnderlyingType, I think that'll do.

Of course, this also means we can no longer take an int input, because we want to support any number. We could take in an object, but then we have to deal with boxing/unboxing, and that can get prohibitively expensive with frequent use, but we'll work that out-of-the-way later.

We can use some bit-fiddling to find out if a value is only composed of valid Enum values. Basically, we'll do the following:

Loop through the valid flags options:

If providedValue ∧ enumValue > 0, then providedValue = providedValue ⊕ enumValue

If providedValue > 0 then it is not composed strictly of the valid enum options

For those unfamiliar with boolean algebra:

∧ = "AND", take all bits of both values and compare them such that the result value only contains those that are 1 in each value, this is & in C#;
⊕ = "XOR", take all bits of both values and compare them such that the result value only contains those that are 1 in one value, and 0 in another, this is ^ in C#;

So, if we rewrite this, it gets a bit ugly, but we'll clean some of that up:

public static TEnum ToEnum<TEnum>(this object value)
{
    Type type = typeof(TEnum);

    if (!type.IsEnum)
    {
        throw new ArgumentException($"{type} is not an enum.");
    }

    if (type.GetCustomAttributes(typeof(FlagsAttribute), true).Length > 0)
    {
        var values = Enum.GetValues(type);
        switch (Enum.GetUnderlyingType(type).FullName)
        {
            case "System.Byte":
                {
                    var myVal = (byte)value;
                    foreach (byte val in values)
                    {
                        if ((myVal & val) > (byte)0)
                        {
                            myVal ^= val;
                        }
                    }
                    if (myVal > (byte)0)
                    {
                        throw new ArgumentException($"{value} is not a valid ordinal of type {type}.");
                    }
                }
                break;
            case "System.SByte":
                {
                    var myVal = (sbyte)value;
                    foreach (sbyte val in values)
                    {
                        if ((myVal & val) > (sbyte)0)
                        {
                            myVal ^= val;
                        }
                    }
                    if (myVal > (sbyte)0)
                    {
                        throw new ArgumentException($"{value} is not a valid ordinal of type {type}.");
                    }
                }
                break;
            case "System.UInt16":
                {
                    var myVal = (ushort)value;
                    foreach (ushort val in values)
                    {
                        if ((myVal & val) > (ushort)0)
                        {
                            myVal ^= val;
                        }
                    }
                    if (myVal > (ushort)0)
                    {
                        throw new ArgumentException($"{value} is not a valid ordinal of type {type}.");
                    }
                }
                break;
            case "System.Int16":
                {
                    var myVal = (short)value;
                    foreach (short val in values)
                    {
                        if ((myVal & val) > (short)0)
                        {
                            myVal ^= val;
                        }
                    }
                    if (myVal > (short)0)
                    {
                        throw new ArgumentException($"{value} is not a valid ordinal of type {type}.");
                    }
                }
                break;
            case "System.UInt32":
                {
                    var myVal = (uint)value;
                    foreach (uint val in values)
                    {
                        if ((myVal & val) > (uint)0)
                        {
                            myVal ^= val;
                        }
                    }
                    if (myVal > (uint)0)
                    {
                        throw new ArgumentException($"{value} is not a valid ordinal of type {type}.");
                    }
                }
                break;
            case "System.Int32":
                {
                    var myVal = (int)value;
                    foreach (int val in values)
                    {
                        if ((myVal & val) > (int)0)
                        {
                            myVal ^= val;
                        }
                    }
                    if (myVal > (int)0)
                    {
                        throw new ArgumentException($"{value} is not a valid ordinal of type {type}.");
                    }
                }
                break;
            case "System.UInt64":
                {
                    var myVal = (ulong)value;
                    foreach (ulong val in values)
                    {
                        if ((myVal & val) > (ulong)0)
                        {
                            myVal ^= val;
                        }
                    }
                    if (myVal > (ulong)0)
                    {
                        throw new ArgumentException($"{value} is not a valid ordinal of type {type}.");
                    }
                }
                break;
            case "System.Int64":
                {
                    var myVal = (long)value;
                    foreach (long val in values)
                    {
                        if ((myVal & val) > (long)0)
                        {
                            myVal ^= val;
                        }
                    }
                    if (myVal > (long)0)
                    {
                        throw new ArgumentException($"{value} is not a valid ordinal of type {type}.");
                    }
                }
                break;
            default:
                throw new ArgumentException($"{type} does not have a valid backing type ({underlyingType}).");
                break;
        }
    }
    else
    {
        if (!type.IsEnumDefined(value))
        {
            throw new ArgumentException($"{value} is not a valid ordinal of type {type}.");
        }
    }

    return (TEnum)Enum.ToObject(type, value);
}

This will work as expected on [Flags] enumerations, which is great! But, there's some improvement to be made.

First and foremost: how do we get rid of that object parameter?

Well, we have a few options, but the easiest is to redefine our ToEnum on each numeric type, and create a version that takes a dynamic. It's ugly, but works:

public static TEnum ToEnum<TEnum>(this byte value) => DynamicToEnum<TEnum>(value);
public static TEnum ToEnum<TEnum>(this sbyte value) => DynamicToEnum<TEnum>(value);
public static TEnum ToEnum<TEnum>(this ushort value) => DynamicToEnum<TEnum>(value);
public static TEnum ToEnum<TEnum>(this short value) => DynamicToEnum<TEnum>(value);
public static TEnum ToEnum<TEnum>(this uint value) => DynamicToEnum<TEnum>(value);
public static TEnum ToEnum<TEnum>(this int value) => DynamicToEnum<TEnum>(value);
public static TEnum ToEnum<TEnum>(this ulong value) => DynamicToEnum<TEnum>(value);
public static TEnum ToEnum<TEnum>(this long value) => DynamicToEnum<TEnum>(value);

private static TEnum DynamicToEnum<TEnum>(dynamic value)
{
    ...

For each repeated case body, there's no good way to do this. With the use dynamic, we lose all safety of our types. There's no way to constrain it generically, because we can't say "it can be any number."

The best we could do would be a local function something like the following:

void testEnum<T>(T myVal, Func<T, bool> aboveZero, Func<T, T, T> and, Func<T, T, T> xor)
{
    foreach (T val in values)
    {
        if (aboveZero(and(myVal, val)))
        {
            myVal = xor(myVal, val);
        }
    }
    if (aboveZero(myVal))
    {
        throw new ArgumentException($"{value} is not a valid ordinal of type {type}.");
    }
}

Which means, we can use it to create a more readable version of our function:

public static TEnum ToEnum<TEnum>(this byte value) => DynamicToEnum<TEnum>(value);
public static TEnum ToEnum<TEnum>(this sbyte value) => DynamicToEnum<TEnum>(value);
public static TEnum ToEnum<TEnum>(this ushort value) => DynamicToEnum<TEnum>(value);
public static TEnum ToEnum<TEnum>(this short value) => DynamicToEnum<TEnum>(value);
public static TEnum ToEnum<TEnum>(this uint value) => DynamicToEnum<TEnum>(value);
public static TEnum ToEnum<TEnum>(this int value) => DynamicToEnum<TEnum>(value);
public static TEnum ToEnum<TEnum>(this ulong value) => DynamicToEnum<TEnum>(value);
public static TEnum ToEnum<TEnum>(this long value) => DynamicToEnum<TEnum>(value);

private static TEnum DynamicToEnum<TEnum>(dynamic value)
{
    Type type = typeof(TEnum);

    if (!type.IsEnum)
    {
        throw new ArgumentException($"{type} is not an enum.");
    }

    if (type.GetCustomAttributes(typeof(FlagsAttribute), true).Length > 0)
    {
        var values = Enum.GetValues(type);

        void testEnum<T>(T myVal, Func<T, bool> aboveZero, Func<T, T, T> and, Func<T, T, T> xor)
        {
            foreach (T val in values)
            {
                if (aboveZero(and(myVal, val)))
                {
                    myVal = xor(myVal, val);
                }
            }
            if (aboveZero(myVal))
            {
                throw new ArgumentException($"{value} is not a valid ordinal of type {type}.");
            }
        }

        var underlyingType = Enum.GetUnderlyingType(type).FullName;
        switch (underlyingType)
        {
            case "System.Byte":
                testEnum((Byte)value, (v) => v > (Byte)0, (v1, v2) => (Byte)(v1 & v2), (v1, v2) => (Byte)(v1 ^ v2));
                break;
            case "System.SByte":
                testEnum((SByte)value, (v) => v > (SByte)0, (v1, v2) => (SByte)(v1 & v2), (v1, v2) => (SByte)(v1 ^ v2));
                break;
            case "System.UInt16":
                testEnum((UInt16)value, (v) => v > (UInt16)0, (v1, v2) => (UInt16)(v1 & v2), (v1, v2) => (UInt16)(v1 ^ v2));
                break;
            case "System.Int16":
                testEnum((Int16)value, (v) => v > (Int16)0, (v1, v2) => (Int16)(v1 & v2), (v1, v2) => (Int16)(v1 ^ v2));
                break;
            case "System.UInt32":
                testEnum((UInt32)value, (v) => v > (UInt32)0, (v1, v2) => (UInt32)(v1 & v2), (v1, v2) => (UInt32)(v1 ^ v2));
                break;
            case "System.Int32":
                testEnum((Int32)value, (v) => v > (Int32)0, (v1, v2) => (Int32)(v1 & v2), (v1, v2) => (Int32)(v1 ^ v2));
                break;
            case "System.UInt64":
                testEnum((UInt64)value, (v) => v > (UInt64)0, (v1, v2) => (UInt64)(v1 & v2), (v1, v2) => (UInt64)(v1 ^ v2));
                break;
            case "System.Int64":
                testEnum((Int64)value, (v) => v > (Int64)0, (v1, v2) => (Int64)(v1 & v2), (v1, v2) => (Int64)(v1 ^ v2));
                break;
            default:
                throw new ArgumentException($"{type} does not have a valid backing type ({underlyingType}).");
                break;
        }
    }
    else
    {
        if (!type.IsEnumDefined(value))
        {
            throw new ArgumentException($"{value} is not a valid ordinal of type {type}.");
        }
    }

    return (TEnum)Enum.ToObject(type, value);
}

Overboard? Probably. But does it work? Oh yeah.

Additionally, as Peter Taylor mentions, you can use where TEnum : struct to help reduce some of the run-time issues, and bring them to the compile-type.


An iterator which behaves like an array with uniform values I'm implementing an iterator which takes a single value and which you can advance for n times with a dereferencing yielding the single value - but before or after that range of n 'positions' you can't dereference. So it's as though you filled a length-n std::vector with a single value and were taking a const iterator over it.

Imlementation is intentionally not thread-safe.

#include <stdexcept>
#include <iterator>
#include <cstddef>

template <typename E>
struct iterate_n_wrapper : public std::iterator<std::input_iterator_tag, E, size_t> {
    iterate_n_wrapper() = delete; // ... so it doesn't meet the forward iterator requirements
    iterate_n_wrapper(const iterate_n_wrapper<E>& other) = default;
    iterate_n_wrapper(const E& datum_, size_t times_)
        : datum(datum_), times(times_), pos(0) { };

    const E & operator[](off_t offset) const {
        auto offset_pos = pos + offset;
        if (offset_pos >= 0 && offset_pos < times) { return datum; }
        throw std::out_of_range("iteration range exceeded");
    }

    const E & operator *() const { return operator[](0); }
    const E * operator ->() const { return &(operator *()); }

    iterate_n_wrapper& operator ++() { pos++; return *this; }
    iterate_n_wrapper& operator --() { pos--; return *this; }

    iterate_n_wrapper operator ++(int) { auto ret = *this; operator++(); return ret; }
    iterate_n_wrapper operator --(int) { auto ret = *this; operator--(); return ret; }

    bool operator ==(const iterate_n_wrapper &other) const {
        return
            datum == other.datum &&
            times == other.times &&
            pos   == other.pos;
    }
    bool operator !=(const iterate_n_wrapper &other) const { return !operator==(other); }

protected:
    const E   datum;
    size_t    times;
    ptrdiff_t pos;
};

A simple example of use:

int main() {
    int a = 123;
    iterate_n_wrapper<int> iwa(a, 6);
    for(int i = 0; i < 4; i++) {
         std::cout << "iwa[" << i << "] = " << iwa[i] << "\n";
    }
    for(int i = 0; i < 100; i++) {
        std::cout << "i = " << i << ", got " << *(iwa++) << "\n";
    }
    return 0;
}

produces:

iwa[0] = 123
iwa[1] = 123
iwa[2] = 123
iwa[3] = 123
i = 0, got 123
i = 1, got 123
i = 2, got 123
i = 3, got 123
i = 4, got 123
i = 5, got 123
terminate called after throwing an instance of 'std::out_of_range'
  what():  iteration range exceeded
Aborted (core dumped)

so, the motivation is that this is a stand-in for an array/vector iterator when you can get the length from someplace else. Although I guess the lack of some kind of corresponding end() is a shortcoming.

As I alluded to in my question in the comments I think you should consider having a past-the-end iterator that would allow your iterator to be used with standard algorithms. Example:

std::copy(iterate_n_wrapper<int>(42, 4), iterate_n_wrapper<int>(), std::ostream_iterator<int>(std::cout, " "));

See std::istream_iterator for an example from the standard library of this concept in action.

cppreference.com has a nice example of how to achieve this.

It doesn't seem like an iterator is actually required to implement "past-the-endness" (note: I haven't dug into the standard to check), but it's something I, and I think most other C++ users, would expect. And the text in the link strongly hints that it's expected.

Normally you'd only supply operator* for an iterator and not operator[], but that's up to you.

Also I don't see a reason for using size_t as the difference_type (third template parameter to std::iterator) - why not stick with the default of std::ptrdiff_t?

You also need to implement Swapability for it to be satisfy the iterator concept.

Except for the missing past-the-end iterator, these are all minor quibbles. Building on std::iterator is was until C++17 (see @Olzhas comment) the right choice and I don't see anything wrong with the rest of your implementation.


Updating messages from a list of messages I've not posted binding specific messages to the gridview part. This shows updating that particular message or message from a list of messages databound with an XML file. My main concern is omitting redundant code in the else part. Any suggestions?

protected void grdMessage_RowUpdating(object sender, GridViewUpdateEventArgs e)
{
    DataRow xRow;
    DataSet mdsRedirect = new DataSet();
    mdsRedirect.ReadXml(st);

    if (Request["MessageID"] != null && Convert.ToInt16(Request["MessageID"]) != 0)
    {
        //Find specific item from XML and set updated value
        mdsRedirect.Tables[0].DefaultView.RowFilter = "MessageID=" + Convert.ToInt16(Request["MessageID"]);
        if (mdsRedirect.Tables[0].DefaultView.Count > 0)
        {
            mdsRedirect.Tables[0].DefaultView[0]["MessageText"] = ((TextBox)(grdMessage.Rows[e.RowIndex].FindControl("txtupdMessage"))).Text;
            mdsRedirect.Tables[0].DefaultView.RowFilter = string.Empty;
        }
        else {
            xRow = mdsRedirect.Tables[0].Rows[e.RowIndex];
            xRow["MessageText"] = ((TextBox)(grdMessage.Rows[e.RowIndex].FindControl("txtupdMessage"))).Text;
        }
    }
    else
    {
        xRow = mdsRedirect.Tables[0].Rows[e.RowIndex];
        xRow["MessageText"] = ((TextBox)(grdMessage.Rows[e.RowIndex].FindControl("txtupdMessage"))).Text;
    }

    grdMessage.EditIndex = -1;
    mdsRedirect.WriteXml(st);
    mdsRedirect.Dispose();
    getxml();//read XML file and bind it
}

Not sure if it's "optimized", but here's something of a simplification:

    protected void grdMessage_RowUpdating(object sender, GridViewUpdateEventArgs e)
    {
        string messageIdString = Request["MessageID"];
        int messageId = messageIdString == null ? 0 : Convert.ToInt16(messageIdString);

        using (DataSet mdsRedirect = new DataSet())
        {
            DataTable table = mdsRedirect.Tables[0];
            string updateMessageText = ((TextBox)grdMessage.Rows[e.RowIndex].FindControl("txtupdMessage")).Text;

            mdsRedirect.ReadXml(st);
            table.DefaultView.RowFilter = messageId == 0 ? table.DefaultView.RowFilter : "MessageID=" + messageId;
            if ((messageId != 0) && (table.DefaultView.Count > 0))
            {
                // Find specific item from XML and set updated value
                table.DefaultView[0]["MessageText"] = updateMessageText;
                table.DefaultView.RowFilter = string.Empty;
            }
            else
            {
                table.Rows[e.RowIndex]["MessageText"] = updateMessageText;
            }

            grdMessage.EditIndex = -1;
            mdsRedirect.WriteXml(st);
        }

        getxml(); // read XML file and bind it
    }


Functional Programming style in F# I'm normally a C# developer, but I've started to learn F# and I want to make sure I'm writing code in a functional way that suits the language. I've quickly pieced this together with my knowledge from reading Functional Programming for the Real World and various material from the Web.

Please look and provide feedback on style, layout or anything you want (Program.fs is where the meat is):

// DataUtils.fs
module DataUtils

open System.IO
open System.Runtime.Serialization.Json
open System.Text

let GetJsonFromObject<'T> (obj:'T) =
    use ms = new MemoryStream()
    (new DataContractJsonSerializer(typeof<'T>)).WriteObject(ms, obj)
    Encoding.Default.GetString(ms.ToArray())

let GetObjectFromJson<'T> (json:string) : 'T =
    use ms = new MemoryStream(ASCIIEncoding.Default.GetBytes(json))
    let obj = (new DataContractJsonSerializer(typeof<'T>)).ReadObject(ms)
    obj :?> 'T

// Web.fs
module Web

open System
open System.Net
open System.IO

let GetJsonFromWebRequest url =

    let req = WebRequest.Create(new Uri(url)) :?> HttpWebRequest
    req.ContentType <- "application/json"

    let res = req.GetResponse () :?> HttpWebResponse
    use stream = res.GetResponseStream()
    use sr = new StreamReader(stream)
    let data = sr.ReadToEnd()
    data

// Model.fs
module Model

open System.Runtime.Serialization

[<DataContract>]
type exchangeRate = {
    [<field: DataMember(Name = "to")>]
    toCurrency:string;
    [<field: DataMember(Name = "rate")>]
    rate:decimal;
    [<field: DataMember(Name = "from")>]
    fromCurrency:string }

// Program.fs
module Program

open System
open Model

let getExchangeRate fromCurrency toCurrency =
    let url = String.Format("http://rate-exchange.appspot.com/currency?from={0}&to={1}", fromCurrency, toCurrency)
    let json = Web.GetJsonFromWebRequest url
    let rate:exchangeRate = DataUtils.GetObjectFromJson json
    rate

let rec displayExchangeRate currencies =
    match currencies with
    | [] -> []
    | (a, b)::tl ->
        let r = getExchangeRate a b
        printfn "%s -> %s = %A" a b r.rate
        displayExchangeRate tl

let currencies = [
    ("GBP", "EUR")
    ("GBP", "USD")
    ("EUR", "GBP")
    ("EUR", "USD")
    ("USD", "GBP")
    ("USD", "EUR") ]

displayExchangeRate currencies |> ignore

printfn "Press any key to exit"
let input = Console.ReadKey()

As you can see it's not doing an awful lot at the moment, just getting some exchange rates from a REST service and displaying them. The next step would be to add exchange rates to the database. I'd normally use Entity Framework for this, but given that Functional Programming is partly about reducing the amount of state that is held, is EF even the way to go?

It looks good! Just a couple of points.

Instead of

let data = sr.ReadToEnd()
data

you can write

sr.ReadToEnd()

sprintf is more idiomatic than String.Format.
displayExchangeRate would be better written as a loop or using
List.iter (or Seq.iter).
DataContractJsonSerializer.WriteObject uses Encoding.UTF8, but you're decoding with Encoding.Default, which returns the OS's current ANSI code page.


Ruby banking system program As a beginner I'd like to get any kind of feedback. The more the better. Any optimization and style mistakes?

class Account
      attr_reader :name, :balance

      def initialize(name, balance=100)
        @name = name
        @balance = balance
      end

      private

      def pin
      @pin = 789
      end

      def pin_check
        puts "Welcome to the banking system, #{@name}!\n" + "To access your account, input PIN please"
        @input_pin = gets.chomp.to_i
      end

     def pin_error
        return "Access denied: incorrect PIN."
     end

    public

      def access
        if pin_check == pin
          puts "Input d: to deposit money, s: to show balance or w: to withdraw money"
          action = gets.chomp.downcase
            case action
            when "d"
            deposit
            when "s"
            display_balance
            when "w"
           withdraw
            else
        puts "Try again"
          end
        else puts pin_error
        end
      end

      def amount
        puts "Input the amount"
        @money = gets.to_i
      end

      def over_error
        print "You don't own that kind of money, dude! Your balance: $#{@balance}"
      end

      def deposit
       @balance += amount
        puts "Deposited: $#{@money}. Updated balance: $#{@balance}."
      end

      def display_balance
        puts "Balance: $#{@balance}."
      end

      def withdraw
        if  amount <= @balance
          @balance -= @money
           puts "Withdrew: $#{@money}. Updated balance: $#{@balance}."
        else
            puts over_error
        end
      end
    end

    checking_account = Account.new("James Bond", 520_000)
    checking_account.access

There are a few things that I would do differently and/or change:

I usually find it easier to put all the public methods at the top and the private methods at the bottom instead of mixing them. This is also what I see most people do.
Be consistent. In pin_error you return an error message, in over_error you print the message (elsewhere you use puts). I would make both methods just puts the error message.
I would not assign an instance variable in amount just return the value and then change withdraw to use a local variable. I would also call it get_amount or something since it does more than just return an amount.

def withdraw
  amt = get_amount
  if  amt <= @balance
    @balance -= amt
    puts "Withdrew: $#{amt}. Updated balance: $#{@balance}."
  else
    over_error
  end
end

The pin method is odd in that it returns the bin but also sets an instance variable unnecessarily. I would pass the pin into the constructor.
I would make pin_check actually check if the pin is correct and return true or false. I would also give it an active name, with a bang, like check_pin!


Concerned about thread safety I'm writing start/stop module for my application.
I have a class that manages application life cycle and I called it Work.
Work uses WorkState class that contains 4 states the application can be in, that's:

INITIALIZING (state between NOT_RUNNING and RUNNING)
RUNNING
FINALIZING (state between RUNNING and NOT_RUNNING)
NOT_RUNNING

The class also contains some useful methods to manage states.
Work uses QueryExecutor. QueryExecutor is just a subclass of ThreadPoolExecutor with overriden method afterExecute() that changes application state after performing a start/stop task. (e.g. changing state from INITIALIZING to RUNNING after performing a start operation).
MY VISION:
When a thread invoke the start()/stop() methods, the code mustn't allow other thread to interfere the process until it's done. Also, there's no way to perform the stop action when the system is not started and vice versa.
MY CONCERNS:

Should I change locking system to "fair"?
Will it work smoothly with 2 threads? (There's no possibility that the start()/stop() can be accessed at the same time by more than 2 threads)
Does the code have any bugs I can't see?
Any way to write it much simpler?

Work.java
public class Work {

    //public start() and stop() methods that can be accessed
    //from two threads:
    //- JavaFX main thread (user action)
    //- other thread (after all of the task are done)
    //
    //Those methods return a boolean that indicates if
    //the chosen action was performed properly
    //(just to show an alert to user)

    public static boolean start() {
        return processQuery(Query.START);
    }

    public static boolean stop() {
        return processQuery(Query.STOP);
    }

    //------Internals

    private static final QueryExecutor queryProcessor = new QueryExecutor(Work::unlockProcessing);
    private static final WorkState state = new WorkState();
    private static final Object lock = new Object();

    private static boolean processQuery(Query query) {
        synchronized (lock) {
            if (state.isInProperState(query)) {
                state.changeState(query);
                queryProcessor.execute(query == Query.START ? Work::startQuery : Work::stopQuery);
                return true;
            } else {
                return false;
            }
        }
    }

    private static void unlockProcessing() {
        synchronized (lock) {
            state.matureState();
        }
    }

    private static void startQuery() {
        // <SOME SYNCHRONOUS CODE>
    }

    private static void stopQuery() {
        // <SOME SYNCHRONOUS CODE>
    }

}

WorkState.java
public class WorkState {

    private State state = State.NOT_RUNNING;

    public void changeState(Query query) {
        state = query == Query.START ? State.INITIALIZING : State.FINALIZING;
    }

    public void matureState() {
        state = state == State.INITIALIZING ? State.RUNNING : State.NOT_RUNNING;
    }

    public boolean isInProperState(Query query) {
        return (state == State.RUNNING && query == Query.STOP)
                || (state == State.NOT_RUNNING && query == Query.START);
    }

    private enum State {
        INITIALIZING, RUNNING, FINALIZING, NOT_RUNNING;
    }

}

QueryExecutor.java
public class QueryExecutor extends ThreadPoolExecutor {

    private final Runnable unlockProcessingAfterExecute;

    public QueryExecutor(Runnable unlockProcessingAfterExecute) {
        super(1, 1, 0L,
                TimeUnit.MILLISECONDS, new LinkedBlockingQueue<>());
        this.unlockProcessingAfterExecute = unlockProcessingAfterExecute;
    }

    @Override
    protected void afterExecute(Runnable r,
                                Throwable t) {
        super.afterExecute(r, t);
        unlockProcessingAfterExecute.run();
    }

}

Query enum
public enum Query {
    START, STOP
}

Please take a look at this library for inspiration on how to state machine could be defined:
https://projects.spring.io/spring-statemachine/

Should I change locking system to "fair"? => Could you please clarify what you are trying to achieve
Will it work smoothly with 2 threads? (There's no possibility that the start()/stop() can be accessed at the same time by more than 2 threads) => Yes. You have declared the only monitor object (lock) and configured corePoolSize = 1, maximumPoolSize = 1.
Does the code have any bugs I can't see? => I do not understand what is the purpose of this exercise
Any way to write it much simpler? => I think yes. Work is a singleton. the synchronized keyword could be used directly on Work's methods. And without clarification, it looks like that the whole solution could be replaced with ThreadPoolExecutor, cause it is not clear why do we need RUNNING, FINALIZING states.


Separate model from DB mapper (entity manager) I've read that it is good to separate model from DB mapper (entity manager) (but at the end, it is perhaps personal - just a matter of opinion). Anyway, if I understood correctly, I should not have data models with DB connections, I should not have any methods with SQL (CRUD) in a class, right?

I will give you how my code looks and if you could tell me how to separate - where to put CRUD (for example separate file name), how it would look and how class User should look after that.

I have a database with a table named users with the following columns:

id
username
password
first_name
last_name

In the directory includes I have database.php:

$connection = new PDO($dsn, $user, $pass, [ PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]);

Also, in the directory includes I have user.php with a class named User and it has DB connection and methods with CRUD (SQL):

<?php

class User
{
    protected $connection = null;

    protected $statments = [];

    protected static $table_name="users";

    protected $id = null;
    protected $username = null;
    protected $password = null;
    protected $first_name = null;
    protected $last_name = null;

    private $arrayOfUserInstances = array();

    public function __construct(PDO $connection)
    {
        $this->connection = $connection;
    }

    // SETTERS ...

    // GETTERS ...

    // METHODS:

    private function instantiate($record) {
        $user = new User($this->connection);
        foreach ($record as $attribute=>$value) {
            if (property_exists('User', $attribute)) {
                $user->$attribute = $value;
            }
        }
        return $user;
    }

    public function findAllUsers()
    {
        if (!isset($this->statments[__METHOD__])) {
            $this->statments[__METHOD__] = $this->connection->prepare("SELECT * FROM users");
        }
        $stmt = $this->statments[__METHOD__];
        if (!($stmt->execute())) {
            return null;
        }
        while ($record = $stmt->fetch(PDO::FETCH_ASSOC)) {
            $user = $this->instantiate($record);
            $this->arrayOfUserInstances[] = $user;
        }
        $stmt->closeCursor();
        return $this->arrayOfUserInstances;
    }

    public function findById($id)
    {
        if (!is_numeric($id) || $id < 1) {
            throw new InvalidArgumentException('ID must be positive integer');
        }
        if (!isset($this->statements[__METHOD__])) {
            $this->statements[__METHOD__] = $this->connection->prepare('SELECT * FROM users WHERE id = :id');
        }
        $stmt = $this->statements[__METHOD__];
        $stmt->execute([':id' => $id]);
        $record = $stmt->fetch(PDO::FETCH_ASSOC);
        $stmt->closeCursor();
        if (!$record) {
            return null;
        }

        return $this->instantiate($record);
    }

    public function fullName() {
        if (isset($this->first_name) && isset($this->last_name)) {
            return $this->getFirstName()." ".$this->getLastName();
        } else {
            return null;
        }
    }
}

The User class is doing too much. It represents a "User" and also handles the data connection and mapping. You have a need for two classes in the very least. The Repository Pattern is a common design solution for this and is quite easy to set up.
You first need to split up your concerns. You have a "User" and a "place users are stored."
The User class would be called the Domain Model. This represents a row from the "users" table and contains the data and methods that operate on that data. It does not contain data access methods. Rather, it should only contain "business logic."
From Wikipedia:

A domain model is a system of abstractions that describes select aspects of a sphere of knowledge, influence, or activity (a domain [3]). The model can then be used to solve problems related to that domain. The domain model is a representation of meaningful real-world concepts pertinent to the domain that need to be modeled in software. The concepts include the data involved in the business and rules the business uses in relation to that data.
A domain model generally uses the vocabulary of the domain so that a representation of the model can be used to communicate with non-technical stakeholders.

(emphasis, mine)
class User
{
    private $id;
    private $username;
    private $password;
    private $first_name;
    private $last_name;

    public function __construct($username, $password, $id = 0) {
        $this->setUsername($username);
        $this->setPassword($password);
        $this->setId($id);
    }

    // Getters

    // Setters

    public function getFullName() {
        if (isset($this->first_name) && isset($this->last_name)) {
            return $this->getFirstName() . ' ' . $this->getLastName();
        } else {
            return null;
        }
    }

    public function canChangePassword($oldPassword, $newPassword) {
        // Logic to check if the password can be changed
    }

    public function changePassword($oldPassword, $newPassword) {
        if (!$this->canChangePassword($oldPassword, $newPassword)) {
            throw new Exception("Cannot change password");
        }

        $this->password = $newPassword;
    }
}

Not every property needs a setter method. In fact, a User is not even a valid object unless it has a username, and password. The constructor is the perfect place to supply those values because the username shouldn't change, and the password can change, but the change in value should be tightly controlled by the User object (hence the canChangePassword and changePassword methods).
The data access code would go into a UserRepository class that specializes in CRUD operations on the "users" table. You can optionally create an interface for the repository class in case you want to unit test your application and provide a mock user repository.
interface IUserRepository
{
    public function add(User $user);
    public function findAll();
    public function find($id);
    public function findByUsername($username);
    public function update(User $user);
    public function remove(User $user);
}

The interface sets up the main operations you can perform on Users in the database. The UserRepository class below is where we actually start the data access code:
class UserRepository implements IUserRepository
{
    private $connection;

    public function __construct(PDO $connection) {
        $this->connection = $connection;
    }

    public function add(User $user) {
        // Create prepared statement for INSERT
    }

    public function findAll() {
        $results = // Query by prepared statement
        $users = [];

        foreach ($results as $data) {
            $users[] = $this->createInstance($data);
        }

        return $users;
    }

    public function find($id) {
        $data = // Query by prepared statement

        return $this->createInstance($data);
    }

    public function findByUsername($username) {
        $data = // Query by prepared statement

        return $this->createInstance($data);
    }

    public function update(User $user) {
        // Create prepared statement for UPDATE
    }

    public function remove(User $user) {
        // Create prepared statement for DELETE
    }

    private function createInstance($data) {
        return $this->map($data, new User($data['username'], $data['password'], $data['id']));
    }

    private function map($data, User $user) {
        foreach ($record as $attribute => $value) {
            $method = 'set' . ucfirst($attribute);

            if (method_exists($user, $method)) {
                call_user_func_array(array($user, $method), array($value));
            } else {
                throw new Exception("No setter method exists for '$attribute'");
            }
        }

        return $user;
    }
}

The instantiating and mapping of User objects is kept internal to the repository class. You could split this concern out into its own "mapper" or "factory" class if you wanted, but I generally start out with the mapping code right inside the repository class. Later on if I need to split out the mapping code, doing so is a simple refactoring job because these methods are private.
Now let's explore a sample use case where we change the password of a user:
// Create the "repostory" object
$users = new UserRepository($pdo);

// Fetch the user from the database
$user = $users->findByUsername('jdoe');

// Modify the User
$user->changePassword('password1', 'password2');

// Persist the changes back to the database
$users->update($user);

Clean and simple. That's the goal of splitting out your data access from your domain model. Furthermore, since your User class is devoid of data access code, you can now write unit tests validating the business logic of its methods.
Unit Testing
Earlier I mentioned that defining an interface for the repository can assist unit testing. We'll take a simple "MVC" example to illustrate this point. We have a "users controller" class that handles an HTTP request. It has two arguments in its constructor. The first argument has a Type Hint of IUserRepository and the second argument is the $_GET or $_POST array.
class UsersController
{
    private $params;
    private $users;

    public function __construct(IUserRepository $users, $params) {
        $this->users = $users;
        $this->params = $params;
    }

    public function changePassword() {
        $user = $this->users->findByUsername($this->params['username']);
        $user->changePassword($this->params['password1'], $this->params['password2']);
        $this->users->update($user);
    }
}

The changePassword method queries the repository for the user and calls the changePassword method on the User object. Lastly, it updates the user. Since the $this->users property was injected into the UsersController object in the constructor with a Type Hint, we can provide our own mock object to unit test the controller with.
class MockUserRepository implements IUserRepository
{
    private $data;

    public function __construct($existingUsers = array()) {
        $this->data = array(
            'all' => $existingUsers,
            'added' => array(),
            'updated' => array(),
            'removed' => array()
        );
    }

    public function add(User $user) {
        $this->data['added'][] = $user;
        $this->data['all'][] = $user;
    }

    public function findAll() {
        return $this->data['all'];
    }

    public function find($id) {
        // Find in $this->data['all']
    }

    public function findByUsername($username) {
        // Find in $this->data['all']
    }

    public function update(User $user) {
        $this->data['updated'][] = $user;
    }

    public function remove(User $user) {
        // remove from $this->data['all']
        $this->data['removed'][] = $user;
    }

    public function getData() {
        return $this->data;
    }
}

Now, the simple unit test:
$userToFind = new User('jdoe', 'password1', 123);
$mockUsers = new MockUserRepository(array($userToFind));
$params = array(
    'username' => 'jdoe',
    'password1' => 'password1',
    'password2' => 'abc123'
);
$controller = new UsersController($mockUsers, $params);

$controller->changePassword();

// Assert that $userToFind has a different password
// Assert that $userToFind was "updated"

Now we can unit test something more complex like a "controller" in an MVC application without requiring a connection to the database.

@PeraMika said:

I hope that it is OK to just have class User and class UserRepository without interface and or other classes that you have created here.

The interface is not a requirement. The most important thing is to separate the Domain Model from the Data Access.
@PeraMika said:

Is it good to have two classes for every table in DB, for example - if we have a table "Comments" - class Comment and class CommentRepository? Or, if we have table categories - class Category and class CategoryRepository?

The answer is two parts: "Yes" and "It depends".
If you have two tables called "comments" and "categories" you should have two Domain Models: Comment and Category. Yes, you should create one Domain Model class per table in the database.
When it comes to creating the repository classes, it depends. It's not a requirement to create one repository class per database table. If you don't have a large number of tables, one class might suffice. As your application grows, the number of tables grow. As a result, your data access code grows in size and stuffing all of it into one file can become cumbersome to maintain.
Really the decision of how many repository classes you need comes down to good object oriented design. I always approach class design with these in mind first:

Do one thing and do it well
Don't Repeat Yourself (DRY)

Do you need one repository class per table? No. If you start out doing this then you are creating a system that is easily extensible, yet doesn't make any assumptions about future business requirements. This is why I advocate for one repository class per table.
@PeraMika asked:

In methods with SQL (PDO), for example findAll()... is it good to check if the query is executed successfully, and if not - return false; For example:
if (!($stmt->execute())) {
    return false;
}

Also, in findById($id) is it good to have something like:
$record = $stmt->fetch(PDO::FETCH_ASSOC); $stmt-> closeCursor();

if (!$record) {
    return false;
}

Many Object Relation/Mapping (ORM) libraries in multiple languages and platforms seem to implement these rules:

If multiple records can be returned, but the query does not return results, an empty array or "collection" is returned. This helps prevents null pointer exceptions in later code

If a single record should be returned, for instance querying by primary key, and the query does not return results, return null instead and let the calling code decide if this is an "exceptional" condition (and then throw an exception).

If a single record should be returned and multiple records are found, throw an exception. You are expecting something to be unique and it is not. Your repository cannot resolve this situation, so failing early and failing loudly is the best option.

If the execute method returns false because of a failure in the database or problem with the SQL, this is an "exceptional" condition. It is appropriate to throw an exception in this scenario with the message being the error message sent back from the database.


PHP PDO Class with multiple Select Efficiency and speed I am very new to classes, and not sure how efficient this class is, would appreciate improvement feedback and info on efficiency including speed.

The class includes a construct that creates a connection to the database and two other functions that get data from the database and return some values concated with some HTML.

This is made from code that I've found around the web mostly SO, so would be good if suggestions were easy to understand for a novice.

Can improvements be made to this so that it is efficient and best practices are incorporated?

<?php
class MyClass {
protected $PDO;

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

public function getCount($sq,$bkid,$bkname) {
    $sth = $this->PDO->prepare("SELECT COUNT(sentence) AS b FROM sentence, chapters, books WHERE MATCH(sentence) AGAINST (:q IN BOOLEAN MODE) AND chapters.book_Id = books.book_Id AND sentence.chapter_Id = chapters.chapter_Id AND books.book_Id = :bk");
    $sth->execute(array(':q' => $sq, ':bk' => $bkid));
    $row = $sth->fetch(PDO::FETCH_ASSOC);
    $total = $row['b'];
    if($total!=0)
    {
    $ensq = urlencode($sq);
    $count = "$bkname$total ";
    return $count;
    }
    else
    {
    return;
    }
}

public function getwriter($sqn) {
    $nsth = $this->PDO->prepare("SELECT COUNT(DISTINCT(writer)) AS n FROM sentence WHERE MATCH(writer) AGAINST (:q IN BOOLEAN MODE)");
    $nsth->execute(array(':q' => $sqn));
    $nrow = $nsth->fetch(PDO::FETCH_ASSOC);
    $ntotal = $nrow['n'];
    if($ntotal!=0)
    {
    $nensq = urlencode($sqn);
    $ncount = "writers$ntotal ";
    return $ncount;
    }
    else
    {
    return;
    }
}

public function getChapter($cqn) {
    $csth = $this->PDO->prepare("SELECT COUNT(chapter_Title) AS c FROM chapters WHERE MATCH (chapter_Title) AGAINST (:q IN BOOLEAN MODE)");
    $csth->execute(array(':q' => $cqn));
    $crow = $csth->fetch(PDO::FETCH_ASSOC);
    $ctotal = $crow['c'];
    if($ctotal!=0)
    {
    $censq = urlencode($cqn);
    $ccount = "Chapters$ctotal ";
    return $ccount;
    }
    else
    {
    return;
    }
}
}
$dbh = new PDO('mysql:host=localhost;dbname=dbname;charset=utf8', 'db', 'pass');
?>

And also is it ok to call the function multiple times next to each other like so:

$t = new MyClass($dbh);
echo $t->getCount($search,11,"Book 1");
echo $t->getCount($search,4,"Book 2");
echo $t->getCount($search,39,"Book 3");
echo $t->getCount($search,56,"Book 4");
echo $t->getWriter($search);
echo $t->getChapter($search);
$dbh = null;

The most important thing you have to learn about classes: every method should be responsible for one task only. Means a method that gets you a count should return you a count, not some HTML.

Two other minor improvements would be a better SQL formatting, so anyone could actually read it and a nice helper function that exists in PDO, fetchColumn():

So your methods should be like

public function getCount($sq,$bkid) {
    $sql = "SELECT COUNT(sentence)
            FROM sentence, chapters, books
            WHERE MATCH(sentence) AGAINST (:q IN BOOLEAN MODE)
                AND chapters.book_Id = books.book_Id
                AND sentence.chapter_Id = chapters.chapter_Id
                AND books.book_Id = :bk"
    $sth = $this->PDO->prepare($sql);
    $sth->execute(array(':q' => $sq, ':bk' => $bkid));
    return $sth->fetchColumn();
}

Whereas for the HTML formatting there should be another method/function. Let me suggest to you something like this:

function getLink($title, $count, $parameters)
{
    if ($count)
    {
        $qs = http_build_query($parameters);
        $title = htmlspecialchars($title);
        return "$title$total\n";
    }
}

it could be used like this:

$count = $t->getCount($search, 11);
echo getLink("Book 1", $count, ['id' => 11, 'q'=> $search]);

Here, as you can see, a handy http_build_query() would take care of encoding your keys and values.


Rendering a large collection using BackboneJS and LoDash I have a Playlist object which contains many PlaylistItem children. If I have 10,000 children then the UI gets blocked from rendering until all 10,000 children have been processed.

To avoid this, I've created a recursive function wrapped in a setTimeout. This allows a chunk of the playlist to be rendered, allow the UI to update by pausing for setTimeout and then continue to run until it is empty.

Is this a good implementation? Any critiques? I know I still should implement pagination in the long run, but this is for the short term.

render: function () {
    this.$el.html(this.template(
        _.extend(this.model.toJSON(), {
            //  Mix in chrome to reference internationalize.
            'chrome.i18n': chrome.i18n
        })
    ));

    //  Group playlistItems into chunks of 200 to render incrementally to prevent long-running operations.
    var chunkSize = 200;
    var playlistItemChunks = _.toArray(this.model.get('items').groupBy(function (playlistItem, index) {
        return Math.floor(index / chunkSize);
    }));

    var self = this;
    this.incrementalRender(playlistItemChunks, function () {

        self.$el.find('img.lazy').lazyload({
            container: self.$el,
            event: 'scroll manualShow'
        });
    });

    return this;
},

incrementalRender: function (playlistItemChunks, onRenderComplete) {
    //  Render a chunk:
    if (playlistItemChunks.length > 0) {
        var playlistItemChunk = playlistItemChunks.shift();

        //  Build up the views for each playlistItem.
        var items = _.map(playlistItemChunk, function(playlistItem) {
            var playlistItemView = new PlaylistItemView({
                model: playlistItem
            });

            return playlistItemView.render().el;
        });

        //  Do this all in one DOM insertion to prevent lag in large playlists.
        this.$el.append(items);

        var self = this;
        setTimeout(function() {
            self.incrementalRender(playlistItemChunks, onRenderComplete);
        });

    } else {
        onRenderComplete();
    }

},

You code looks good, I only can give a few suggestions:

I find it more convenient to use ...bind(this) instead of var self = this; - in this case the extra variable is not necessary the code looks better.
Maybe it is better to use an index within the array - in this way the array will not be copied several times. Of course, the JS is highly optimized internally, but it is a good to help it a little.
Rendering 10.000 items is still very CPU consuming process (especially on mobile devices) so maybe use the similar approach that you have used with images and build the next chunk only when the user scrolled down to the end?


Sorting CSV table in Powershell I'm wondering if some PS gurus can advise on whether my method for sorting and recombining some data fields by using an array of PSCustomObjects is as efficient as it might be.

I'm importing a CSV file that contains a number of person details, including Full Names in a single field that have a variety of separators between first and last names (e.g. comma, space, comma + space, semicolon, etc). Last name comes first. Sanitising those variants is also an objective.

The dataset needs to be sorted by last name and reassembled with a single FullName field. Doing a simple sort by the FullName field doesn't work reliably given the variety of delimiters between the name parts.

I've attempted it creating an array of new PSCustomObjects that comprises of all the original fields from the CSV plus the Full Name field split into two new name fields. It's then sorted by the last name, and the results are stored in a new array ready for output, joining the previously-split name fields back together with a comma and space.

Any suggestions for better efficiency welcome.

$data = import-csv .\Cust.csv
# CSV fields: FullName, StreetAddress, City, Postcode
# Create a temporary array for sorting
$splitNames = @()
# Split "FullName" into separate name parts and add all to temp array
$data | foreach {
    $spltName = $_.FullName -split '[\s|,|;]+'
    $splitNames += [PSCustomObject]@{
        Last = $spltName[0]
        First = $spltName[1]
        StreetAddress = $_.StreetAddress
        City = $_.City
        Postcode = $_.Postcode
    }
}
# Create an output array to hold the final result
$sortedNames = @()
# Sort the temporary array by lastname, rejoin name field and add to output array
$splitNames | Sort Last | foreach {
    $FullName = $_.Last, $_.First -join ', '
    $sortednames += [PSCustomObject]@{
        FullName = $Fullname
        StreetAddress = $_.StreetAddress
        City = $_.City
        Postcode = $_.Postcode
    }
}
$sortednames

Sample data - first row is the header:

"FullName","StreetAddress","City","Postcode"
"Bloggs,Joe","1 Some Street","City","1001"
"Bloggs Jane","1 Some Street","City","1001"
"Bloggs;Jill","1 Some Street","City","1001"
"Bloggs, Jo","1 Some Street","City","1001"

I'm glad we established in the comments that your code doesn't in fact run slow. When you use the word "efficiency", most people would take that to mean run-time speed (or space, depending on the context).

You should never optimize for efficiency unless there is a demonstrated lack of efficiency that has a negative impact on humans beings. Optimize for humans, not for computers. Computers don't care.

In this case, if I had gone down the path of trying to make it run faster, we would probably have ended up with less readable, less maintainable code.

We can certainly optimize for readability or style though, as you pointed out in the comments.

I should say first that I don't have a problem with your code. I think it's fine.

(This is one bug, however. Instead of Sort Last, you should do Sort Last,First. "Bloggs, Jane" should come before "Bloggs, Joe" in the output.)

I've rewritten the code below how I would personally write it. There are a couple of techniques that you may not be familiar with that can be very useful.

One is "capturing groups" in regular expressions. E.g. (?<first>.+), that matches a group named "first", and we can get the captured text with this: $matches['first']

($matches is a "magic variable" defined by PowerShell, if you are wondering.)

Another technique is "calculated properties" with Select-Object. You can make properties by supplying a property name and a script block that calculates the value. That is often more straightforward than creating objects with [PSCustomObject].

# E.g. "Bloggs;Joe" ==> "Bloggs, Joe"
function NormalizeName($fullName)
{
    $separators = '\s|,|;'
    if ($fullName -match "^(?<last>[^$separators]+)[$separators]+(?<first>.+)")
    {
        $last  = $matches['last']
        $first = $matches['first']
        "$last, $first"
    }
    else
    {
        # Doesn't match, so return unchanged.
        $fullName
    }
}

Import-Csv .\Cust.csv |
    Select-Object @{name="FullName"; exp={NormalizeName $_.FullName} }, StreetAddress, City, Postcode |
    Sort-Object FullName


Reverse each word of a string in C I had this interview question like a year ago and was asked to code, on a piece of paper, how to reverse each word of a string.  Since I am used to Java, I proposed the obvious answer of using split + reverse, which are native commands in Java.  I was then told I couldn't use those, so I floundered and ended up with a really terrible solution (even though it technically would've worked).

Anyway, it was bugging me lately, so I gave it a shot in straight C, which I am not very good at, so it took me a good while to actually get it working.

I was wondering:

Is this a good solution?
Have I forgotten anything obvious?
Have I done anything non-kosher in the C world?

Again, I'm not very good at C, so even small points will probably help me out.

#include <stdio.h>

void reverseString(char* start, char* end){
   while (start < end){
      char temp = *start;
      *start = *end;
      *end = temp;
      ++start;
      --end;
   }
}

char* word_start_index(char* p)
{
    while((*p != '\0') && (*p == ' ')){
        ++p;
    }

    if(*p == '\0')
        return NULL;
    else
        return p;
}

char* word_end_index(char* p)
{
    while((*p != '\0') && (*p != ' ')){
        ++p;
    }

    return p-1;
}

void main(){
    char arr[] = "kevin is a good programmer";
    char* test = arr;

    while (test != '\0'){
        char* curWordStart = word_start_index(test);
        if (curWordStart == NULL)
            break;
        char* curWordEnd = word_end_index(curWordStart);
        reverseString(curWordStart, curWordEnd);
        test = curWordEnd + 1;
    }
    printf("%s \n", arr);
}

Also, would taking a different approach, like in higher level languages of breaking the string into an array of strings (so I guess a 2D array of chars), then stepping through and reversing each one, be a good approach as well? I thought about this first and was unable to hash it out.

I have a few issues with in your code:

Consistency in naming - use either camelCase or not_camel_case but don't
mix
consistency in braces.  The opening brace for a function goes in the first
column.
word_start_index and word_end_index should take a const parameter
word_start_index is the same as strspn(string, " "); or if you are also
looking for punctuation, strspn(string, " \t\n.,;:");
word_end_index - as for word_start_index but use strcspn (note the
'c')
word_end_index as a function (ie not in your context) fails for an empty
string or a string starting with a space  (it returns the char before the
string starts).
variable test in main() is misnamed. I would prefer something that shows it
is a string.
the test while (test != '\0') in main() is wrong - should be *test !=
'\0'.
Your loop always exits from the break
no return or parameters in main()

Also, arguably the position of the stars in your pointers is wrong.  I prefer char* p to be written char *p, which makes it clear that it is p that takes the star. Consider code such as char* a, b;. This is bad because it gives the impression that b is a pointer.


A dynamic string array that's loopable without requiring a count Here's a dynamic loopable string array that doesn't require a count to be maintained at the caller level.

Definition

char **getList() {
    char **items = (char *[]) {"John", "Jane", NULL}; // dynamic/variable hard coded list

    int size = 0;
    for (char **ptr = items; *ptr; ptr++) { // count items
        size++;
    }

    char **data = malloc(sizeof(char *) * (size + 1));
    if (data == NULL) goto err;

    for (int i = 0; *items; items++, i++) { // copy
        data[i] = malloc(sizeof(char) * strlen(*items) + 1);
        if (data[i] == NULL) goto err;
        strcpy(data[i], *items);
    }

    data[size] = NULL; // last item
    return data;

    err:
    fprintf(stderr, "Allocation failed.\n");
    return NULL;
}

Caller

char **data = getList();
for (char **ptr = data; *ptr; ptr++) {
    puts(*ptr);
}
free(data);

Please review if this code can be improved upon for any redundancy or optimizations.

review if this code can be improved upon for any redundancy or optimizations.

Micro optimization

String duplication does not take advantage of the pre-calculation of the length.

    data[i] = malloc(sizeof(char) * strlen(*items) + 1);
    if (data[i] == NULL) goto err;
    strcpy(data[i], *items);

For longer strings, code may (or may not) improve with the below.  This tended to improve things.  Less so today (usually a tie) unless in embedded platforms.

    size_t sz = strlen(*items) + 1;
    data[i] = malloc(sz);  // dealing with `char` strings, no need for sizeof(char)
    if (data[i] == NULL) goto err;
    memcpy(data[i], *items, sz);

Note: sizeof(char) * strlen(*items) + 1 is logically wrong.  Should have been sizeof(char) * (strlen(*items) + 1), yet since sizeof(char) is 1, makes no arithmetic difference.

Redundancy

No need to start at 0 and add 1: Start at 1.  Better to use size_t for sizing.  Better to size to the referenced type, than explicitly code the type.

// int size = 0;
size_t size = 1;
for (char **ptr = items; *ptr; ptr++) {
    size++;
}
// char **data = malloc(sizeof(char *) * (size + 1));
char **data = malloc(sizeof *data * size);

Even better, just subtract:

for (char **ptr = items; *ptr; ptr++) {
    ;
}
size_t size = ptr - items;
char **data = malloc(sizeof *data * size);

Design flaw

if (data[i] == NULL) goto err; fails to free prior allocations.

Design

The for (int i = 0; *items; items++, i++) increments one pointer with a ++ and indexes the other.  I'd expect the same for both.

Caller flaw

free(data); leaks the individual strings. Recommend coding a freeList(char **).


Track a fix area with meanshift -OpenCV I need your help in OpenCV. I'm a student and am working on a project in which involves a little autonomous car. I use Python for the main programming language.

I have some problem with my meanshift.  When I run the program, I want the camera fixed on an area on the floor and stay on this area.  I would like for the car to drive alone while following the floor, and I don't want that meanshift to find another object different from the floor.

The program works well, but not optimally for the floor.  Do you have a solution or a way to proceed?

def camshift(frame1,frame2,trackWindow) :
    points=[]
    size1=frame1.shape[0] #width
    size2=frame1.shape[1] #length
    #hsv referentiel
    hsv = cv2.cvtColor(frame1, cv2.COLOR_BGR2HSV)
    #Mask
    mask = cv2.inRange(hsv,np.array((0., 60.,32.)), np.array((180.,255.,255.)))
    x0, y0, w, h = trackWindow

    trackWindow1=[x0,y0,w,h]
    x1 = x0 + w -1
    y1 = y0 + h -1

    hsv_roi = hsv[y0:y1, x0:x1]
    mask_roi = mask[y0:y1, x0:x1]
    #compute histogram for that area
    hist = cv2.calcHist( [hsv_roi], [0], mask_roi, [16], [0, 180] )
    #Normalisation
    cv2.normalize(hist, hist, 0, 255, cv2.NORM_MINMAX);
    hist_flat = hist.reshape(-1)
    points.append(hsv)
    prob = cv2.calcBackProject([hsv,cv2.cvtColor(frame2, cv.CV_BGR2HSV)],[0],hist_flat,  [0,180],1)
    prob &= mask
    term_crit = ( cv2.TERM_CRITERIA_EPS | cv2.TERM_CRITERIA_COUNT, 10, 1 )
    #checking conditions not to go out of bounds
    if (trackWindow[0] < 0) :
        trackWindow[0] = 0
    if (trackWindow[1] < 0) :
        trackWindow[1] = 0
    if (trackWindow[0]+trackWindow[3] >= size1) :
        trackWindow1.append(size1-y0)
        w=trackWindow1[4]
        trackWindow1.pop()
    if (trackWindow[0]+trackWindow[2] >= size2) :
        trackWindow1.append(size2-x0)
        h=trackWindow1[4]
        trackWindow1.pop()
    x0, y0, w, h = trackWindow
    if (trackWindow[0]+trackWindow[2])<100 :
        trackWindow = (125,125,200,100)
    if (trackWindow[1]+trackWindow[3])<100 :
        trackWindow = (125,125,200,100)
    # self.ellipse, trackWindow = cv2.CamShift(prob, trackWindow, term_crit)

    new_ellipse, track_window = cv2.CamShift(prob,trackWindow, term_crit)
    #we draw the rectangle on the relevant area
     cv2.rectangle(frame1, (trackWindow[0],trackWindow[1]), (trackWindow[0]+trackWindow[2],trackWindow[1]+trackWindow[3]), color=(255,0,0))

    cv2.imshow("mask",mask)
    return track_window

Whitespace. Use it. It makes code much clearer. Here's a list of places where you can add whitespace to make your code clearer.

(frame1,frame2,trackWindow) : \$\rightarrow\$ (frame1, frame2, trackWindow):
trackWindow1=[x0,y0,w,h] \$\rightarrow\$ trackWindow1 = [x0, y0, w, h]
( cv2.TERM_CRITERIA_EPS | cv2.TERM_CRITERIA_COUNT, 10, 1 ) \$\rightarrow\$ (cv2.TERM_CRITERIA_EPS | cv2.TERM_CRITERIA_COUNT, 10, 1)
cv2.imshow("mask",mask) \$\rightarrow\$ cv2.imshow("mask", mask)

There are many other places like this in your code, so I'm going to recommend looking at PEP8, Python's official style guide, on how to correct these.

Finally, in all your if conditions, are just if conditions. If you're not using elif, then all of these conditions will be evaluated if they are true. Are you sure that you want this?


Using R to transform Excel files to be combined I have 50+ excel files that I need to transform (and eventually combine) to use in analyses. I’ve written a code that transforms them into the format I need, but it is extremely slow for files with high species counts (the transformed output will have > 2 million rows). My first question: is there a way to speed up my function? My second question: once all the files are combined, there will be many millions of lines of data. Will I be able to manipulate (e.g. subset, statistically analyze) the data in R? Can R (and/or my laptop) handle that much data? I’ve only used R with several thousand lines and a hundred or so columns before, so I don’t know what’s possible.
The first part of the code below is to produce a data frame resembling what my files look like. In reality, my files are larger (45rows, 12-21 columns) and they are not all the same (different number of columns, and the Habitat column is in different locations with different names), thus some of the strange code in the function. I could clean up each file so they are the same before I use my function, if that would be faster.

Sample data

library(dplyr)
#library(readxl) #this isn't used here, but this is what I use to load the data files.

aa<-c('Jul','Jul','Jul','Oct','Oct','Oct')
bb<-c('MA1','MA2','MA3','MA1','MA2','MA3')
a<-c(NA,100:104) #when loaded, the data has both NA's and 0's
b<-seq(1,12,2)
c<-seq(0,35,6)
d<-c(NA,NA,2000,NA,200,0)
e<-c(1:6)
Chiro.1<-data.frame(aa,bb,a,b,c,d,e)
colnames(Chiro.1)<-c('Chironomidae','Habitat','0','0.5','1','2','3')
Chiro.1<-as_tibble(Chiro.1)
Chiro.1$Habitat<-as.character(Chiro.1$Habitat)
Chiro.1$Chironomidae<-as.character(Chiro.1$Chironomidae)

The function

Cedar.bs.fun<-function (x, scol,Hcol) {
  #x = dataframe
  #scol = column number data starts
  #Hcol = column habitat type is in
  spe.list<-list()
  pb <- txtProgressBar(min = 0, max = nrow(x), style = 3)
  for (i in 1:nrow(x)) for (j in scol:ncol(x)){ #not every file has the same number of rows and columns
    if (!is.na(x[i,j])) {
      dat <- as.data.frame(matrix(ncol=4, nrow=0))
      colnames(dat)<-c('Taxa','Habitat','Date','Size')
      if (x[i,j]> 0) {
        n<-round(sum(x[i,j]),0) #original files have some decimals
        for(k in 1:n) {
          dat[k,1] = as.character(colnames(x[,1]))
          dat[k,2] = as.character((x[i,Hcol]))
          dat[k,3] = as.character((x[i,1]))
          dat[k,4] = colnames(x[,j])
          setTxtProgressBar(pb, i)
        }
      }
      spe.list[[length(spe.list)+1]] <- dat
      spe.list.1<-do.call("rbind",spe.list)
    }
  }
  spe.list.1$Size<-as.numeric(as.character(spe.list.1$Size)) #the column headings of the original files are the beginnings of size bins (eg. 1 mm - 2 mm)
  spe.list.1$Size<-spe.list.1$Size+0.5 #I need the final size to be the middle of the bin
  spe.list.1$Size[spe.list.1$Size==0.5]<-0.25
  spe.list.1$Size[spe.list.1$Size==1]<-0.75
  close(pb)
  return(spe.list.1)
    }

Run the function

final<-Cedar.bs.fun(Chiro.1,scol=3,Hcol=2)
#combined.1<-rbind(final,final.2,final.3,etc.) #eventually I will be combining all 50 or so files into one data frame for further manipulation and statistical analysis

And yes, the original files are mine made long before I new much of anything about R and how to organize data. Now I'm paying for it. Still tons to learn.

I have not figured out all processing you need to do, so my solution will not give you the exact same data.frame, but it should be close enough for you to get by with minimal post-processing.

General consideration

You are using nested for loops to fill a data.frame. This is usually a bad idea, there are almost always better ways to go about it. If you really need to use loops, read about preallocation first (for example in this question on stackoverflow question)
Think about the structure of your data. Do you really need one row for each single individual, or is a count variable enough? (see below)
Your task can be split up into two distinct steps: Reshaping the input, and replicating the rows. A function should always do one thing and not several (though in this case I agree that it is not obvious that you are actually doing two things)

Reshaping the input

The first function restructures your data so that it is easier to deal with. We use the reshape2 package for that:

parse_chiro <- function(x){
  taxa <- names(x)[[1]]
  reshape2::melt(
    Chiro.1,
    id.vars = c("Habitat", taxa),
    variable.name = "Size",
    value.name = "count"
  )
}

res <- parse_chiro(Chiro.1)

This gives you a data.frame with all the columns you want, + a count variable that contains how often you want the row to be replicated. The syntax for reshape is a bit hard to explain and I refer you to the package documentation for that. You can achieve something similar with the newer tidyr package, but I personally prefer reshape.

Expanding the data

The next part is expanding the data.frame with the count variable.
This is where most if not all of the speed improvement to your original function comes from. I cannot think of an application where you would want your data like that, but if you still need to do it, go about it like this:

expand_chiro <- function(x){
  # Removes all rows with NA count. I am not sure if you really want that?
  x <- x[!is.na(x$count), ]
  x$rowid <- seq_len(nrow(x))
  rep_rowid <- rep(x$rowid, round(x$count))
  x[rep_rowid, ]
}

Using the vectorized function rep() and data.frame row indexing will yield a significant speed increases to your nested loops. There were also a few things wrong with your loop that could have improved the performance, but the way I propose is much cleaner, so I am not going into that.

Remarks

rbind all your parsed data.frames before you do the expanding (so that you only need to expand once)
I would recommend to stick to the tidyverse style guide for naming your functions and variables. It is currently the most popular style guide for R, and you are already using tidyverse packages (readxl, dplyr)
2M rows are no problem in R, the limit is your RAM. You can use format(object.size(x), "auto") to display the size of your object in human readable form. In theory, you can work with objects about 1/3 - 1/2 the size of your available RAM, but if you are not super careful and know what you are doing you might crash R. Your data.frame should be only a few hundred MB so no need to worry.


Reverse each word in the string. All spaces in the string should be retained This was part of a code challenge.  I use them to help me learn c++/c# along with tutorials and guides.  Basically I was taking a string...

  The quick brown fox jumps over the lazy dog.

...and reversing the words in the string like this.

  ehT kciuq nworb xof spmuj revo eht yzal .god

The following code works and compiles.  However, I feel like this was a verbose way to achieve the objective.

  split

This function was a copy paste from another Stack Overflow page.  I'm not sure if this is idiomatic in c++, but it feels like an overkill.  I just used it because it worked when I tested it on single word strings.

  vector_to_string

I came up with this function because I had forgotten how to do this in c++.  I recall a built in function somewhere in its library.

  reverse_words

This was the provided function for the challenge.  Everything inside of the {} is what I came up with.

Everything in the int main() I came up as well for testing.  Test are provided, but I haven't quite learned how to implement this offline yet.  Following is my code, with the test after that.

#include <string>
#include <iostream>
#include <vector>
#include <algorithm>

using namespace std;

vector<string> split(string s, string delimiter)
{
    size_t pos_start = 0, pos_end, delim_len = delimiter.length();
    string token;
    vector<string> res;

    while ((pos_end = s.find(delimiter, pos_start)) != string::npos)
    {
        token = s.substr(pos_start, pos_end - pos_start);
        pos_start = pos_end + delim_len;
        res.push_back(token);
    }

    res.push_back(s.substr(pos_start));
    return res;
}

string vector_to_string(vector<string> v)
{
    string s;

    for (auto &e : v)
    {
        reverse(e.begin(), e.end());
        s += e + " ";
    }

    return s;
}

string reverse_words(string str)
{
    vector<string> v = split(str, " ");
    string s = vector_to_string(v);
    return s.substr(0, s.length() - 1);
}

int main()
{
    cout << reverse_words("The quick brown fox jumps over the lazy dog.") << endl;
    cout << reverse_words("apple") << endl;
    cout << reverse_words("a b c d") << endl;
    cout << reverse_words("") << endl;
    cout << reverse_words("ab   ba   cd") << endl;
}

I was provided with these tests:

Describe(Tests)
{
  It(Sample_Test_Cases)
  {
    Assert::That(reverse_words("The quick brown fox jumps over the lazy dog."), Equals("ehT kciuq nworb xof spmuj revo eht yzal .god"));
    Assert::That(reverse_words("apple"), Equals("elppa"));
    Assert::That(reverse_words("a b c d"), Equals("a b c d"));
    Assert::That(reverse_words("double  spaced  words"), Equals("elbuod  decaps  sdrow"));
    Assert::That(reverse_words(""), Equals(""));
    Assert::That(reverse_words("ab   ba   cd"), Equals("ba   ab   dc"));
  }
};

Anyhow, any advice on this is much appreciated and thank you for your help.  Again, I feel like this is a verbose way to solve this problem and I suspect it can be greatly optimized as well.

The split function

vector<string> split(string s, string delimiter)
{
    size_t pos_start = 0, pos_end, delim_len = delimiter.length();
    string token;
    vector<string> res;

    while ((pos_end = s.find(delimiter, pos_start)) != string::npos)
    {
        token = s.substr(pos_start, pos_end - pos_start);
        pos_start = pos_end + delim_len;
        res.push_back(token);
    }

    res.push_back(s.substr(pos_start));
    return res;
}

delim_len should be declared as const, because it doesn't change.  Or simply drop it; it is only used in one place.

This function forces all substrings to be first copied into the token variable and then copied into the vector — that's two copies.  It can be reduced to one copy by using std::move (defined in <utility>) on the argument to push_back, or by simply dropping the token variable.

The function should have std::string_view (defined in <string_view>) parameters instead of std::string parameters for efficiency.

Putting everything together:

auto split(std::string_view s, std::string_view delimiter)
{
    std::vector<std::string> result;

    std::size_t pos_start = 0, pos_end;
    while ((pos_end = s.find(delimiter, pos_start)) != s.npos) {
        res.push_back(s.substr(pos_start, pos_end - pos_start));
        pos_start = pos_end + delimiter.size();
    }

    res.push_back(s.substr(pos_start));
    return res;
}

vector_to_string

string vector_to_string(vector<string> v)
{
    string s;

    for (auto &e : v)
    {
        reverse(e.begin(), e.end());
        s += e + " ";
    }

    return s;
}

vector_to_string is a non-descriptive name — reverse_join may be better.  The delimiter should be an argument rather than hard coded.

This function should strip the last space instead of letting its caller do so.  Note that this function performs two operations logically (reverse and join).

Copying the whole vector takes a lot of space; consider copying one string at a time.

And yes, there is a simpler way to do this using std::accumulate (defined in <numeric>):

auto result = std::accumulate(v.begin(), v.end(), std::string{},
                              [](std::string lhs, std::string_view rhs) {
                                  lhs += rhs;
                                  lhs += ' ';
                                  return lhs;
                              });
result.pop_back();

reverse_words

string reverse_words(string str)
{
    vector<string> v = split(str, " ");
    string s = vector_to_string(v);
    return s.substr(0, s.length() - 1);
}

Again, std::string_view parameter.  Even if the stripping happens here, it should be done using pop_back to avoid copying the string and enable named return value optimization.

Simplification

Here's a simplification, using streams to do the split job:

#include <cassert>
#include <iostream>
#include <sstream>
#include <string>
#include <string_view>

std::streamsize read_spaces(std::istream& is)
{
    std::streamsize count = 0;
    while (is.peek() == ' ') {
        is.get();
        ++count;
    }
    return count;
}

// const std::string& because std::istringstream is constructed from a std::string
std::string reverse_words(const std::string& string)
{
    std::istringstream iss{string};
    std::ostringstream oss{};

    for (std::string word; iss >> word;) {
        std::reverse(word.begin(), word.end());
        oss << word << std::string(read_spaces(iss), ' ');
    }
    return oss.str();
}

int main()
{
    assert(reverse_words("The quick brown fox jumps over the lazy dog.") == "ehT kciuq nworb xof spmuj revo eht yzal .god");
    assert(reverse_words("apple") == "elppa");
    assert(reverse_words("a b c d") == "a b c d");
    assert(reverse_words("double  spaced  words") == "elbuod  decaps  sdrow");
    assert(reverse_words("") == "");
    assert(reverse_words("ab   ba   cd") == "ba   ab   dc");
}

(live demo)

Miscellaneous

#include <string>
#include <iostream>
#include <vector>
#include <algorithm>

Sort the #includes in alphabetical order to each navigation.

using namespace std;

Avoid global using namespace std;.  It brings in many common identifiers from the std namespace and causes name clashes.  Although it doesn't matter too much for a small program, get into the habit of explicitly qualifying names with std:: instead.  An alternative is using declarations (using std::string, std::vector;), which only pulls in the specified names.  See Why is using namespace std; considered bad practice? for more information.


Email text validator I'm using this pattern for the first time and wanted to check if this is the correct implementation.

class.validator.strategy.php

abstract class ValidatorStrategy
{
    abstract public function check( $name, $val );

}

class.text.validator.php

class TextValidator extends ValidatorStrategy
{

    public function check( $name, $val )
    {
         //logic here
    }
}

class.number.validator.php

class NumberValidator extends ValidatorStrategy
{

    public function check( $name, $val )
    {
         //logic here
    }
}

class.validator.php

include('validator.strategy.php');
include('class.text.validator.php');
include('class.number.validator.php');
include('class.email.validator.php');

class Validator
{

    //holds strategy object
    protected $validatorStrategy = array();

    //holds form field
    protected $fields = array();

    public function __construct()
    {
        $this->validatorStrategy[ 'text' ] = new TextValidator();
        $this->validatorStrategy[ 'number' ] = new NumberValidator();
        $this->validatorStrategy[ 'email' ] = new EmailValidator();
    }

    public function ruleForTextField( $name, $value )
    {
        $this->fields[ $name ][ 'value' ] = $value;
        $this->fields[ $name ][ 'type' ] = 'text';
    }

    public function ruleForNumbertField( $name, $value )
    {
        $this->fields[ $name ][ 'value' ] = $value;
        $this->fields[ $name ][ 'type' ] = 'number';
    }

    public function ruleForEmailField( $name, $value )
    {
        $this->fields[ $name ][ 'value' ] = $value;
        $this->fields[ $name ][ 'type' ] = 'email';

    }

    public function isValidate()
    {
        $status = 0;

        foreach ( $this->fields as $key => $val )
        {
            if ( !$this->validatorStrategy[ $val[ 'type' ] ]->check( $key, $val ) )
            {
                $status++;
            }

        if ( $status == 0 )
        {
            return true;
        }
        else
        {
            return false;
        }
    }

}

Uses:

$validator = new Validator();
$validator->ruleForTextField('username', $_POST['username'] );
$validator->ruleForNumberField('age', $_POST['age'] );
$validator->ruleForEmailField('email', $_POST['email'] );

if($validator->isValidate())
{
    echo 'validated';
}
else
{
   echo 'fail validation';
}

Inside the isValidate() method, it iterate against all validator strategy and perform appropriate logic.

Is this is the correct way to use this pattern?

No, strategy pattern relies on an interface to allow composition to determine functionality at run time.  Notice that it is easier to extend with new speaking functionality; and can be used to execute at run time.  Here you are just using the array of the interface instantiations to do multiple validations.

I.E.

Interface for the pattern

public interface Speak {
  public void execute();
}

Implementations of the pattern

public class Quack extends Speak {
  public void execute() { /* Quack */ }
}

public class Bark extends Speak {
  public void execute() { /* bark */ }
}

Class that uses the strategy

public Animal {
  private Speak _speak;
  public Animal(Speak s) { _speak = s; }
}

Example usage

main() {
  new Animal(new Bark()); /* creates animal that barks */
  new Animal(new Quack()); /* creates animal that quacks */
}


Class that holds calculation results and has to be immutable I'm roughly trying to follow the guidelines in "Effective Java", "Item 15: Minimize mutability", hence the class is not "final" on purpose.

Information is gathered from a number of temperature meters.
Some may fail to report, so the size of the map is not known.

The serializing bothers me. Is there a simpler way to make this class immutable ?

public class Result<T extends Number> {
    private final Map<String, T> result;

    private Result(final Map<String, T> result) {
        this.result = result;
    }

    public static final Result<Number> of() {
        return new Result<>(new LinkedHashMap<>());
    }

    /** Add or replace value. */
    public Result<T> put(final String key, final T value) {
        final Map<String, T> copy = new LinkedHashMap<>(result);
        copy.put(key, value);
        return new Result<>(copy);
    }

    @SuppressWarnings("unchecked")
    public T get(final String key) throws Exception {
        return ((T) convertFromBytes(convertToBytes(result.get(key))));
    }

    @Override
    public String toString() {
        return result.toString();
    }

    private byte[] convertToBytes(Object object) throws IOException {
        try (ByteArrayOutputStream bos = new ByteArrayOutputStream(); ObjectOutput out = new ObjectOutputStream(bos)) {
            out.writeObject(object);
            return bos.toByteArray();
        }
    }

    private Object convertFromBytes(byte[] bytes) throws IOException, ClassNotFoundException {
        try (ObjectInput in = new ObjectInputStream(new ByteArrayInputStream(bytes))) {
            return in.readObject();
        }
    }
}

At the moment your class is not immutable because the Map given in the constructor is not copied (and Map itself isn't immutable). You should instead have a constructor like

public Result(final Map<String, T> result) {
    this.result = new LinkedHashMap<>(result);
}

What's the goal of the of() method ? Shouldn't it be named empty() instead ?

public static final Result<Number> empty() {
    return new Result<>(new LinkedHashMap<>());
}

Or having two parameters (1 key, 1 value) ?

public static final Result<Number> of(final String key, final T value) {
    Map<String, T> base = new LinkedHashMap<>();
    base.put(key, value);
    return new Result<>(base);
}

The fact that get() throws an IOException or a ClassNotFoundException bothers me because it makes the current underlying implementation "leaking" out of your class. Who wants (and expects) to have to handle an IOException when getting a value from a Map ?
Since these exceptions are not supposed to happen in this case, I would rethrow them as unchecked exceptions

@SuppressWarnings("unchecked")
public T get(final String key) throws Exception {
    try {
        return ((T) convertFromBytes(convertToBytes(result.get(key))));
    }
    catch(IOException | ClassNotFoundException e) {
        throw new RuntimeException(e);
    }
}


Returning substrings (motifs) and original strings (sequences) from a file of strings (sequences) I would like to get some help/tips on writing better and more pythonic code, as well as variable naming.

Code info:

.backtranscribe() is just a method to convert the U's in the sequence to T's (ATUG -> ACTG)
a - 1-based index where substring starts
b - size of substring
c - text input of names of sequences to get substrings of (the data/sorted...fa file has hundreds of name/sequence pairs)

def comp(a, b, c):
    mirna_set = set(c.data.splitlines())
    motif_size = b
    results = []
    motif_set = set()
    handle = open("data/sorted.human.mature.fa", "rU")
    for record in SeqIO.parse(handle, "fasta"):
        if record.id in mirna_set: # check if db mirna is one in user input
            index_5p = a-1
            flag_motif_unique = False
            motif = ""
            while(flag_motif_unique == False):
                motif = record.seq.back_transcribe()[index_5p:index_5p+motif_size]
                if motif not in motif_set:
                    flag_motif_unique = True
                    motif_set.add(motif)
                index_5p += 1
            pos = "pos:" + str(index_5p) + "-" + str(index_5p+10)
            results.append(">" + record.id + " " + motif + " " + pos)
            results.append(record.seq.back_transcribe())
        else:
            continue
    handle.close()
    return(results)

Full app here (with input/output)

Disclaimer: I know very little about bioinformatics, so I’m reviewing this on the quality of the Python code. I’m not qualified to comment on the scientific accuracy.

So here are a few suggestions:

You need better comments and documentation.  Ideally a docstring on the function(s), and some more comments that explain why the code was written this way – how does it relate to the underlying scientific principles? As it stands, it’s fairly opaque, and so it would be hard to another scientist to verify that it’s a correct implementation.

As a sidebar to this, you should pick much better variable names as the inputs to your function – a, b and c are utterly useless. Likewise the function name isn’t very helpful – lots of functions compute something, but what does this function compute?

Perhaps something like:

def gen_uniq_subsequences(index_5p, motif_size, mirna_list):

More characters, but substantially easier to read.
The output isn’t a particularly useful format. You’re returning a list of strings, which is fine for printing, but what if somebody wants to get the individual components of those results. Say, break out the position and the motif separately from the record ID?

I’d have this function return some chunk of structured data (say, a namedtuple), and then the pretty printing goes off to a separate function. Makes it much easier to reuse the outputs of this function.

Which leads me to my next comment…
Your function is doing too much work.  Ideally a function should have a single responsibility, an action that can be described in a single sentence. But your function does quite a lot:

Unpacks the third input from a string into a set.
Opens the sorted.human.mature.fa file, and starts parsing it.
Constructs the results list, which is some sort of scientific computation.
Assembles that into rigid strings for pretty-printing.

Most of those actions don’t need to be in this function – pushing them out will allow you to simplify this part of the code, which makes it easier to review, read and maintain.

For example, suppose I already have a set that contains my miRNAs. It would be much cleaner if I could pass that in directly, rather than packing it into a string so that you can unpack it again.
You can simplify your looping constructs. Two places immediately jump out at me:

You don’t need the else: continue; if you omit this the you get the same behaviour.
If you slightly rearrange the while loop and add a break, you can get rid of the flag_motif_unique sentinel variable.

Neither of these are ground-breaking, but they make your code a bit easier to write.


Makefile for building classwork from markdown files and LaTeX The following makefile builds my classwork for my various college classes and builds pdfs to hold the notes I took during class. I would like feedback on my directory structure and any optimizations that can be thought of to help improve it. I've been using and updating it for the past 18 months. I know that I should probably document it a bit better

The directory structure of the classes is as follows

ROOT
|- (class short code)
|  |- various subdirs for classwork
|- .. repeated for each of the classes I'm taking this semester
|- old
|  |- s1 (classes from semester 1)
|  |- s2 (classes from semester 2)
|- mybib.bib (biblatex file for sources)

The makefile is then as follows

# Makefile for college essays
#

PANDOC  := pandoc
#LATEX_FLAGS := -interaction=batchmode

SEARCHDIR  := $(shell find ./* -prune -type d \( ! -name "old*" \) -print)
LATEXFILES := $(shell find ${SEARCHDIR} -name '*.tex')
MARKDOWN   := \
    $(shell find ${SEARCHDIR} \( -name '*.md' -and ! -path '*node_modules*' \))
DOT_GRAPHS := $(shell find ${SEARCHDIR} -name '*.dot')
SLIDESHOWS := $(shell find ${SEARCHDIR} -name '*.slideshow')

# Pandoc Stuff
TEMPLATE   := college.latex
BIBFILE    := mybib.bib

PDFS       := $(MARKDOWN:.md=.pdf) $(LATEXFILES:.tex=.pdf)
#DOCX       := $(MARKDOWN:.md=.docx)
GRAPHOUT   := $(DOT_GRAPHS:.dot=.eps)
SLDESHWOUT := $(SLIDESHOWS:.slideshow=.ss.pdf)
NULLREDIR:= #> /dev/null

# Begin Primary Target
all: $(PDFS) $(GRAPHOUT) $(SLDESHWOUT)

# target to just build the latex documents
latex-out: $(LATEXFILES:.tex=.pdf)

# Clean up targets
clean: mdclean texclean dotclean
mdclean:
    rm -f $(PDFS)
texclean:
    rm -f *.aux
    rm -f *.bbl
    rm -f *.bbl
    rm -f *.log
    rm -f *.blg
    rm -f *.xml
    rm -f *.out
    rm -f *.bcf
dotclean:
    rm -f $(GRAPHOUT)
    rm -f $(DOT_GRAPHS:.dot=-eps-converted-to.pdf)

#######################################
# Build Rules for Various File Types
#######################################
%.pdf: %.md college.latex apa.csl
    $(info $@ from $<)
    @${PANDOC} --bibliography ${BIBFILE} --filter pandoc-citeproc \
        $< -o $@ --template ${TEMPLATE}

%.pdf: %.tex mybib.bib
    -$(info $@ from $<)
    -pdflatex ${LATEX_FLAGS} $< $(NULLREDIR)
    -if [ -e $(notdir $(basename $<)).bcf ] ; then \
            biber $(notdir $(basename $<)).bcf $(NULLREDIR) ; \
        else \
            bibtex $(notdir $(basename $<)).aux $(NULLREDIR) ; \
        fi
    -pdflatex ${LATEX_FLAGS} $< $(NULLREDIR)
    -pdflatex ${LATEX_FLAGS} $< $(NULLREDIR)
    -mv $(notdir $@) $@ $(NULLREDIR)
    -rm -f $(basename $<).aux

%.ss.pdf: %.slideshow
    pandoc --bibliography ${BIBFILE} -f markdown \
        -t beamer $< -o $@

%.eps: %.dot
    dot -Teps $< -o $@

%.png: %.dot
    dot -Tpng $< -o $@

include depend.makefile

I have a similar script.  So from my experiences I would:

build the pdfs through latexmk

sometimes it needs more runs of pdflatex.  I found it too hard to get right in plain make.

have directories for work and artefacts.

I don't like to have temporary files among my carefully written texts. The risk of an accidental rm or overwrite is just always present.
Generally the idea is to split files into: pure input, files that are created.  And also these split into files that can be discarded and which should be archived (say they were handed in).

I found it useful to have a "work" directory for the dirty stuff.  It helps to give each document its own subfolder.  In latexmk I can use the -o parameter as -o work/($basename $<)/.  That keeps files reasonably separated.  Then I can also do parallel builds.

Stuff that needs to be kept can just be moved over with a rule to copying them.  Cleaning also gets much easier as you can just wipe the work folder empty.

make smaller projects

I would at least at the class level give them their separate makefile and directory structure. Personally I do it per each document.  Just imagine that after say a year you need to adjust something in the build - and then you end up debugging all your previous essays makefiles and/or text to not lose them.

Not makefile related - but useful is to have the texts in git repositories.   If you put all in one monolithic project the history is going to be very messy.  Much easier to follow on a per project basis.