Refactor the method which is the sequence of the similarly looking steps to (or towards) the design patterm(s) I need some help to understand if the code below could be refactored to something less straightforward, less repetitive and more towards any appropriate pattern.

What I feel uncomfortable with in the code is the flow of repetitive tasks with the same pattern like:

// Get the result from some operation (API call / or any other operation);
// Check if the result is somehow valid;
// If it is not valid, set the response object accordingly and return early;
// If it is valid, continue with the next step with the overall same logic but different details.

Does the code look like being able to get refactored to (or towards) some usefully applicable here design pattern?

Would love to hear any feedback on it.

Here is the code:

/**
 * Check if the given email exists in the SendGrid recipients global list
 * and its custom field 'status' has the value 'subscribed'.
 *
 * @param  string  $email The email to check.
 *
 * @return object  (object)['isfound'=>false, 'issubscribed'=>false];
 */
public function getSubscriberStatus(string $email): object
{
    $result = (object) ['isfound' => null, 'issubscribed' => null];

    /**
     * Find the email in the SendGrid global list.
     */
    $endpoint = "contactdb/recipients/search?email=$email";
    $found = $this->callSendGrid('GET', $endpoint);
    if ($found->status !== 200) {
        Log::error(sprintf('[SENDGRID] Error while searching the email: %s in the SendGrid list, status: %s, message: %s', $email, $found->status, $found->message));
        $result->isfound = false;
        $result->issubscribed = false;
        return $result;

        // throw new \Exception("Error while searching the email: $email in the SendGrid list");
    }

    if (!($found->data->recipient_count > 0)) {
        $result->isfound = false;
        $result->issubscribed = false;
        return $result;
    }

    /**
     * Find the recipient with email exactly matching the required one.
     */
    $recipient = collect($found->data->recipients)->first(function ($item) use ($email) {
        return $item->email === $email;
    });

    /**
     * No exactly matching emails.
     */
    if (!$recipient) {
        $result->isfound = false;
        $result->issubscribed = false;
        return $result;
    }

    $result->isfound = true;

    /**
     * Get the status field of the recipient's 'custom_fields' array.
     */
    $status = collect($recipient->custom_fields)->first(function ($item) {
        return $item->name === 'status';
    });

    if ($status->value !== 'subscribed') {
        $result->issubscribed = false;
        return $result;
    }

    $result->issubscribed = true;
    return $result;
}

I share your concern regarding the code presented to contain repetitions.

Design patterns are something to notice and use on an architectural level and do not apply here.

Suggestion:
• use in-line-comments for one-liners

(I kept several things the way you coded them (and, while not objecting strongly, I'd rather not): one guideline with say meaning code:
Say what you mean, mean what you say. (Scott D. Meyers(?)))
The @return doc comment will need improving, some lines remain overly long:

/**
 * Check if the given email exists in the SendGrid recipients global list
 * and its custom field 'status' has the value 'subscribed'.
 *
 * @param  string  $email The email to check.
 *
 * @return object  (object)['isfound'=>false, 'issubscribed'=>false];
 */
public function getSubscriberStatus(string $email): object
{
    $result = (object) ['isfound' => false, 'issubscribed' => false];

    // Find the email in the SendGrid global list.
    $endpoint = "contactdb/recipients/search?email=$email";
    $found = $this->callSendGrid('GET', $endpoint);
    if ($found->status !== 200) {
        Log::error(sprintf('[SENDGRID] Error while searching the email: %s in the SendGrid list, status: %s, message: %s', $email, $found->status, $found->message));
        return $result;

        // throw new \Exception("Error while searching the email: $email in the SendGrid list");
    }

    if (!($found->data->recipient_count > 0)) {
        return $result;
    }

    // Find the recipient with email exactly matching the required one.
    $recipient = collect($found->data->recipients)->first(function ($item) use ($email) {
            return $item->email === $email;
        });

    // No exactly matching emails.
    if (!$recipient) {
        return $result;
    }

    $result->isfound = true;

    // Get the status field of the recipient's 'custom_fields' array.
    $status = collect($recipient->custom_fields)->first(function ($item) {
            return $item->name === 'status';
        });

    if ($status->value === 'subscribed') {
        $result->issubscribed = true;
    }

    return $result;
}


Prime number sieve in Standard ML As practice, I have written a prime number sieve in Standard ML.

It works in a pretty straightforward way:

The list of known primes starts with just 2.
Starting with 3, check whether each odd integer is relatively prime to all numbers in the list of known primes (i.e. is not divisible by them)

If it's relatively prime, add it to the list of known primes.
If it's not, check the next odd integer.

Repeat step 2 until the list has however many primes we're looking for.
Reverse the list so that the primes are in ascending order.

Here's my code:

fun primes 0 = []
  | primes n =
    let
      fun relativelyPrime x = List.all (fn p => (x mod p <> 0))
      fun buildPrimes ps x =
          if length ps >= n then ps
          else if relativelyPrime x ps then buildPrimes (x::ps) (x + 2)
          else buildPrimes ps (x + 2)
    in
      rev (buildPrimes [2] 3)
    end

I'm still fairly new to Standard ML, so I doubt the above code is optimal or entirely idiomatic. Are there any improvements I could have made if I knew more about the language?

(Note: I realize that negative numbers will return [2]. I could raise an exception, but that's not the point of this question.)

fun buildPrimes ps x =
          if length ps >= n then ps

Don't forget that length is expensive. It's definitely worth using an extra accumulator variable to track either the length or the number of primes left to find. I haven't done proper benchmarks, but it seems to give a speedup of a factor of 2 for input 10000.

I understand why you prepend newly found primes to the list and then reverse it at the end: appending the prime to the end of the list is expensive. But the other side of the argument is that the smaller primes are far more valuable for eliminating composite numbers, and in my testing replacing (x::ps) with (ps @ [x]) and ditching the rev makes a significant improvement to speed.

If performance is a high priority then you could take a hybrid approach which gives you the best of both worlds at the cost of complicating the code: have one list which is sorted in increasing order and another list which is sorted in decreasing order. Use additional accumulator variables to track their lengths. Add new primes to the front of the decreasing list, and when the decreasing list is as long as the increasing list replace them with asc @ (rev desc) and [].

My SML isn't at a level where I feel qualified to say what is idiomatic and what isn't, but one of the most advanced things I studied in SML at university was lazy infinite lists, and I think they have a claim to be fairly idiomatic in functional languages in general. It's not too hard to rework this sieve into a lazy infinite style. We define a type for lazy infinite lists of integers:

datatype stream = Cons of int * (unit -> stream);

where unit is the tuple of length 0, (); then a lazy list of all integers can be built with

fun integers n = Cons(n, fn () => integers (n + 1));

Filtering an infinite list is useful enough to define it outside the prime generator:

fun filter f (Cons(x,xs)) =
    if f(x) then Cons(x, fn () => filter f (xs()))
    else filter f (xs());

And then the sieve itself just builds up layers of filters:

val primes =
    let
        fun filter_multiples m = filter (fn i => i mod m <> 0)
        fun prime_extractor (Cons(p,xs)) = Cons(p, fn () => prime_extractor (filter_multiples p (xs())))
    in
        prime_extractor (integers 2)
    end;

For testing purposes, converting the first n elements to a normal list is convenient:

fun take 0 (Cons(x, xs)) = []
  | take n (Cons(x, xs)) = x :: (take (n - 1) (xs()));

For production quality this should probably be wrapped in a module, but that's at the level that I've heard of it but don't know any details.


Get current opened file version FileVersionInfo.GetVersionInfo(typeof(Program).Assembly.CodeBase.Replace("file:///", "")).ProductVersion

That code, is a one-liner, but has quite a lot of steps for it to return the File Version. Is there maybe a AppDomain way to get the File Version? (Or in general a shorter way)

(The code DOES work - I dont know why its being voted to close as "Broken Code")

This one line might be left as-is, it's short and simple enough to be fully understood at first sight (especially if outer method has a meaningful name). However it has the disadvantage to be hard to test because it is doing 3 things:

Getting the assembly which is the entry point of your application.
Resolving the full path of that assembly.
Reading the version information of that assembly.

In an ideal world I'd probably want to test them in isolation but it might happen only when I will have a bug there, for sure I want to reuse this code elsewhere then I will introduce separate methods.

public static FileVersionInfo GetProductVersion()
    => FileVersionInfo.GetVersionInfo(GetEntryAssemblyPath()).ProductVersion;

public static string GetEntryAssemblyPath()
    => GetAssemblyPath(Assembly.GetEntryAssembly());

I changed typeof(Program).Assembly to Assembly.GetEntryAssembly() because if I move it into a separate DLL then I won't have Program. Note, however, that they may differ (see later).

What I'd really change is the way you obtain the path. Hard-coding file:// is tricky and unnecessary and you do not handle URL escapes. Moreover it will return an incorrect result if assembly has been loaded with Load(). It's easier to write a slightly more correct version:

public static string GetAssemblyPath(Assembly assembly)
     => Uri.UnescapeDataString(new UriBuilder(assembly.Location).Path);

Note that here I'm accessing Assembly.Location but it throws an exception for dynamically generated assembly and it's an empty string for assemblies loaded with Load(). A "better" version should then handle both cases:

public static string GetAssemblyPath(Assembly assembly)
{
    if (assembly.IsDynamic || assembly.Location == String.Empty)
        return ""

     return Uri.UnescapeDataString(new UriBuilder(assembly.Location).Path);
}

Little bit better but if we want to make GetEntryAssemblyPath() a reusable method we should do better because the entry assembly is not always defined (web applications come to my mind in this moment but I kind-of remember about other issues in hosted environments). The choice between Assembly.CodeBase and Assembly.Location isn't important here but in another context you should check for Assembly.Location == String.Empty also when using Assembly.CodeBase.

In your tests you can now verify GetAssemblyPath() in isolation and you may add separate tests for other functions when you will find an issue there (or when their code will be more complicate).

Last pedantic note: we're not checking if file effectively contains the required information and, usually, for assemblies it's also possible to read ProductInformationalVersionAttribute (which might be different).


Preventing invalid entity state that contains two related collections I need to define an entity (CompositeContactList) that contains and manages two collections of identifiers
(IncludedLists and ExcludedLists) that are ids of some other entities (ContactList).

The business rules are the following:
1. The collections must contain only unique identifiers.
2. The same identifier cannot belong to both IncludedLists and ExcludedLists.
3. Identifiers can be added or removed (performing the checks above).

Note: Adding an identifier to the ExcludedLists is not opposite to removing it from the IncludedLists.
For instance, there is a list A that contains included lists B and C and excluded list D.
That means that A = (B + C) - D.
An attempt to add list C to the ExcludedLists leads to an error while C is in the IncludedLists.
If the list C is removed from the IncludedLists, the list A will be the following: A = B - D.

I introduced two IEnumerable<Guid> properties to get the collections and four methods to manage them.
There are few areas I'd like to hear some feedback about. Which variant is better?

Property naming

IncludedLists and ExcludedLists
IncludedListIds and ExcludedListIds

Method naming:

IncludeList(id)
RemoveFromIncludes(id)
ExcludeList(id)
RemoveFromExcludes(id)

AddIncludedList(id)
RemoveIncludedList(id)
AddExcludedList(id)
RemoveExcludedList(id)

AddIncludedListId(id)
RemoveIncludedListId(id)
AddExcludedListId(id)
RemoveExcludedListId(id)

Validation

Rule 1. When I add the same identifier twice, does it may sense to throw an exception saying that the id
cannot be added? Is there any harm in 'doing nothing' here?
Rule 2. Trying to add a list id to the included lists throws an error if it's already belongs to the excluded.
Does it make sense to introduce some helping methods like CanAddToIncludedLists or TryAddToIncludedLists?
Does it make sense to throw an ArgumentException if an empty Guid is provided?

Usability

Does it make sense in declaring 'Add' methods as void and 'Remove' methods returning bool?
(similar to .NET collections).
Does it make sense in adding other collection-specific methods like ClearIncludedLists or ClearAll?

Code

public class MyCompositeEntity
{
    private readonly ISet<Guid> _excludedLists = new HashSet<Guid>();
    private readonly ISet<Guid> _includedLists = new HashSet<Guid>();

    public MyCompositeEntity(Guid id)
    {
        if (id == Guid.Empty)
            throw new ArgumentException("The identifier cannot be empty.", nameof(id));

        Id = id;
    }

    public Guid Id { get; }

    public IEnumerable<Guid> IncludedLists => _includedLists;

    public IEnumerable<Guid> ExcludedLists => _excludedLists;

    public void IncludeList(Guid id)
    {
        if (id == Guid.Empty)
            throw new ArgumentException("The identifier cannot be empty.", nameof(id));

        if (ExcludedLists.Contains(id))
            throw new ArgumentException(
                "The identifier cannot be included because it is already excluded. " +
                "Please remove it from the excluded lists first.",
                nameof(id));

        _includedLists.Add(id);
    }

    public void ExcludeList(Guid id)
    {
        if (id == Guid.Empty)
            throw new ArgumentException("The identifier cannot be empty.", nameof(id));

        if (IncludedLists.Contains(id))
            throw new ArgumentException(
                "The identifier cannot be excluded because it is already included. " +
                "Please remove it from the included lists first.",
                nameof(id));

        _excludedLists.Add(id);
    }

    public bool RemoveFromIncludes(Guid id)
    {
        if (id == Guid.Empty)
            throw new ArgumentException("The identifier cannot be empty.", nameof(id));

        return _includedLists.Remove(id);
    }

    public bool RemoveFromExcludes(Guid id)
    {
        if (id == Guid.Empty)
            throw new ArgumentException("The identifier cannot be empty.", nameof(id));

        return _excludedLists.Remove(id);
    }
}

I would give these the same signature

public void IncludeList(Guid id)

public bool RemoveFromIncludes(Guid id)

And rename

public bool AddToIncludes(Guid id)
{
    ...
    return _includedLists.Add(id);
}

Another approach is

Dictionary<GUID, bool>

and just one list


Digital Clock Script While learning JavaScript I thought it was a good idea to make a digital clock program. I followed a youtube tutorial (with some tweaks to my preference) and came up with my final code that I have. I came to CodeReview to get the script reviewed, and to ask is there anything wrong with the script and/or any way to improve it as a whole. Ways to make the script shorter (yet also high in performance) are also accepted. I am also curious on if I have any 'red flags' currently. I will be giving the best answers the green checkmark. Thanks!

function showTime() {
  const date = new Date();
  let hours = date.getHours();
  let minutes = date.getMinutes();
  let seconds = date.getSeconds();
  let session = (hours < 12) ? "AM" : "PM";

  if (hours === 0) {
    hours = 12;
  } else if (hours > 12) {
    hours -= 12;
  }
  hours = (hours < 10) ? '0' + hours : hours;
  minutes = (minutes < 10) ? '0' + minutes : minutes;
  seconds = (seconds < 10) ? '0' + seconds : seconds;

  const time = `${hours} : ${minutes} : ${seconds} ${session}`;
  document.querySelector('div').innerHTML = time;

  setTimeout(showTime,1000)
}

showTime()
<!DOCTYPE html>

      <title>Digital Clock</title>

TL;DR: I am a JavaScript programmer, and CSS currently isn't my suite, therefore I know that the clock can use some styling. How can the JavaScript code be improved?

1
You are repeating condition ? '0' + integer : integer 3 times.
This can be reduced with const zeroPad = (num, places) => String(num).padStart(places, '0')
and doing
  hours = zeroPad(hours, 2);
  minutes = zeroPad(minutes, 2);
  seconds = zeroPad(seconds, 2);

The 2 is now a repeated magic number and you may wish to store it in a variable with a descriptive name.
2

Usually, there are no spaces around the equals sign:

3
I'm not familiar with JavaScript, but I would replace the nested setTimeout:
function showTime() {
  // ...
  setTimeout(showTime, 1000)
}

with a setInterval:
setInterval(showTime, 1000);
I would also place a space after the comma.
4
For formatting the time, take a look at this answer which suggests using:
hours = hours % 12;
hours = hours ? hours : 12;

5
You use document.querySelector('div') but this "returns the first Element within the document that matches the specified selector".
I would suggest using

 and document.getElementById


Building navigation structure for multiple users by adding values to multiple arrays at once I have a multidimensional array with associative values... I want to add some information to multiple arrays at the same time. Doing $array1=$array2=array(... seems to work but I'm worried that it's too easy. Will any array get extra data or have data overwritten by doing this?
What kind of unexpected behavior might I expect from this kind of code? Al should have 3 entries in it, while Bob and Chris will each have two... right? It seems to work, but I'm worried I'm overlooking something.

<?    $content['Al'][]= array(
       "link"=>"/admin.php",
       "text"=>"Admin Login",
       "icon"=>"wp.png");
    $content['Al'][]=$content['Bob'][]=$content['Chris'][] = array(
       "link"=>"/Wordpress/wp-login.php",
       "text"=>"Login to Wordpress",
       "icon"=>"wp.png");
    $content['Al'][]=$content['Bob'][]=$content['Chris'][] = array(
       "link"=>"/Gallery/Login.php",
       "text"=>"Login to Gallery",
       "icon"=>"picutres.jpg");

function userbox($heading,$contents){
    if($contents=="") return;
    echo "

".$heading."

";

    foreach ($contents as $key){
        if($key['link']!="")echo "";
        if($key['icon']!="")echo"";
        echo $key['text'];
        if($key['link']!="")echo "";
        if($key['updated']!="")echo " (".$key['updated'][0].")";
        echo "
";
        }
    echo "

";
}

?>

Then in the content of the page I have

<?userbox($user,$content[$user]);?>

This should show a 'refbox' div with the users links in it within the page.

It works and I can't see any side effects, but it is unusual to see code like this, and I am not sure of the point of creating all 3 user arrays at once.

Do you intend to display all 3 user menu options at the same time, or can only 1 user be logged in at once?

This is my preference for readability, also if you create a menu array and need items in a different order for different users, it would be difficult to achieve using the current code.

<?php

function build_menu($user)
{
    // ### clearly show what each test is and what menu options are being set
    $menu = array();

    if ($user == 'Al') {
        $menu[] = array(
            "link" => "/admin.php",
            "text" => "Admin Login",
            "icon" => "wp.png");
    }

    if (in_array($user, array('Bob', 'Chris'))) {
        // menu options for multiple users can use in_array test
    }

    // default menu options for all users
    $menu[] = array(
        "link" => "/Wordpress/wp-login.php",
        "text" => "Login to Wordpress",
        "icon" => "wp.png");

    $menu[] = array(
        "link" => "/Gallery/Login.php",
        "text" => "Login to Gallery",
        "icon" => "picutres.jpg");

    return $menu;
}

function userbox($heading, $contents)
{
    // ### contents is supposed to be an array, why test it for being a blank string?
    // ### use {} to show scope
    if ($contents == "") {
        return;
    }

    // ### escape heading, what if it has html chars
    echo "

" . htmlspecialchars($heading). "

";

    /// ### think about your variable naming, $contents is actually menu items
    /// ### and $key is ambiguous, key of what?
    /// ### what if you used foreach ($menu_items as $menu_item) {

    foreach ($contents as $key) {

        // ### use {} around if statements, it is easier to see the scope of what is effected
        // ### use not empty instead of != "", that way if an array key doesn't exist like the 'updated' key below, your program won't generate a warning
        // ### use single quotes if you don't want to escape every double quote

        // if($key['link']!="")echo "";
        if (!empty($key['link'])) {
            echo '';
        }

        // ### i have given an example above, I will leave these ones for you to change
        if($key['icon']!="")echo"";

        // ### escape html incase there are special characters
        // echo $key['text'];
        echo htmlspecialchars($key['text']);

        if($key['link']!="")echo "";
        if($key['updated']!="")echo " (".$key['updated'][0].")";
        echo "
";
    }
    echo "

";
}

$user = 'Al';
$menu = build_menu($user);
userbox($user, $menu);


NAddOdd - Topcoder - Single Round Match 714 Round 1 - Division I, Level Two I am interested in DP problems from the past couple of days and am trying to solve various problems on topcoder. To be very frank could not even solve a single one till now. I went through https://community.topcoder.com/stat?c=problem_statement&pm=14408 question and thought to try this. I could come up with a recursive solution easily but now I am struggling for a DP solution. The following is the best I came up with:

package testpackage;

import java.util.HashMap;
import java.util.Map;

public class NAddOdd {

    public static Map<Long, Long> map = new HashMap<Long, Long>();

    public static void main(String[] args) throws Exception{
        System.out.println(solve(1, 1000, 3));
    }

    public static long solve(long L, long R, int K) throws InterruptedException{
        long answer=0l;
        for(long l = R;l>=L;l--){
            answer+=solveInter(l,K);
        }
        return answer;
    }
    public static long solveInter(long h, long K) throws InterruptedException{
        if(map.get(h) != null){
            return map.get(h);
        }
        if(K>=h){
            if(map.get(h) == null)
                map.put(h, 0l);
            return 0l;}
        if(h%2 == 0){
            if(map.get(h) == null)
                map.put(h, 1+solveInter(h/2, K));
            return map.get(h);
        }
        else{
            h = h+K;
            if(map.get(h) == null)
                map.put(h, 1+ solveInter(h, K));
            return map.get(h);
        }

    }

}

But this takes time on bigger values. Also I know memory would fails on large long values. Any clues to perfect this one?

Thanks.

First the minor things about your coding style:

0l should be 0L to avoid confusing it with 01
ALWAYS use brackets after each if/for/while/...
variables are lowerCamelCase by convention. You don't need to copy the question literally. Using single capital letters usually means it's a generic type parameter. It does make sense to use l, r and k here instead of longer more readable names though, since the method is directly related to the given question.
throws InteruptedException can be removed since it's never thrown.
if(map.get(h)==null) is always true after the initial != null check. No need to check that again.
putting something in a map to then taking it out again right after seems silly. Just store it in a temp variable.

There are 2 small optimisations that reduce entries in the map without giving up speed.

The first optimisation is to first check if h is smaller than k and return 0 without storing it.

public static long solveInter(long h, long k) {
    if(h <= k){
        return 0L;
    }
    if(map.get(h) != null){
        return map.get(h);
    }

The second is for odd numbers to also do the following "even" step at the same time like this: map.put(h, 2 + solveInter((h+k)/2, k));

That way, no number larger than R is ever stored.

After a bit of testing it seems a lot more numbers don't need to be stored as well. For example running

public static void main(String[] args) throws Exception{
    System.out.println("total: "+ solve(1000000L, 1001000L, 1001));
    System.out.println("stored entries: " +map.size());
}

shows that this change alone reduces the entries from 15941 to 10962 and this for the total: 19478 steps in the solution.

That's about as much as I can save on your solution without making compromises. If you don't mind giving up a bit of speed to further reduce the number of stored results (to solve the memory issues) you can for example not store any even numbers. This changes the computation from this:

    if(h%2 == 0){
        int result = 1 + solveInter(h / 2, k);
        map.put(h, result);
        return result;
    }

    int result = 2 + solveInter((h + k) / 2L, k);
    map.put(h, result);
    return result;

into this:

    int result = 0;
    while(h%2 == 0){
        if(h <= k) {
            return result;
        }
        result ++;
        h /= 2;
    }

    result = result + 2 + solveInter((h + k) / 2L, k);
    map.put(h, result);
    return result;

This then get's the stored entries in the previous example down to 5730.

I didn't run this on the 4th example
(System.out.println("total: " + solve(1645805087361625L, 9060129311830846L, 74935));)
because those numbers are huge and will take a long time even if you would have a lookup table for all the solveInter results.

I have no idea about how many entries you can store in the map. I did notice that the chains are usually really short. Even for huge numbers like:

System.out.println("total: "+ solve(Long.MAX_VALUE-1, Long.MAX_VALUE-1, 1));

the total is only 66. This is because number goes down logarithmicaly. Even if you have a lot of odd numbers the total will only be 2 or 3 times as large. So to reduce the memory requirements further you could change the map to Map<Long, Short> (32000 will certainly be big enough to store the length of the chain).

Perhaps someone more math minded can come up with some more speed improvements by reducing the required calculations :)


Class made to do some basic RGB analysis I created this class to load in an image and get some specific values I needed.

from PIL import Image
import pandas as pd

class Img:
    def __init__(self,filename):
        self.name = filename
        self.image = Image.open(self.name) # open image using PIL

        # convert the image to a pandas df
        self.df = pd.DataFrame(data=list(self.image.getdata()))

        # get the RGB values from the image
        self.rList = self.df[0]
        self.gList = self.df[1]
        self.bList = self.df[2]

        # get the mean RGB values from the image
        def getMean(self,rgb):
            return rgb.mean()

        self.r = getMean(self,self.rList)
        self.g = getMean(self,self.gList)
        self.b = getMean(self,self.bList)

        # calculate luminance(brightness) based on the RGB values
        def lumi(self):
            l = (0.299*self.rList + 0.587*self.gList + 0.114*self.bList)
            return l[l!=0]

        self.l = lumi(self)

        # calculate the contrast using the Michelson contrast formula.
        def con(self):
            min = self.l.quantile(0.05)
            max = self.l.quantile(0.95)
            return ((max-min) / (max + min))

        self.contrast = con(self)

My goal: With the class structure created, I intend to create some instances into an list and plot values.

def loadImages(folder):
    images = []
    for image in os.listdir(folder+'/'):
        if image.endswith('.jpg'):
            name = folder+'/'+image
            images.append(Img(name))
    return images

images = loadImages('faces')

from matplotlib import pyplot as plt

plt.scatter([x for x in range(0,len(images))],[x.b for x in images])

All those inner functions in your constructor are unnecessary. Inner functions should be used sparingly, normally you have only these cases:

Functions which can do something based on parameters you pass to it and which are not directly related to some object. Make these functions stand-alone functions.
Functions which do something with an object or return some information about that object. Make these functions methods (or properties) of the class of the object.
Functions which logically belong to some class because they make only sense in combination with them. Make these functions static methods of the class (by adding the decorator @staticmethod).

In your case I would do Option 2 (with the addition of making them properties which will be calculated when you first access them).

class Img:
    def __init__(self,filename):
        self.name = filename
        self.image = Image.open(self.name) # open image using PIL

        # convert the image to a pandas df
        self.df = pd.DataFrame(data=list(self.image.getdata()))

        # get the RGB values from the image
        self.r = self.df[0].mean()
        self.g = self.df[1].mean()
        self.b = self.df[2].mean()

        self._luminance = None
        self._contrast = None

    @property
    def luminance(self):
       """Calculate luminance (brightness) based on the RGB values"""
        if self._luminance is None:
            self._luminance = (0.299*self.r + 0.587*self.g + 0.114*self.b)
        return self._luminance

    @property
    def contrast(self):
        """Calculate the contrast using the Michelson contrast formula."""
        if self._contrast is None:
            min_ = self.luminance.quantile(0.05)
            max_ = self.luminance.quantile(0.95)
            self._contrast = (max_ - min_) / (max_ + min_)
        return self._contrast


Combine a set of “ranges” to find least number of ranges Given a set of ranges with lower bound and upper bound, I need to combine these ranges to produce minimum number of ranges which will cover all the values in the original set and nothing else.

Example 1:

Input:

[100, 200], [120-170],[210 - 230]

Output:

[100-200], [210-230]

Example 2:

Input:

[100, 200], [120-210],[210 - 230]

Output:

[100-230]

This is what I have so far. This program works as expected and prints out the results as expected. But I'm wondering if there is any better way to do it.

public class Range {

        public static void main(String[] args) {

            int[][] Ranges1 = new int[][] {     new int[] { 94133, 94133 },
                                                new int[] { 94226, 94399 },
                                                new int[] { 94200, 94299 }

                                            };

            Range Range = new Range();

            int[][] normalizedRanges = Range.getTheNormalizedRanges(Ranges1);

            System.out.println("Normalized range:");
            Range.print(normalizedRanges);

        }

        private int[][]  getTheNormalizedRanges(int[][] RangesToNormalize) {

            int[][] newrange = new int[RangesToNormalize.length][];

            boolean[] rangePurged = new boolean[RangesToNormalize.length];

            int newIndex = 0;

            this.sortRange(RangesToNormalize);

                      for (int i = 0; i < RangesToNormalize.length; i++) {

                if (rangePurged[i])
                    continue;

                if (i == (RangesToNormalize.length - 1) ){
                    newrange[newIndex] = RangesToNormalize[i];
                    break;
                }

                int min = RangesToNormalize[i][0];
                int max = RangesToNormalize[i][1];

                int nextMin = RangesToNormalize[i + 1][0];
                int nextMax = RangesToNormalize[i + 1][1];

                if (max >= nextMin) {

                    if (max >= nextMax)
                        newrange[newIndex] = new int[] { min, max };
                    else
                        newrange[newIndex] = new int[] { min, nextMax };

                    //keep track of the ranges that are purged from the original list.

                    rangePurged[i+1] = true;

                } else {

                    //if there is no overlap, add the current range to the new list

                    newrange[newIndex] = RangesToNormalize[i];
                }

                newIndex++;

            }

            return newrange;

        }

        private void print(int[][] printRange){

            if(printRange == null || printRange.length <=0)
                throw new IllegalArgumentException("printRange is empty or null");

            for (int i = 0; i < printRange.length; i++) {

                if (printRange[i] != null)
                {

                    System.out.println("["+ printRange[i][0] + ", "
                            + printRange[i][1]+"]");
                }
            }
        }

        private void sortRange(int[][] rangeToSort){

            Arrays.sort(rangeToSort, new Comparator<int[]>() {

                @Override
                public int compare(int[] range1, int[] range2) {

                    int range1Min = range1[0];
                    int range2Min = range2[0];

                    if (range1Min > range2Min)
                        return 1;

                    if (range1Min < range2Min)
                        return -1;

                    return 0;
                }

            });
        }
    }

The program is NOT working as expected. You can see the bug if you give it the following input

    int[][] zipRanges1 = new int[][] {
            new int[] { 1, 5 },
            new int[] { 2, 6 },
            new int[] { 3, 4 }
    };

The problem is that you compare ranges from the input only. The correct logic is that for each input range, you should iterate over newZipRanges and "insert" the current input in the correct place. You sohuld make the Logic to normalize the zipcode range piece of code into a method that compares and possibly merges two ranges.

However, the story does not end here. after the above processing is done (after you finish iterating over the input), you will need to iterate over the newZipRanges and normalize it. consider the following case:

newZipRanges contains {1, 4}, {7, 9}
and now the input range you're processing is {1, 8} you will merge it into the first new range and produce newZipRanges == {1, 8}, {7, 9} which obviously needs normalizing.  I think after any modification to newZipRanges (after merging an input range into it) you need to iterate over the newZipRanges and see if any merging needs to be done (perhaps sorting every time? not sure)

one last advice, to merge overlapping ranges, use Math.min on the lower bounds and Math.max on the upper bounds. its nicer than the if question you have.


Simple proof-of-work (based on HashCash) I'm trying to build a simple (yet effective) brute-force protection for my App-API.
The client requests an identity - the server supplies a random string (called seed, which it remembers via PHP  session) and a security level.
If the client wants to login it needs to supply a suffix for the seed which results in a SHA512 hash with leading zero-bytes (security level times).
Java (generation):
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;

public class HashCash {
    private final static String HASH_ALGORITHM = "SHA-256";

    private final MessageDigest hashGenerator;

    /**
     * Creates a new HashCash generator using the SHA-256 algorithm
     * @throws NoSuchAlgorithmException If the "SHA-256" algorithm is not available
     */
    public HashCash() throws NoSuchAlgorithmException {
        hashGenerator = MessageDigest.getInstance(HASH_ALGORITHM);
    }

    /**
     * Generates a HashCash for the specified level
     * @param seed A seed for the HashCash
     * @param targetLevel The target level for the HashCash
     * @return The generated HashCash
     */
    public long generateHashCash(final String seed, final int targetLevel) {
        // Variation counter (and final result)
        long counter = 0;
        while(true) {
            // Check if the current hashCash reaches the desired security level
            if(getSecurityLevel(seed + counter) >= targetLevel)
                return counter;
            // Increase hash counter
            counter++;
        }
    }

    /**
     * Computes the security level of the given value
     * @param value The value to compute the level of
     * @return The security level of the value
     */
    private final int getSecurityLevel(final String value) {
        hashGenerator.update(value.getBytes());
        final byte[] hash = hashGenerator.digest();
        int level = 0;
        for(final byte b: hash) {
            if(b != 0)
                break;
            level++;
        }
        return level;
    }
}

PHP (verification):
const HASH_CASH_SEC_LEVEL = 8;

/**
 * Verifies a HashCash suffix (by checking it's security level)
 * @param string $seed The seed of the identity
 * @param string $suffix The HashCash suffix
 * @return bool Whether the given HashCash suffix is valid
 */
function verifyHashCash(string $seed, string $suffix): bool {
    $hash = hash('sha256', $seed.$suffix, true);
    $level = 0;
    $length = strlen($hash);
    for($b = 0; $b < $length; $b++) {
        if(ord($hash[$b]) !== 0)
            break;
        $level++;
    }
    return $level >= HASH_CASH_SEC_LEVEL;
}

Are there any security related issues?
Are there performance optimizations for the Java code?

public HashCash() throws NoSuchAlgorithmException {

SHA-256 is a requirement for any Java SE environment. Instead of throwing you should be catching and converting to a RuntimeException (or IllegalStateException) with this particular exception as cause.
public long generateHashCash(final String seed, final int targetLevel) {
...
private final int getSecurityLevel(final String value) {

If you're going to design for performance you should not be using String types. The seed should be a byte array from the start. And you could use e.g. ByteBuffer to write an integer to an existing array that already starts with the seed.
Note that SHA-256 is a Merkle-Damgard hash function that handles input block-by-block. That means that if you start off with a large enough seed (512 bits or larger) you might as well pre-hash it. Of course this means mucking about within the hash implementation.
if(b != 0)
    break;
level++;

Generally the level calculation is performed on bits or even values (where the hash value, when seen as a unsigned number, is below a certain value).
As for performance: you can stop comparing whenever you encounter a number that doesn't have the right level. It won't matter much when compared the time required to perform SHA-256 in all probability, but it is still wasteful.
I don't expect getSecurityLevel to perform the actual hash calculation. A getter is generally not expected to perform any work at all.
The PHP code doesn't seem to reflect the structure of the Java application. If you have one getSecurityLevel method I would expect the same in the verification application, whichever language is used.

Otherwise, yes, it is a very simple proof of work. I'd rather require a few hashes instead of one, to average out the amount of work performed. With just one hash there will be a lot of luck involved, and the actual work will therefore vary rather wildly.
In the unlikely case that you've invented yet another popular PoW: I'd use 128 bits rather than a 64 bit long. It's pretty easy to make a counter that operates on bytes directly, so you would not even have to convert to the binary values required for the hashing.


TextWriterTraceListener class I need to extend the default TextWriterTraceListener class to include timestamps with each message on same line and file rotation. I can't seem to find any way of doing it with TraceListener configuration.

Can you please review it and let me know any flaws to be used in a multi-threaded application?

using System;
using System.Collections.Generic;
using System.IO;
using System.Diagnostics;
using System.Text;
namespace ConsoleApplication1
{
    public class MyTextWriterTraceListener : TextWriterTraceListener
    {
        public MyTextWriterTraceListener(System.IO.Stream stream) : base(stream) { }
        public MyTextWriterTraceListener(System.IO.Stream stream, string name) : base(stream, name) { }
        public MyTextWriterTraceListener(string FileName, string name) : base(FileName, name) { RotateLogFiles(FileName); }
        public MyTextWriterTraceListener(string FileName) : base(FileName) { RotateLogFiles(FileName); }
        public MyTextWriterTraceListener(System.IO.TextWriter writer, string name) : base(writer, name) { }
        public MyTextWriterTraceListener(System.IO.TextWriter writer) : base(writer) { }
        public override void Write(string Msg)
        {
            base.Write(Msg);
            base.Flush();
        }
        public override void WriteLine(string Msg)
        {
            base.WriteLine(LinePrefix() + Msg);
            base.Flush();
        }

        private void RotateLogFiles(string FileName)
        {
            FileInfo TheFileInfo = new FileInfo(FileName);
            if (TheFileInfo.Exists == false)
                return;
            if (TheFileInfo.LastWriteTime.Date < DateTime.Today)
            {
                TheFileInfo.MoveTo(string.Format(@"{0}\{1}_{2}{}", TheFileInfo.DirectoryName, TheFileInfo.Name, TheFileInfo.LastWriteTime.ToString(), TheFileInfo.Extension));
            }
        }

        [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)]
        private string LinePrefix()
        {
            DateTime Now = DateTime.Now;
            return string.Format("{0} : ", Now.ToString("dd-MMM-yyyy HH:mm:ss"));
        }
    }
}

Possible issues

1 Follow a style guideline. It is common convention to camel case local variable names.

var theFile = new FileInfo(...)  instead of  var TheFile = new FileInfo(...).

Format your file equally (i.e. either all methods are separated by a blank line or none).

2 Keep lines short and use more variables to get better readability and easier debugging:

var newPath = Path.Combine(file.DirectoryName, file.Name, file.LastWriteTime.ToString(), file.Extension)
file.MoveTo(newPath);

instead of

TheFileInfo.MoveTo(string.Format(@"{0}\{1}_{2}{}", TheFileInfo.DirectoryName, TheFileInfo.Name, TheFileInfo.LastWriteTime.ToString(), TheFileInfo.Extension));

3 Use meaningful names.

RollingTextWriterTraceListener instead of MyTextWriterTraceListener

[MethodImpl(MethodImplOptions.NoInlining)] instead of [System[...]NoInlining)]

What do you do if you need an other implementation - MyOtherTextWriterTraceListener?
The same applies to TheFileInfo, ConsoleApplication1.

4 Remove redundant qualifiers to remove clutter.

public MyTextWriterTraceListener(Stream stream) : base(stream) { }

instead of

public MyTextWriterTraceListener(System.IO.Stream stream) : base(stream) { }

var file = new FileInfo(...) instead FileInfo file = new FileInfo(...)

5 Are you sure you need the [MethodImpl(MethodImplOptions.NoInlining)] attribute?

What I like

1 if (TheFileInfo.Exists == false) return;

because the return statement reduces nesting (see arrow head anti pattern) and == false because its more obvious and than !TheFileInfo.Exists. A lonely ! can quick be overseen. I would recommend the same procedure for the following if statement, too:

if (TheFileInfo.LastWriteTime.Date >= DateTime.Today) return;

instead of

if (TheFileInfo.LastWriteTime.Date < DateTime.Today) { ... }

General recommendations

1 As k3b mentioned I would recommend using log4net, too. It has more flexibility you will ever need and you can configure it via config file which means you can change your logging output without recompiling.


MySQLi query and error handling for select, insert, and update I am creating a new webpage with database connection. I want to make sure that I am handling the MySQLi query correctly for Search, Insert, and Update. I also want to make sure if any errors do pop up, that the function will catch the error for type of query I am running.

Is the if (!$Results) the correct way to test for Error/Failure on both Select and Insert/Update?

I am planning on having another section of functions for each table in my database for Select, Insert/Update.

function DB_Select($Query, $Results, $Rows) {
    DB_Test($DB_Connection);
    $Results = mysqli_query($DB_Connection, $Query);
    if (!$Results) {
        $Message = 'Failed Search: ' . mysqli_error();
        include $NAV_DB_Failure;
        exit();
    } else {
        $Rows = mysqli_affected_rows($Results);
    }
}

function DB_Insert_Update($Query, $Results, $Rows) {
    DB_Test($DB_Connection);
    $Results = mysqli_query($DB_Connection, $Query);
    if (!$Results) {
        $Message = 'Failed Insert/Update: ' . mysqli_error();
        include $NAV_DB_Failure;
        exit();
    } else {
        $Rows = mysqli_affected_rows($Results);
    }
}

Sorry to say, but you are doing it all wrong, from getting a mysqli connection to your ideas on the error reporting. I've got an article on this latter topic, Error reporting basics but it is rather long, so here is a gist:

On a live site, not a single word from the error message should be shown to a site user. Because:

for the casual user it is pretty cryptic and confusing.
it it essentially useless for the site programmer/admin as they likely won't be browsing the site at the moment and will have no idea that error has been occurred.
the information provided in the error message extremely useful for a potential hacker, as it provides them with the feedback for their actions and may leak some sensitive information on the site.

Instead, the error message should be logged for the future reference, whereas just a generic error page should be shown.

From this you can tell that your idea of showing the complete error message to the site user is essentially flawed. Instead of manually handling every single error message, you should just let it go, and then create a handler that will dispatch the error message according to the current mode.
The idea of adding the query type is rather useless as well. Given PHP will always provide you with the file name and line number where the error occurred, you can always find the problem query and thus learn its type.
Now, to the code review.
First, I would put mysqli into Exception mode, to make it report all errors automatically. To do so, I will add the following line to mysqli connection code:
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);

So, given you are using a function to get the mysqli connection, it could be written this way
function DB_Connect {
    static $conn;
    if (!$conn) {
         mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
         $conn = mysqli_connect('your credentials');
    }
    return $conn;
}

this function will ensure that we are using only a single connection and also will turn Exception mode for mysqli
Then I would make it only one function,
function DB_query($Query) {
    $DB_Connection = DB_connect();
    $result = mysqli_query($DB_Connection, $Query);
    return $result;
 }

with such a function you'll be able to perform any query and also get any result type you need. For example,
// select query returns an array with rows
$users = DB_Query("SELECT * FROM users")->fetch_all();

// update query returns the number of rows affected
$rows_updated = DB_Query("UPDATE users SET active=1")->num_rows();

However, there is another drawback. In fact, for such a function it is mandatory to support prepared statements. Otherwise it will be wide open to SQL injection. Therefore, we will change it to make it allow variables used in the queries
function DB_query($query, $params = [])
{
    $conn = DB_connect();
    if ($params)
    {
        $stmt = $conn->prepare($query);
        $types = str_repeat('s', count($params));
        $stmt->bind_param($types, ...$params);
        $stmt->execute();
        $result = $stmt->get_result();
    } else {
        $result = mysqli_query($conn, $query);
    }
    if ($result)
    {
        return mysqli_fetch_all($result);
    } else {
        return mysqli_affected_rows($result);
    }
}

This function will return you an array of rows for SELECT and other queries that return any data, and the number of affected rows otherwise. Examples:
$workers = DB_query("SELECT * FROM workers WHERE salary >= ?",[$salary]);

DB_query("INSERT INTO workers SET name=?, salary=?",[$name,$salary]);
$id = DB_connect()->last_insert_id();

As for the $NAV_DB_Failure - create the error handler like this
set_error_handler("myErrorHandler");
function myErrorHandler($errno, $errstr, $errfile, $errline)
{
    error_log("$errstr in $errfile:$errline");
    header('HTTP/1.1 500 Internal Server Error', TRUE, 500);
    readfile("NAV_DB_Failure.html");
    exit;
}

and this file will be shown in case of any error, while keeping the error message for your eyes only.


Implement Transposition cipher in c++ I try to implement the encryption by Columnar transposition.

Input: key_secret and Text_to_encrypt, both of them are in lower.

Output: encrypted_text.

The code works fine but I have some questions:

it warns: "Function 'begin' could not be resolved", Function 'end' could not be resolved, Function 'copy_if' could not be resolved. How can I get rid of it?
it creates mapOriginAlphabet slowly. How can I improve it?

I use gcc 5.3.0

Here the code: .h file

#ifndef TRANSPOSITIONCIPHER_H_
#define TRANSPOSITIONCIPHER_H_

#include <iostream>
#include <string>
#include <algorithm>
#include <map>
#include <set>

#define log(s) std::cout << s << std::endl

class transpositioncipher {
private:
    std::map<char,bool> mapOriginAlphabet;
    std::map<char,char> mapAlphabet;
    std::vector<int> vecOrder;
    std::string mKeySecret;

    std::string removeDuplicates(const std::string strOrigin);
    void createOrder();
    void createSubstitution();

public:
    transpositioncipher(std::string KeySecret);
    virtual ~transpositioncipher();

    std::string encrypt(const std::string strOrigin);
    std::string decrypt(const std::string strOrigin);
    void print();
};

#endif /* TRANSPOSITIONCIPHER_H_ */

.cpp file

#include "transpositioncipher.h"

transpositioncipher::transpositioncipher(std::string KeySecret) {

    mKeySecret = removeDuplicates(KeySecret);
    createOrder();
    for (int i = 97; i < 123; ++i) {
        char c = (char) i;
        std::size_t found = mKeySecret.find(c);
        if (found != std::string::npos) {
            mapOriginAlphabet[c] = false;
        } else {
            mapOriginAlphabet[c] = true;
        }
    }
     createSubstitution();
}

transpositioncipher::~transpositioncipher() {
}

std::string transpositioncipher::removeDuplicates(const std::string strOrigin) {

    std::set<char> moved;
    std::string result;
    result.reserve(strOrigin.size());

    //Only copy one character of each type to the new string
    std::copy_if(std::begin(strOrigin), std::end(strOrigin),
        std::back_inserter(result),
        [&moved](const auto& value) {return moved.insert(value).second;});

     return result;
}

void transpositioncipher::createOrder() {

    std::set<char> chars { mKeySecret.begin(), mKeySecret.end() };

    std::string strTemp = std::string(chars.begin(), chars.end());

    for (auto& x : strTemp) {
        std::size_t found = mKeySecret.find(x);

        vecOrder.push_back(found);
    }

}

void transpositioncipher::createSubstitution() {

    int iCols = mKeySecret.length();
    int iRows = 26 / iCols + 1;

    std::vector<std::vector<char>> vecTemp;

    std::vector<char> vecFirstRow;
    for (auto& x : mKeySecret) {
        vecFirstRow.push_back(x);
    }
    vecTemp.push_back(vecFirstRow);

    int index = 97;
    int iNumRow = 0;

    while (iNumRow < iRows) {
        std::vector<char> row;
        int iNumCol = 0;
        while (iNumCol < iCols) {
            if (mapOriginAlphabet[(char) index]) {
                row.push_back((char) index);
                iNumCol++;
            }
            index++;
            if (index > 123) {
                break;
            }
        }
        vecTemp.push_back(row);
        iNumRow++;
    }

    index = 97;
    for (auto& indexCol: vecOrder) {
        for (auto& vec: vecTemp) {
            if (indexCol >= vec.size())
                continue;
            else {
                mapAlphabet[(char)index] = vec.at(indexCol);
                index ++;
            }
        }
    }
}

std::string transpositioncipher::encrypt(const std::string strOrigin) {
    std::string ret;
    for (auto& x: strOrigin) {
        ret += mapAlphabet[x];
    }

    return ret;
}

I see a number of things that may help you improve your code.

Fix the formatting

The code has an inconsistent format, with varying indentation and brace placement.  To make your code easier to read, you should choose a style and apply it consistently.

Avoid confusing branching constructs

The code currently contains this:

for (auto& vec: vecTemp) {
    if (indexCol >= vec.size())
        continue;
    else {
        mapAlphabet[(char)index] = vec.at(indexCol);
        index++;
    }
}

(with the formatting fixed).  There are several problems with this.  The first is that the control flow is confusing.  It can be made much simpler:

for (auto& vec: vecTemp) {
    if (indexCol < vec.size()) {
        mapAlphabet[(char)index] = vec.at(indexCol);
        index ++;
    }
}

Use appropriate types for variables

The next problem with that code is that it uses an old C-style cast to turn index into a char.  In fact, it's cast to a char just about every time it's used.  Why not just declare it as a char in the first place and remove the casts?

Eliminate "magic numbers"

Instead of using numbers such as 97 and 123 as with this code:

for (int i = 97; i < 123; ++i) {
    char c = (char) i;
    std::size_t found = mKeySecret.find(c);

it would make the code a lot easier to understand if we used the appropriate type of variable and used character constants like this:

for (char c = 'a'; c <= 'z'; ++c) {
    std::size_t found = mKeySecret.find(c);

Use boolean results directly

Rather than this:

std::size_t found = mKeySecret.find(c);
if (found != std::string::npos) {
    mapOriginAlphabet[c] = false;
} else {
    mapOriginAlphabet[c] = true;
}

The code would be much shorter and clearer written like this:

bool found = mKeySecret.find(c) != std::string::npos;
mapOriginAlphabet[c] = !found;

Or possibly this:

mapOriginAlphabet[c] = mKeySecret.find(c) == std::string::npos;

Understand standard template library containers

The code for encrypt contains these lines:

for (auto& x: strOrigin) {
    ret += mapAlphabet[x];
}

This seems to do the right thing, but in fact, it inserts non alphabetic characters into the mapAlphabet which is not the desired behavior here.  Instead, you should use mapAlphabet.at(x) like this:

for (const auto& x: strOrigin) {
    try {
        ret += mapAlphabet.at(x);
    } catch (std::exception &e) {
        // ignore out of range inputs
    }
}

Use const where practical

Performing an encryption or decryption with a given keyword should not alter the underlying transpositioncipher object, so those should be made const.

Prefer const & as function arguments

Several of the functions take a std::string as an argument, but should probably be const std::string & instead to avoid the pointless duplication of a string.  The constructor and removeDuplicates are two such places, as are encrypt and decrypt.

Re-examine your algorithm

What you have implemented is not a transposition cipher, but a substitution cipher in which the ciphertext alphabet is derived by a transposition.  Encryption and decryption could be done with an inverse pair of std::unordered_map objects which may give slightly better performance.

Don't keep variables that are not needed

The vecOrder and mapOriginAlphabet variables are only need during object construction, so they should not be permanent data members of the object.  They could be created and destroyed as the map is being constructed.  Better yet, don't use them at all, as shown in the next suggestion.

Rethink your algorithm

Ultimately, the goal of the constructor is to create a substitution map based on the keyword.  The procedure is to eliminate duplicate letters in the keyword and then to create a table that includes all of the letters, rearranged according to the keyword.  For example, the keyword "bongo" would be reduced to "bngo" and then your vecTemp would look like this:

'b', 'o', 'n', 'g',
'a', 'c', 'd', 'e',
'f', 'h', 'i', 'j',
'k', 'l', 'm', 'p',
'q', 'r', 's', 't',
'u', 'v', 'w', 'x',
'y', 'z',

We then read down the columns according to the alphabetic order of the letter at the top of each column and create the substitution map:

a = b
b = a
c = f
d = k
e = q
f = u
g = y
h = g
i = e
j = j
k = p
l = t
m = x
n = n
o = d
p = i
q = m
r = s
s = w
t = o
u = c
v = h
w = l
x = r
y = v
z = z

I think that if you ponder it a while longer, there is perhaps a more direct way to derive the map. Instead, you could start with something like this:

std::string &delete_duplicates(std::string &str, size_t len) {
    for (size_t i=0; i < len; ++i) {
        str.erase(std::remove(str.begin()+i+1, str.end(), str[i]),
            str.end());
    }
    return str;
}

Then in the constructor, you could have this:

delete_duplicates(keyword, keyword.size());
std::string alphabet{keyword + "abcdefghijklmnopqrstuvwxyz"};
delete_duplicates(alphabet, keyword.size());

That gives the linear string that represents the two dimensional array shown above. Remap them into vector of strings and sort:

std::vector<std::string> columns(keyword.size());
for (size_t i=0, col=0; i < alphabet.size(); ++i) {
    columns[col++] += alphabet[i];
    if (col >= columns.size()) {
        col = 0;
    }
}
std::sort(columns.begin(), columns.end());
// now replace the alphabet
alphabet.clear();
for (auto &word : columns) {
    alphabet += word;
}

Now you can construct an encryption map (and a decryption map, if you like) by simply linearly assigning each letter of the "new" alphabet.


Simplified Android Spinner I have written a simple spinner wrapper, but was wondering if any of you could think of any ways to make it more robust. It only handles strings at the moment.

MySpinner:

package a.b.c;

import android.content.Context;
import android.util.AttributeSet;
import android.widget.ArrayAdapter;
import android.widget.Spinner;

public class MySpinner extends Spinner {

    // constructors (each calls initialise)
    public MySpinner(Context context) {
        super(context);
        this.initialise();
    }
    public MySpinner(Context context, AttributeSet attrs) {
        super(context, attrs);
        this.initialise();
    }
    public MySpinner(Context context, AttributeSet attrs, int defStyle) {
        super(context, attrs, defStyle);
        this.initialise();
    }

    // declare object to hold data values
    private ArrayAdapter<String> arrayAdapter;

    // add the selected item to the end of the list
    public void addItem(String item) {
        this.addItem(item, true);
    }
    public void addItem(String item, boolean select) {
        arrayAdapter.add(item);
        this.setEnabled(true);
        if (select) this.selectItem(item);
        arrayAdapter.sort(new Comparator<String>() {
            public int compare(String object1, String object2) {
                return object1.compareTo(object2);
            };
        });
    }

    // remove all items from the list and disable it
    public void clearItems() {
        arrayAdapter.clear();
        this.setEnabled(false);
    }

    // make the specified item selected (returns false if item not in the list)
    public boolean selectItem(String item) {
        boolean found = false;
        for (int i = 0; i < this.getCount(); i++) {
            if (arrayAdapter.getItem(i) == item) {
                this.setSelection(i);
                found = true;
                break;
            }
        }
        return found;
    }

    // return the current selected item
    public String getSelected() {
        if (this.getCount() > 0) {
            return arrayAdapter.getItem(super.getSelectedItemPosition());
        } else {
            return "";
        }
    }

    // allow the caller to use a different DropDownView, defaults to android.R.layout.simple_dropdown_item_1line
    public void setDropDownViewResource(int resource) {
        arrayAdapter.setDropDownViewResource(resource);
    }
    // internal routine to set up the array adapter, bind it to the spinner and disable it as it is empty
    private void initialise() {
        arrayAdapter = new ArrayAdapter<String>(super.getContext(), android.R.layout.simple_spinner_item);
        arrayAdapter.setDropDownViewResource(android.R.layout.simple_dropdown_item_1line);
        this.setAdapter(arrayAdapter);
        this.setEnabled(false);
    }
}

To use:

Use a.b.c.MySpinner instead of Spinner in your XML layout file
Set up a variable, mMySpinner = (MySpinner)findViewById(R.id.spinner);
You can then use all the functions which should be self-explanatory
If there are no items in the list, the spinner is disabled to prevent untoward events

mMySpinner.clearItems() - to remove all the items
mMySpinner.addItem("Blue") - to add Blue as an item in list (items are sorted by abc)
mMySpinner.selectItem("Red") - to make the indicate item the current selection
mMySpinner.getSelected() - to return the current selected item string

This is a very nice class, and I don't wish to discredit the author (FrinkTheBrave) in any way!  I do, however, want to offer a few enhancements and correct the nasty bugs that are inherent in the original.

The issues/enhancements addressed here:

  o Do not insert duplicates

  o Re-do selection if new row inserted

  o getSelected will return null rather than empty string if nothing selected

  o Proper comparison of string (a.equals(b) rather than a == b)

  o Sort is now case-insensitive

  o Removed 'this.' from this.foo where unneeded (which means, everywhere).

  o Changed setDropDownViewResource as per comment by @seand

To explain more about the re-selection:  Consider the following.  Spinner currently has these items (where > indicates the selected item)

    Antelope
  > Cat
    Dog

Now, we insert Bear.  Unless we re-do it, the second item will remain as 'selected.'  But because we inserted Bear alphabetically, the true Selected item has moved to the third position.

    Antelope
  > Bear
    Cat
    Dog

So, before the insertion of an non-selected String, we will save the String of the current selection (if there is one).  Then, after the insertion and the sort, we will select it again.

    Antelope
    Bear
  > Cat
    Dog

So if I may be so bold....

package c.b.a;

import java.util.Comparator;

import android.content.Context;
import android.util.AttributeSet;
import android.widget.ArrayAdapter;
import android.widget.Spinner;

public class AdvancedSpinner extends Spinner {

    // Thanks to CodeReview: http://codereview.stackexchange.com/questions/491/simplified-android-spinner
    // declare object to hold data values
    private ArrayAdapter<String> arrayAdapter;

    // constructors (each call initialize)
    public AdvancedSpinner(Context context) {
        super(context);
        initialize();
    }

    public AdvancedSpinner(Context context, AttributeSet attrs) {
        super(context, attrs);
        initialize();
    }

    public AdvancedSpinner(Context context, AttributeSet attrs, int defStyle) {
        super(context, attrs, defStyle);
        initialize();
    }

    // add the selected item to the end of the list
    public Boolean addItem(String item) {
        return addItem(item, true);
    }

    public Boolean addItem(String item, boolean select) {
        Boolean addFlag = true;

        for (int i = 0; i < getCount(); i++) {
            if (arrayAdapter.getItem(i).equals(item)) {
                // Don't add an item that's already in the list
                // But we do need to mark it as "selected" if
                // applicable.
                addFlag = false;
                if (select)
                    setSelection(i);
                break;
            }
        }
        if (addFlag) {
            String saveSelected = null;
            if (! select) {
                // We need to preserve the prior selection, which may be
                // messed up by our array sort operation.  So we will
                // save the selected item's value, and (after the insert)
                // we will reinstate its selection status.
                saveSelected = getSelected();
            }
            arrayAdapter.add(item);
            setEnabled(true);
            arrayAdapter.sort(new Comparator<String>() {
                public int compare(String object1, String object2) {
                    return  object1.compareToIgnoreCase(object2);
                };
            });
            if (select)
                selectItem(item);
            else if (saveSelected != null)
                selectItem(saveSelected);
        }
        return addFlag;
    }

    // remove all items from the list and disable it
    public void clearItems() {
        arrayAdapter.clear();
        setEnabled(false);
    }

    // make the specified item selected (returns false if item not in the list)
    public boolean selectItem(String item) {
        boolean found = false;
        for (int i = 0; i < getCount(); i++) {
            if (arrayAdapter.getItem(i).equals(item)) {
                setSelection(i);
                found = true;
                break;
            }
        }
        return found;
    }

    public String getSelected() {
        // return the current selected item
        // Changed (Dennis) to return null if nothing selected, rather than the
        // (misleading) empty string.  Also, prior incarnation would ForceClose
        // if array was not empty but nothing was selected.
        String rtnVal = null;
        if (getCount() > 0) {
            int i = super.getSelectedItemPosition();
            if (i >= 0)  // An item has been selected
                rtnVal = arrayAdapter.getItem(i);
        }
        return rtnVal;
    }

    // allow the caller to use a different DropDownView, defaults to android.R.layout.simple_spinner_dropdown_item
    public void setDropDownViewResource(int resource) {
        arrayAdapter.setDropDownViewResource(resource);
    }

    // internal routine to set up the array adapter, bind it to the spinner and disable it as it is empty
    private void initialize() {
        arrayAdapter = new ArrayAdapter<String>(super.getContext(), android.R.layout.simple_spinner_item);
        arrayAdapter.setDropDownViewResource(android.R.layout.simple_spinner_dropdown_item);
        setAdapter(arrayAdapter);
        setEnabled(false);
    }
}


Class structure to handle API exchange I'm developing a simple application that crawls web pages to obtain some information. For this I used and tested some libraries, like crawler4j, jsoup, jaunt and htmlunit. I change the crawler APIs several times because sometimes perceived that one served me better in a certain feature than the one I was using. I may have to do it again, and every time I do so I have to go around the code making various refactorings.

So I decide to separate the calls for these APIs in a kind of encapsulated class that holds all the operations I have to do with these API. So if I decide to change the crawler API I just point to another implementation of
CrawlerApiHandler.

I have troubles to identify one design pattern to mitigate this problem.

Below the 2 classes and the interface I created so far:

I named with suffix handler, but I don't know if it is it's appropriate.

/**
 * Created by alexandre on 29/01/2016.
 */
public interface CrawlerApiHandler {
    String visit (String url);
}

public class JauntApiHandlerImpl implements CrawlerApiHandler {
    UserAgent userAgent;

    public JauntApiHandlerImpl(UserAgent userAgent) {
        this.userAgent = userAgent;
    }

    @Override
    public String visit(String url) {
        try {
            return userAgent.visit(url).toString();
        } catch (ResponseException e) {
            return "";
        }
    }
}

public class JsoupCrawlerApiHandler implements CrawlerApiHandler {
    public static final String USER_AGENT = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.152 Safari/537.36";

    @Override
    public String visit(String url) {
        try {
            return Jsoup.connect(url).timeout(20000)
                    .userAgent(USER_AGENT).get().toString();
        } catch (IOException e) {
            //LOG
            return "";
        }
    }
}

Honestly, I think you have it just right with your current code: simple enough, yet powerful enough.

In your case, you need to support multiple implementations of Web crawlers API. To be able to abstract your code from all those implementations, an interface needs to be introduced. This interface will define the contract that your Web crawlers will satisfy. When this interface is introduced, the rest of your code can depend on that interface only, and not on the implementation.

Right now, your contract defines a methid visit(url) that takes a URL and returns the HTML content at that URL. This could be changed at a later time (e.g. adding time-outs or error handlers) but it's a good start.

The next problem to tackle is how to use the right implementation? This is typically done by using dependency injection. To put it simply, this is a design pattern where a class lets its dependencies be injected through constructor or setters instead of constructing them itself. For your example, consider a class MyClass that would depend on a Web crawler:

public class MyClass {

    private CrawlerApiHandler crawler;

    // the crawler to use in being injected into this class
    public BodyExtractor(CrawlerApiHandler crawler) {
        this.crawler = crawler;
    }

    // later on, use crawler to do some fancy things

}

You could then centralize the real implementation in a configuration class / file. The great advantage is that you now need to define the real implementation class in a single place and the rest of the code is completely decoupled from it by just depending on the interface.


Convert cardinal abbreviations into full cardinal direction I'm still learning Ruby and I made the following function to convert "N" or "SE" into "NORTH" or "SOUTH EAST" respectively.  The function works and the tests all pass, but the code seems to be overly complicated.  Any suggestions on how to clean it up and make it more Ruby-ish?

Code

module Example
  class Utils
    CARDINAL_PATTERN = /^([NS]?)([EW]?)$/
    CARDINAL_DB = {'N'=>'NORTH', 'S'=>'SOUTH', 'E'=>'EAST', 'W'=>'WEST' }
    def correct_cardinal word
      clean_word = word.gsub(/\p{Punct}/, '').upcase
      match = CARDINAL_PATTERN.match(clean_word)
      if match
        if (not match[1].empty?) and (not match[2].empty?)
          CARDINAL_DB[match[1]] + " " + CARDINAL_DB[match[2]]
        elsif (not match[1].empty?)
          CARDINAL_DB[match[1]]
        elsif (not match[2].empty?)
          CARDINAL_DB[match[2]]
        else
          ""
        end
      else
        word
      end
    end
  end
end

Tests

describe Example::Utils do
  let(:utils) { Example::Utils.new}

  describe '#correct_cardinal' do
    it 'should correct S' do
      expect(utils.correct_cardinal('S')).to eq "SOUTH"
    end

    it 'should correct N' do
      expect(utils.correct_cardinal('N')).to eq "NORTH"
    end

    it 'should correct E' do
      expect(utils.correct_cardinal('E')).to eq "EAST"
    end

    it 'should correct W' do
      expect(utils.correct_cardinal('W')).to eq "WEST"
    end

    it 'should strip single period' do
      expect(utils.correct_cardinal('W.')).to eq "WEST"
    end

    it 'should correct SE' do
      expect(utils.correct_cardinal('SE')).to eq "SOUTH EAST"
    end

    it 'should correct SW' do
      expect(utils.correct_cardinal('SW')).to eq "SOUTH WEST"
    end

    it 'should correct NE' do
      expect(utils.correct_cardinal('NE')).to eq "NORTH EAST"
    end

    it 'should correct NW' do
      expect(utils.correct_cardinal('NW')).to eq "NORTH WEST"
    end

    it 'should strip multiple periods' do
      expect(utils.correct_cardinal('N.W.')).to eq "NORTH WEST"
    end

    it 'should not correct 1' do
      expect(utils.correct_cardinal('1')).to eq "1"
    end

    it 'should not ordinalize eggs' do
      expect(utils.correct_cardinal('EGGS')).to eq "EGGS"
    end

  end

end

Some notes:

I'd leave some room between the constants and the method definitions (subjective).
if (not match[1].empty?) and (not match[2].empty?). Parenthenses not necessary. In any case, for boolean operations, use !, &&, ||.
When you have so many similar ifs, they can usually be refactored into something else (usually, with no conditionals at all, but array processing).

First, with your code as reference mind, I'd write:

module Example
  class Utils
    CARDINAL_PATTERN = /^([NS]?)([EW]?)$/
    CARDINAL_DB = {'N' => 'NORTH', 'S' => 'SOUTH', 'E' => 'EAST', 'W' => 'WEST'}

    def correct_cardinal(word)
      clean_word = word.gsub(/\p{Punct}/, '').upcase
      match = CARDINAL_PATTERN.match(clean_word)
      if match
        cardinals = match.captures.reject(&:empty?)
        CARDINAL_DB.values_at(*cardinals).join(" ")
      else
        word
      end
    end
  end
end

On a second refactor, as @janos pointed out, you can build the complete hash of {CARDINALS => NAME} (well, it's so short you could just write down all its values, but let's build it programatically to see a way to do it).

module Example
  class Utils
    CARDINALS = {'N' => 'NORTH', 'S' => 'SOUTH', 'E' => 'EAST', 'W' => 'WEST'}
    # DIRECTIONS: {"NE"=>"NORTH EAST", "NW"=>"NORTH WEST", "N"=>"NORTH", ...}
    DIRECTIONS = ["N", "S", ""].product(["E", "W", ""]).map do |cardinals|
      name = CARDINALS.values_at(*cardinals).compact.join(" ")
      [cardinals.join, name]
    end.to_h

    def correct_cardinal(word)
      clean_word = word.gsub(/\p{Punct}/, '').upcase
      DIRECTIONS.fetch(clean_word, word)
    end
  end
end


Elegant way to sort on multiple properties that might be undefined I'm trying to sort an array with JavaScript. The objects should be sorted based on several properties (about 4 in my real scenario), which are all strings that may also be null or undefined. A null or undefined value should be treated similar, and should be "earlier" in the sorted array than items with values.

I used unit tests to spec the behavior, and the simplest code that would work (which is really, really verbose) is the following:

function myComparer(a, b) {
  if (!!a.label && !!b.label) {
    var diff = a.label.localeCompare(b.label);
    if (diff !== 0) {
      return diff;
    }
  }

  if (!!b.label && !a.label) {
    return -1;
  }

  if (!!a.label && !b.label) {
    return 1;
  }

  if (!!a.code && !!b.code) {
    var diff = a.code.localeCompare(b.code);
    if (diff !== 0) {
      return diff;
    }
  }

  if (!!b.code && !a.code) {
    return -1;
  }

  if (!!a.code && !b.code) {
    return 1;
  }

  return 0;
}

test("Can compare given labels", function() {
  var result = [{label: 'Z'}, {label: 'A'}].sort(myComparer);
  strictEqual(result[0].label, 'A');
  strictEqual(result[1].label, 'Z');
});

test("Can compare null label with given label", function() {
  var result = [{label: null}, {label: 'A'}].sort(myComparer);
  strictEqual(result[0].label, null);
  strictEqual(result[1].label, 'A');
});

test("Can compare null label with given label - reverse", function() {
  var result = [{label: 'Z'}, {label: null}].sort(myComparer);
  strictEqual(result[0].label, null);
  strictEqual(result[1].label, 'Z');
});

test("Can compare given codes", function() {
  var result = [{code: 'Z'}, {code: 'A'}].sort(myComparer);
  strictEqual(result[0].code, 'A');
  strictEqual(result[1].code, 'Z');
});

test("Can compare null code with given code", function() {
  var result = [{code: null}, {code: 'A'}].sort(myComparer);
  strictEqual(result[0].code, null);
  strictEqual(result[1].code, 'A');
});

test("Can compare null code with given code - reverse", function() {
  var result = [{code: 'Z'}, {code: null}].sort(myComparer);
  strictEqual(result[0].code, null);
  strictEqual(result[1].code, 'Z');
});

test("Will sort on label before code", function() {
  var result = [
    {id: 1, label: 'Z', code: 'Z'},
    {id: 2, label: 'Z', code: 'A'},
    {id: 3, label: 'A', code: 'Z'},
    {id: 4, label: 'A', code: 'A'}].sort(myComparer);
  strictEqual(result[0].id, 4);
  strictEqual(result[1].id, 3);
  strictEqual(result[2].id, 2);
  strictEqual(result[3].id, 1);

});

<link href="http://code.jquery.com/qunit/qunit-1.12.0.css" rel="stylesheet"/>

This will not be pretty for 4 or more properties (I figured you can imagine this, I tried to keep my example a bit smaller).

I can think of several ways to improve this, but I struggle to balance:

Performance
Readability
Verbosity

Which is why I figured I'd ask here: do you have any suggestions on how to refactor my version into something more elegant?

PS. I'm happy to use ECMAScript5 stuff, I can use polyfills if anything's missing in browsers.

I saw this because you linked to it from a Knockout-related question on Stack Overflow. My first suggestion is to create a function that compares a single property, since the comparison of each property is the same. You can also use some other shortcuts.

function compareProperty(a, b) {
  return (a || b) ? (!a ? -1 : !b ? 1 : a.localeCompare(b)) : 0;
}

function myComparer(a, b) {
  return compareProperty(a.label, b.label) || compareProperty(a.code, b.code);
}

function compareProperty(a, b) {
  return (a || b) ? (!a ? -1 : !b ? 1 : a.localeCompare(b)) : 0;
}

function myComparer(a, b) {
  return compareProperty(a.label, b.label) || compareProperty(a.code, b.code);
}

test("Can compare given labels", function() {
  var result = [{label: 'Z'}, {label: 'A'}].sort(myComparer);
  strictEqual(result[0].label, 'A');
  strictEqual(result[1].label, 'Z');
});

test("Can compare null label with given label", function() {
  var result = [{label: null}, {label: 'A'}].sort(myComparer);
  strictEqual(result[0].label, null);
  strictEqual(result[1].label, 'A');
});

test("Can compare null label with given label - reverse", function() {
  var result = [{label: 'Z'}, {label: null}].sort(myComparer);
  strictEqual(result[0].label, null);
  strictEqual(result[1].label, 'Z');
});

test("Can compare given codes", function() {
  var result = [{code: 'Z'}, {code: 'A'}].sort(myComparer);
  strictEqual(result[0].code, 'A');
  strictEqual(result[1].code, 'Z');
});

test("Can compare null code with given code", function() {
  var result = [{code: null}, {code: 'A'}].sort(myComparer);
  strictEqual(result[0].code, null);
  strictEqual(result[1].code, 'A');
});

test("Can compare null code with given code - reverse", function() {
  var result = [{code: 'Z'}, {code: null}].sort(myComparer);
  strictEqual(result[0].code, null);
  strictEqual(result[1].code, 'Z');
});

test("Will sort on label before code", function() {
  var result = [
    {id: 1, label: 'Z', code: 'Z'},
    {id: 2, label: 'Z', code: 'A'},
    {id: 3, label: 'A', code: 'Z'},
    {id: 4, label: 'A', code: 'A'}].sort(myComparer);
  strictEqual(result[0].id, 4);
  strictEqual(result[1].id, 3);
  strictEqual(result[2].id, 2);
  strictEqual(result[3].id, 1);

});

<link href="http://code.jquery.com/qunit/qunit-1.12.0.css" rel="stylesheet"/>

Using the conditional-expression operator greatly shortens the code, but the biggest gain comes from using a function for common code. I also rearranged the conditional logic so that it doesn't have to make as many comparisons.


Minimum element in a tuple I'm solving exercise 2 from lecture 3 of Berkeley's CS 61A (2012):

  Write a function min_element that returns the minimum element in a tuple.

def min_element(tup):
    """ Returns the minimum element in tup.
    >>> a = (1, 2, 3, 2, 1)
    >>> min_element(a)
    1
    """

I know that Python has a built-in min function, but at this point in the course, the only operations on tuples that we have studied are indexing [1] [-1], slicing [1:], and concatenation +, so my solution needs to restrict itself accordingly.

My solution:

def min_element(tup):
    """ Returns the minimum element in tup.
    >>> a = (1, 2, 3, 2, 1)
    >>> min_element(a)
    1
    """
    def find_min(tup, min):
        if not tup:
           return min
        else:
           min = tup[0] if tup[0] < min else min
           return min_element(tup[1:], min)
    if tup:
        return find_min(tup[1:], tup[0])

Can this solution be improved?

Berkeley CS61A alum here. Your solution looks fine. Having sat through many a lecture where the prof would go through problems like this, I can pretty much guarantee this is the solution that would have been given:

def min_element(tup):
    min = tup[0]
    for element in tup[1:]:
        if element < min:
            min = element
    return min

So this solution might be considered by someone really pedantic to be a bit "better" than yours since it is less verbose and because, in general, recursion tends to be slower than iteration. Your solution works just fine though :)


Sort a binary file without loading it into memory or using a temporary file I had a silly assignment:

  You are given a binary file FLOATS.DAT of unknown length, type float
  data has been written in it. Write a program which sorts the data in
  the file (the output is a sorted file FLOATS.DAT). You need to sort it
  without loading the file into memory, or by using a temporary file.
  Use the Shell sort algorithm.

Obviously by using a temp. file it would be easy, even more if you could load it into memory.

Here is my solution:

#include <stdio.h>

// sizeof(float) = 4 on my machine

int main(int argc, char **argv)
{
    size_t sajz = sizeof(float); // for convenience
    int n; // total number of floats in the file

    FILE *dat;

    dat = fopen("FLOATS.DAT", "r+b");

    if (dat == NULL)
    {
        printf("Error while opening.\n");
        return 1;
    }

    // find the number of floats in the file
    fseek(dat, 0, SEEK_END);
    n = ftell(dat)/sajz;
    rewind(dat);

    float tjmh, tx;
    int i,j,h;

    // standard shell sort algorithm, the hard part is when I have to
    // read a position I first have to jump to it, then read it,
    // same when writing to a location
    for (h = n/2; h>0; h/=2)
    {
        for (i=h; i<n; i++)
        {
            j = i;

            fseek(dat, i*sajz, SEEK_SET);
            fread(&tx, sajz, 1, dat);
            rewind(dat);

            fseek(dat, (j-h)*sajz, SEEK_SET);
            fread(&tjmh, sajz, 1, dat);
            rewind(dat);

            for (; j>=h && tx < tjmh; j -= h)
            {

                fseek(dat, j*sajz, SEEK_SET);
                fwrite(&tjmh, sajz, 1, dat);
                rewind(dat);

                // this was tricky to figure out
                // I need the value of `j` after it has been
                // subtracted from h
                fseek(dat, (j - 2*h)*sajz, SEEK_SET);
                fread(&tjmh, sajz, 1, dat);
                rewind(dat);
            }

            fseek(dat, j*sajz, SEEK_SET);
            fwrite(&tx, sajz, 1, dat);
            rewind(dat);

        }
    }

    fclose(dat);

    return 0;
}

Can this be improved in any way?

Code snippet to generate the file with numbers 2.0, 3.0, 10.0, 4.0, 1.0, 7.0, 9.0, 5.0, 6.0, 8.0

int main()
{
    FILE *dat;

    dat = fopen("FLOATS.DAT", "wb");

    float a;
    if (dat)
    {
        a = 2.00;
        fwrite(&a, sizeof(float), 1, dat);
        a = 3.00;
        fwrite(&a, sizeof(float), 1, dat);
        a = 10.00;
        fwrite(&a, sizeof(float), 1, dat);
        a = 4.00;
        fwrite(&a, sizeof(float), 1, dat);
        a = 1.00;
        fwrite(&a, sizeof(float), 1, dat);
        a = 7.00;
        fwrite(&a, sizeof(float), 1, dat);
        a = 9.00;
        fwrite(&a, sizeof(float), 1, dat);
        a = 5.00;
        fwrite(&a, sizeof(float), 1, dat);
        a = 6.00;
        fwrite(&a, sizeof(float), 1, dat);
        a = 8.00;
        fwrite(&a, sizeof(float), 1, dat);

        fclose(dat);
    }

    return 0;
}

You can use an array here as well.

Don't Repeat Yourself (DRY)

            fseek(dat, i*sajz, SEEK_SET);
            fread(&tx, sajz, 1, dat);
            rewind(dat);

You do this three times.  The general rule is first time write, second time copy, third time refactor into a common function.  Note that if there is sufficient commonality, you may create the common function with only two uses (e.g. with the fwrite version).  Make a function:

void read_at_position(FILE *source, size_t position, size_t size, void *datum)
{
    fseek(source, position * size, SEEK_SET);
    fread(datum, size, 1, source);
    rewind(source);
}

I made datum a void * because that's what fread takes.  This way you don't have to write a new function if you change the datum type.

Now you can rewrite the original section (and the other four) like

            read_at_position(dat, i, sajz, &tx);

If you're worried about the overhead, you could create a macro instead

#define READ_AT_POSITION(source, position, size, datum) \
    do {\
        fseek(source, (position) * (size), SEEK_SET);\
        fread(datum, size, 1, source);\
        rewind(source);\
    } while (0)

which you'd use as

            READ_AT_POSITION(dat, i, sajz, &tx);

And the compiler will turn it into your original version.

Note that a good compiler will probably do that with the function as well.

Better names

The only way that I know what sajz and dat do is from looking at their declarations.  Good names should balance being easy to write (e.g. size) with being descriptive (size_of_each_datum or size_of_float).  Perhaps datum_size instead of sajz.

Similar problem with tjmh and tx.  Neither name tells me anything about what they do.

Consider changing h to gap_size.  Yes, h is a standard name in a shellsort (as per Wikipedia), but will most people remember that if they see it?

Reusability

Rather than writing this directly into main consider making a function.  E.g.

void sort_floats_in_file(FILE *source, size_t datum_size);

You could generalize this by passing in a comparison function.

void sort_in_file(FILE *source, size_t datum_size, int (*compar)(const void *, const void*));

However, that can add unnecessary overhead in simple cases like this.  Perhaps the compiler takes care of that.  Perhaps not.

Robustness

You say

    size_t sajz = sizeof(float); // for convenience

And later

    float tjmh, tx;

Consider flipping the order around and saying

    float tjmh, tx;
    size_t sajz = sizeof tjmh; // for convenience

Then if you change the data type of tjmh and tx, sajz will change to match automatically.  As is, it would be easy to forget and only change in the one place.

Similarly,

    float a;
    if (dat)
    {
        a = 2.00;
        fwrite(&a, sizeof(float), 1, dat);

Consider rewriting this as

    if (dat)
    {
        test_write(2.00, dat);

with

void test_write(float a, FILE *dat)
{
    fwrite(&a, sizeof a, 1, dat);
}

Not only is this less typing, it avoids the problem of changing the type in one place and not the other.  Admittedly there's some extra work to write the function, but the reduction per test case should cover it.


Calculating pi using bash I've written a little program to calculate pi using the Nilakantha series:

  For this formula, take three and start alternating between adding and subtracting fractions with numerators of 4 and denominators that are the product of three consecutive integers which increase with every new iteration. Each subsequent fraction begins its set of integers with the highest one used in the previous fraction. Carry this out even a few times and the results get fairly close to pi. (http://www.wikihow.com/Calculate-Pi)

I don't really understand other ways of calculating pi, or they take too long to perform.

echo "Enter scale please"
read SCALE
VALUE=2
PI=3
FITNESS=1
while true
do
PI=$(echo "scale=$SCALE;$PI+(4/($VALUE*($VALUE+1)*($VALUE+2)))-(4/(($VALUE+2)*($VALUE+3)*($VALUE+4)))" | bc)
VALUE=$(($VALUE+4))
FITNESS=$(($FITNESS+1))
echo "###############"
echo "--> $FITNESS // $VALUE"
echo "$PI"
done

I would really like to know how to detect when I get the same output multiple times, so I can tell the program to terminate when it has reached the most accurate version of pi possible.

Also if you have other suggestions on how to improve the code and/or know a better way of calculating pi (with some explanation), I would like to hear.

I see a few things that could allow you to improve your program.  First, though, I don't consider myself a bash expert, so there may well be better ways of doing these things.

Use a "shebang" line

As this question points out, you should always use a "shebang" line for your bash scripts.  So the first line would be:

#!/usr/bin/env bash

Pass values as arguments

Rather than prompting for the SCALE value, it's generally better to use a command line argument.  That way, the script can be reused by other shell scripts.

Provide a stopping mechanism

As each term is calculated, eventually, it will be equal to zero given the passed scale.  This suggests a mechanism for stopping: check each term for 0 before adding it.

Indent do and while loops

I don't know of a bash style guide (there probably is one!) but I like to see the contents of loops indented to make it easier to read.

Putting it all together

Here's a modification of your script with all of these suggestions implemented:

bashpi.sh

#!/usr/bin/env bash
SCALE=$1
VALUE=2
PI=0
FITNESS=1
DELTA=3
while [ $(echo "$DELTA==0" |bc) != "1" ]
do
    PI=$(echo "$PI+$DELTA" | bc)
    DELTA=$(echo "scale=$SCALE;(4/($VALUE*($VALUE+1)*($VALUE+2)))-(4/(($VALUE+2)*($VALUE+3)*($VALUE+4)))" | bc)
    VALUE=$(($VALUE+4))
    FITNESS=$(($FITNESS+1))
    echo "###############"
    echo "--> $FITNESS // $VALUE"
    echo "$PI"
done

To better understand how this works, you can replace the three echo statements with this one:

echo "DELTA = ${DELTA} --> ${FITNESS} // ${VALUE} : ${PI}"

Sample output

With ./bashpi.sh 4, and the modified echo above I get this output:

DELTA = .1333 --> 2 // 6 : 3
DELTA = .0064 --> 3 // 10 : 3.1333
DELTA = .0012 --> 4 // 14 : 3.1397
DELTA = .0003 --> 5 // 18 : 3.1409
DELTA = .0001 --> 6 // 22 : 3.1412
DELTA = .0001 --> 7 // 26 : 3.1413
DELTA = .0001 --> 8 // 30 : 3.1414
DELTA = 0 --> 9 // 34 : 3.1415


PHPmailer sanitazion/validation a second oppinion. I wonder if someone could give me some pointers to my PHPmailer Process file and see if they find any misstakes or things that I have missed in the validation/sanitation of the data.
Any input would be most wellcome!

<?php
//includes
require_once('func/mail/mailcon/config-info.php');
require_once('include/class.phpmailer.php');
//Sanitazion and cleanup.
if ($_SERVER['REQUEST_METHOD'] == 'POST') {

$entries = array();
$errors = array();
// Escape and extract all the post values
foreach ($_POST as $key => $value) {
    $entries[$key] = sanitize($value);
}

// Get a set of variable variables for easier use
foreach ($entries as $key => $value) {
    ${$key} = $value;
}
//validation
//contact_Name
if (empty($contact_Name)) {
$errors['contact_Name'] = 'Detta fält måste fyllas i.';
}
elseif (mb_strlen($contact_Name, 'UTF-8') > 50) {
$errors['contact_Name'] = 'Detta namn är för långt, får inte vara över 50 tecken.';
}
//contact_Foretag.
if (mb_strlen($contact_Foretag, 'UTF-8') > 50) {
$errors['contact_Foretag'] = 'Detta företagsnamn är för långt, får inte vara över 50 tecken';
}
//contact_Email
if (empty($contact_Email)) {
$errors['contact_Email'] = 'Detta fält måste fyllas i.';
}
elseif (!(filter_var($contact_Email, FILTER_VALIDATE_EMAIL))) {
$errors['contact_Email'] = 'Fyll i en giltig E-post address.';
}
//contact_Subject
if (empty($contact_Subject)) {
$errors['contact_Subject'] = 'Detta fält måste fyllas i.';
}
elseif (mb_strlen($contact_Subject, 'UTF-8') > 50) {
$errors['contact_Subject'] = 'Detta ämne är för långt, får inte vara över 50 tecken.';
}
//contact_Message
if (empty($contact_Message)) {
$errors['contact_Message'] = 'Detta fält måste fyllas i.';
}
elseif (mb_strlen($contact_Message, 'UTF-8') > 5000) {
$errors['contact_Message'] = 'Detta meddeoande är för långt, får inte vara över 5000 tecken.';
}

//If no errors
if (empty($errors)) {

$formOK = true;
$mail = new PHPMailer(true);

try {
  $mail->IsSMTP();
  $mail->SMTPDebug = 0;
  $mail->SMTPAuth = true;
  $mail->SMTPSecure = "ssl";
  $mail->Host = MAIL_HOST;
  $mail->Port = MAIL_PORT;
  $mail->Username = MAIL_USER;
  $mail->Password = MAIL_PASS;
  $mail->SetFrom($contact_Email, $contact_Name);
  $mail->Subject = "$contact_Subject";
  $mail->Body = "Your received the following message from $contact_Mame ($contact_Foretag)   <$contact_Email>:\r\n\r\n$contact_Message";
  $mail->AddAddress(MAIL_ADDR);
  $mail->Send();

 } catch (PHPMailerException $e) {
 header("HTTP/1.1 500 Internal Server Error");
        echo "Exception occurred: ".$e->errorMessage();
        exit();
 }
}
else {
    $formOK = false;
}

//if this is not an ajax request
if(empty($_SERVER['HTTP_X_REQUESTED_WITH']) && strtolower($_SERVER['HTTP_X_REQUESTED_WITH']) !== 'xmlhttprequest'){
    session_start();
    $return_data = array(
        'formOK'  => $formOK,
        'errors'  => $errors,
        'entries' => $entries
    );
    $_SESSION['return_data']  = $return_data;
    header('Location: ' . BASE_URL);
    exit();
}
} //First
else {
header('Location: ' . BASE_URL);
exit();
}

and welcome to CodeReview!

Here's a few things:

You have a lot of pointless comments

("includes": The includes are the next line. We see them; no need to tell us.)

Indent block content.

Whenever you enter an if block, indent it.
Same for while/for/functions/classes/methods/etc

Your variable-variable loop is equivalent to extract.
extract tends to be a bad idea (the manual has various warnings)

It pollutes the scope
You no longer know what variables are and are not defined unless you strictly control what is inside of the array (which you don't for $_POST)
Users can clobber your values, meaning there are certain security implications.
Can be mitigated with options to extract, but still a problem
In short, there are a few situations where extract is convenient and a decent idea; this is not one of them.
use $_POST[...] or $entries[...] directly.

Variable variables are also a bad idea.

Variable variables scream "What I really need is an array!"
You already have an array :)

Don't sanitize content until you're actually ready to put it into the context for which you're sanitizing it.

For example, & is a special character in HTML (in some contexts). DBs do not care about it. Nor do plain text emails. Or email subjects. Or... infinite things.
Rule of thumb: sanitize at the last possible moment.
You can also take the raw value and process it, but you can't always do the reverse.

Be careful with empty() on strings

empty($var) === (isset($var) && $var)
Implicit bool rules are very odd in PHP though. In particular, the string "0" can get you.
I doubt 0 should be a valid email content, but just throwing it out there in case it gets you in the future.

Invert your conditions and you'll see that you can bail early:

if (...) { /* huge block / } else { / redirect / exit; }
Instead you could do if (!...) { / redirect */ exit; }
Now your entire file doesn't have to be indented one step.
This is similar to the concept of returning early, though it tends to be less applicable to files (redirection is one of the few justified uses of exit in web site PHP)

You have a typo of $contact_Mame rather than $contact_Name in your email content
Be careful with 500 errors. IE has a minimum length limit where if your response is less than some number of bytes, IE 'helpfully' replaces your content with its own.

Also, I'm torn on how I feel about this being a 500 response. It's not really an internal error so much as just an exception.
Internal errors implies a major mess up to me like Apache is pissed off, or a E_FATAL PHP error.

You should probably log the email failing to send. You could just use something like trigger_error if you want to go the simple route.
$formOK is gaurenteed to be $formOK === empty($errors). This means you could just use empty($errors) in it's place.
I tend to use only strict equalities with strings these days. If you're comparing something to a string, you're sure 99.9% of the time that it's a string. If you're sure it should be a string, might as well use a strict check.

There are situations where loose string comparisons are still quite handy though.

I would use a GET or POST variable to signify AJAX rather than checking a header.

Headers can be spoofed.
Testing becomes easier.
No potential browser/JS library compatibility issues

$mail->Subject = "$contact_Subject"; is (functionally) equivalent to $mail->Subject = (string) $contact_Subject;

You know it's already a string, so no need for the cast.
Just do $mail->Subject = $contact_Subject.
Also, I wouldn't use "$var" as a cast. I would stick with explict casts ((string) $var).