Calculating change based on input I decided to make a program that calculates change, based on an input. My code is extremely long, and I would like to know how I can fix this.

def make_change(amount)
  values = []
  coins = []
  hash = {}
  if amount >= 50
    coins.push(:H)
    values.push(amount / 50)
    amount = amount % 50
    if amount % 50 >= 25
      coins.push(:Q)
      values.push(amount / 25)
      amount = amount % 25
      if amount % 25 >= 10
        coins.push(:D)
        values.push(amount / 10)
        amount = amount % 10
        if amount % 10 >= 5
          coins.push(:N)
          values.push(amount / 5)
          amount = amount % 5
          if amount % 5 >= 1
            coins.push(:P)
            values.push(amount / 1)
          end
        end
      end
    end
  elsif amount >= 25
    coins.push(:Q)
    values.push(amount / 25)
    amount = amount % 25
    if amount % 25 >= 10
      coins.push(:D)
      values.push(amount/10)
      amount = amount % 10
      if amount % 10 >= 5
        coins.push(:N)
        values.push(amount/5)
        amount = amount % 5
        if amount % 5 >= 1
          coins.push(:P)
          values.push(amount/1)
        end
      end
    end
  elsif amount >= 10
    coins.push(:D)
    values.push(amount / 10)
    amount = amount % 10
    if amount % 10 >= 5
      coins.push(:N)
      values.push(amount / 5)
      amount = amount % 5
      if amount % 5 >= 1
        coins.push(:P)
        values.push(amount / 1)
      end
    end
  elsif amount >= 5
    coins.push(:N)
    values.push(amount / 5)
    amount = amount % 5
    if amount % 5 >= 1
      coins.push(:P)
      values.push(amount / 1)
    end
  elsif amount >= 1
    coins.push(:P)
    values.push(amount / 1)
  else
    coins = []
    values = []
  end
  Hash[coins.zip(values)]
end

Examples

make_change(42) should result in
   => {:Q => 1, :D => 1, :N => 1, :P => 2

make_change(91) should result in
   => {:H => 1, :Q => 1, :D => 1, :N => 1, :P => 1

Here's one way, that uses the method Fixnum#divmod to advantage:

COINS = [["H", 50], ["Q", 25], ["D", 10], ["N", 5], ["P", 1]]

def change(amount)
  raise ArgumentError, 'Only non-negative integers permitted' unless
    amount.is_a?(Fixnum) && amount >= 0
  return [] if amount.zero?
  COINS.map do |label, value|
    nbr, amount = amount.divmod(value)
    [nbr, label]
  end
end

change(260) #=> [[5, "H"], [0, "Q"], [1, "D"], [0, "N"], [0, "P"]]
change(95)  #=> [[1, "H"], [1, "Q"], [2, "D"], [0, "N"], [0, "P"]]
change(100) #=> [[2, "H"], [0, "Q"], [0, "D"], [0, "N"], [0, "P"]]
change(80)  #=> [[1, "H"], [1, "Q"], [0, "D"], [1, "N"], [0, "P"]]
change(45)  #=> [[0, "H"], [1, "Q"], [2, "D"], [0, "N"], [0, "P"]]
change(16)  #=> [[0, "H"], [0, "Q"], [1, "D"], [1, "N"], [1, "P"]]
change(1)   #=> [[0, "H"], [0, "Q"], [0, "D"], [0, "N"], [1, "P"]]
change(0)   #=> []
change(-3)  #=> ArgumentError: Only non-negative integers permitted...
change(1.2) #=> ArgumentError: Only non-negative integers permitted...
change('c') #=> ArgumentError: Only non-negative integers permitted...


Project Euler #15 in Python I've wrote the code for Project Euler #15 and it runs amazingly fast. I know I can write it using binomials but I wanted to calculate by brute force as if I do not know Binomials. But I think it must be taking huge amount of memory so I want to know how I can reduce amount of memory used by program. I've tried to reduce elements in the Dictionary as much as possible but I cannot think of any other thing.

import time

st = time.clock()
results = {}

def routes(a, b):
    if a > b:
        a, b = b, a
    if a == 0:
        return 1
    if a == b:
        if str([a - 1, b]) in results:
            return 2 * results[str([a - 1, b])]
        else:
            part1 = routes(a - 1, b)
            results[str([a - 1, b])] = part1
            return 2 * part1

    if str([a - 1, b]) in results:
        part1 = results[str([a - 1, b])]
    else:
        part1 = routes(a - 1, b)
        results[str([a - 1, b])] = part1
    if str([a, b - 1]) in results:
        part2 = results[str([a, b - 1])]
    else:
        part2 = routes(a, b - 1)
        results[str([a, b - 1])] = part2
    return part1 + part2

number = int(input("Enter n : "))
#print((routes(20, 20)))
print((routes(number, number)))
#print((results))
print((len(results)))
print(("Time took : ", time.clock() - st))

Output 1:

Enter n : 20
Answer = 137846528820
Number of elements in hashtable/dictionary are 229
Time took : 0.002663 seconds.

Output 2:

Enter n : 480
Answer = 250892214537448775747234101104975594366453286042769782121222122282812207368859289614865150179674097028772541144903685677912632260432728444419421872981795600499773664936037617507243883516784270458848737326769427370361260166492752950426905484595009507753796738842521345046644373977709359600

Number of elements in hashtable/dictionary are 115919
Time took : 0.483901 seconds.

The code converts a pair of numbers to a dictionary key by putting the numbers in a list and then turning the list into a string: str([a, b]). But that's not necessary: you can use the tuple (a, b) as a dictionary key without having to convert it to a string.

(The reason that you can't use a list [a, b] as a dictionary key is that lists can be modified, which invalidates their use as keys. Tuples can't be modified, and so you can use them as keys.)
It would simplify the code if the function routes (i) started by looking up its parameters in the results dictionary to see if it already had an answer; and (ii) always stored its result in the dictionary. If you did both of these, then you could avoid a lot of the cases:

results = {}

def routes(a, b):
    "Number of routes from (a, b) to (0, 0) in a grid."
    if (a, b) in results:
        return results[a, b]
    elif a > b:
        r = routes(b, a)
    elif a == 0:
        r = 1
    else:
        r = routes(a - 1, b) + routes(a, b - 1)
    results[a, b] = r
    return r

This code does two things: it computes the number of routes, and it arranges to cache the results of the computation in a dictionary to avoid having to recompute it. The principle of separation of concerns suggests that we should try to split these two things into different bits of code. That would make them easier to understand, test, and reuse.

In this case, Python already has built-in support for caching the results of a function: see functools.lru_cache. So all you need to implement is the route-counting part of the code, and let lru_cache do the caching:

from functools import lru_cache

@lru_cache(maxsize=None)
def routes(a, b):
    "Number of routes from (a, b) to (0, 0) in a grid."
    if a > b:
        return routes(b, a)
    elif a == 0:
        return 1
    else:
        return routes(a - 1, b) + routes(a, b - 1)


Generic LinkedList with add/exists/delete/dlear/iteration functionality I've just been going through core data structures and algorithms recently, wanting to get back in touch with the core fundamentals. I'm starting simple with a LinkedList implementation in C#. The functionality works and was able to test it. I'm just wondering if yo see anything with the code that could be improved.

using System.Collections;
using System.Collections.Generic;

namespace DataStructuresAndAlgorithms.DataStructures.LinkedList
{
    public class SinglyLinkedList<T> : IEnumerable<T>
    {
        public SinglyLinkedListNode<T> Head { get; set; }
        public SinglyLinkedListNode<T> Tail { get; set; }

        public void AddToEnd(T value)
        {
            var newNode = new SinglyLinkedListNode<T>
            {
                Value = value,
                Next = null
            };
            if (Head == null && Tail == null)
            {
                Head = newNode;
                Tail = newNode;
            }
            if (Head == Tail)
            {
                Head.Next = newNode;
                Tail = newNode;
            }
            else
            {
                var existingLast = Tail;
                existingLast.Next = newNode;
                Tail = newNode;
            }
        }

        public void AddToBeginning(T value)
        {
            var newNode = new SinglyLinkedListNode<T>
            {
                Value = value,
                Next = null
            };
            if (Head == null && Tail == null)
            {
                Head = newNode;
                Tail = newNode;
            }
            else
            {
                var existingHead = Head;
                Head = newNode;
                Head.Next = existingHead;
            }
        }

        public bool Exists(T value)
        {
            var head = Head;
            while(head != null)
            {
                if (head.Value.Equals(value))
                {
                    return true;
                }
                head = head.Next;
            }
            return false;
        }

        public void Delete(T value)
        {
            var head = Head;
            SinglyLinkedListNode<T> previous = null;
            while(head != null)
            {
                if (head.Value.Equals(value))
                {
                    if (previous == null)
                    {
                        Head = Head.Next;
                    }
                    else
                    {
                        previous.Next = head.Next;
                    }
                }
                previous = head;
                head = head.Next;
            }
        }

        public void Clear()
        {
            Head = null;
            Tail = null;
        }

        public IEnumerator<T> GetEnumerator()
        {
            var head = Head;
            while(head != null)
            {
                yield return head.Value;
                head = head.Next;
            }
        }

        IEnumerator IEnumerable.GetEnumerator()
        {
            return GetEnumerator();
        }
    }

    public class SinglyLinkedListNode<T>
    {
        public T Value { get; set; }
        public SinglyLinkedListNode<T> Next { get; set; }
    }
}

I would implement ICollection<T> in addition to IEnumerable<T>. It should be easy and straightforward.
Public setters of the Head and Tail properties will allow users of your class to corrupt the structure of linked list.
Not sure if you need to expose Head and Tail as properties at all, as they are the only ones who expose the SinglyLinkedListNode<T> instances used internally.
AddToEnd implementation can be simplified. The case of Head == Tail doesn't differ from else clause. So the method can be refactored as following (note that the non-null Head implies non-null Tail, this should be guaranteed by the design of your class):

public void AddToEnd(T value)
{
    var newNode = new SinglyLinkedListNode<T>
    {
        Value = value,
        Next = null
    };

    if (Head == null)
    {
        Head = newNode;
    }
    else
    {
        Tail.Next = newNode;
    }

    Tail = newNode;
}

Similar refactorings can be applied to AddToBeginning. When you create a new node to be inserted at the beginning, its Next property will always refer to original Head node, whether it was null (list is empty) or actual node. Thus the method can be reduced to:

public void AddToBeginning(T value)
{
    Head = new SinglyLinkedListNode<T>
    {
        Value = value,
        Next = Head
    };

    if (Tail == null)
    {
        Tail = Head;
    }
}

Delete method has a bug - it will never assign null to Tail, even if all elements are deleted from the list.


Sum of Maximum GCD from two arrays I've written a program to solve the same problem as Sum of Maximum GCD.

My solution contains working code and it has successfully passed some of the test cases. But it does throw runtime exceptions and I'm trying to understand where I could be going wrong. I don't see any segmentation faults that could be tripping it up.

  You are given two arrays \$A\$ and \$B\$ containing \$N\$ elements each. Choose a
  pair of elements \$x, y\$ such that:

  \$x\$ belongs to array \$A\$.
  y belongs to array \$B\$.
  \$\gcd(x,y)\$ is the maximum of all pairs \$x, y\$.

  If there is more than one such pair \$x, y\$  having maximum \$\gcd\$, then
  choose the one with maximum sum. Print the sum of elements of this
  maximum-sum pair.

  NOTE: \$\gcd(x,y)\$ the largest integer that divides both \$x\$ and
  \$y\$.

  Constraints:

  \$1 \le N \le 5 \times 10^5\$
  \$1 \le A_i \le 10^6\$
  \$1 \le B_i \le 10^6\$

  Input Format

  The first line of the input contains n denoting the total number of
  elements of arrays \$A\$ and \$B\$. Next line contains \$n\$ space separated
  positive integers denoting the elements of array \$A\$. Next line contains
  \$n\$ space separated positive integers denoting the elements of array \$B\$.

  Output Format

  From all the pairs having maximum gcd, print the sum of one that has
  the maximum sum.

I'm still actively trying to find the issue, though I think I could benefit from unbiased eyes and criticism to help me better my code.

#!/bin/python3

import sys
import itertools
import math
def maximumGcdAndSum(A, B):
    C = []
    C.append(A)
    C.append(B)
    D=[]
    E=[]
    gcds=[1]
    sums=[]
    prodC = itertools.product(*C)
    for element in prodC:
        gcdVal = math.gcd(element[0],element[1])
        D.append((gcdVal,sum(element)))
        gcds.append(gcdVal)
       # sums.append(sum(element))
   # print(D)
    maxGCD = max(gcds)
    sumsL = [1]
#print(gcds)
   # print(sums)
    for i in D:
        if i[0] == maxGCD:
            sumsL.append(i[1])
    return max(sumsL)
    # Complete this function

if __name__ == "__main__":
    n = int(input().strip())
    A = list(map(int, input().strip().split(' ')))
    B = list(map(int, input().strip().split(' ')))
    res = maximumGcdAndSum(A, B)
    print(res)

Sample input tried -

5
3 1 4 2 8
5 2 12 8 3

Output Expected -

16

Output Obtained -

16

There are quite a few ways to reduce the number of lines in your program, and improve the readability of your program:

If you want to return a certain number when A and B are empty, use a guard clause.

Personally I'd default to -1, rather than 1.
When building a list, you can use C = [A, B], rather than appending.
You don't need to build an array, to unpack it into a function. itertools.product(A, B) is fine.
The list gcds is the same as the first index of D. It'd be simpler if you just used D.
The lists E and sums are unused, and so should be removed.
The 'comments', commented out code, is generally unhelpful and distract from the algorithm. I'd recomend removing them.
Rather than building a list with D = [] for i in ...: d.append(i), you can use a list comprehension.
Rather than building a list that may take ~2275GB of RAM, you may instead want to use a generator / generator comprehension.
max works on a list of tuples.

And so I'd rather use:

import itertools
import math

def maximumGcdAndSum(A, B):
    if not (a or b):
        return -1
    nums = (
        (math.gcd(a, b), a + b)
        for a, b in itertools.product(A, B)
    )
    return max(nums)[1]

if __name__ == "__main__":
    n = int(input().strip())
    A = list(map(int, input().strip().split(' ')))
    B = list(map(int, input().strip().split(' ')))
    res = maximumGcdAndSum(A, B)
    print(res)

This however is likely to still produce a time limit exceeded error. I'm thinking of a better algorithm.


classList fallback (IE & older browsers) I'm trying to make some JS code more browser compatible. So I've written an alternative to JS's classList providing its .add(), .remove() and .toggle() functionality because it is not or just partially supported in IE.

(I know there are exiting polyfills, but I like to have a good overview regarding my code.)

My test were successful but before I use it extensively I wanted to check with someone if they see any issues.

    function classes(type, element, targetClass){
            var classArray = element.className.split(/\s/);
            var index = classIndex(classArray, targetClass);
            // if target class is in class list
            if(index !== null){
                // multiple classes
                if(classArray.length > 1){
                    if(type == "remove" || type == "toggle") {
                        classArray.splice(index, 1);
                        element.className = classArray.join(" ");
                    }
                }
                // single class
                else {
                    if(type == "remove" || type == "toggle") {
                        element.className = "";
                    }
                }
            }
            // if target class is not in class list
            else {
                if(type == "add" || type == "toggle") {
                    element.className += " " + targetClass;
                }
            }
            // check if target class is in class list (returns index or null)
            function classIndex(array, targetClass){
                for(i=0; i<array.length; i++){
                    if(array[i]==targetClass){
                        return i;
                    }
                    if(i==(array.length-1)){
                        return null;
                    }
                }
            }
        }

Usage:

classes("toggle", myElement, "classToToggle")

I'm not a big fan of neither the function name classes nor the parameter name type. Something like modifyClassOfElement and action would be better fitting.

You should define constants for the type/action values.

It's convention in JavaScript to have a space between keywords and a following open bracket: if  (

In classIndex the second if is unnecessary. Just put return null; after the loop. Or, even better, you can replace the whole function with an .indexOf() call.

Move if (type == "remove" || type == "toggle") { up one level so you don't need it twice. Alternatively I'd just drop the special case for a single class, as it's an tiny optimization that is hardly worth it.

Remove one level of brackets on the final else:

else if (type == "add" || type == "toggle") {
  element.className += " " + targetClass;
}

I'd consider throwing an exception, if the type/action is unknown to aid debugging.

It would be nice for the function to use classList if it's available. Personnally I'd implement this as a function that returns an existing classList or an object that implements the same interface (DOMTokenList)


Cascading a property for a game I'm trying to write a simple game. I have an Entity object, that has a list of Sprite objects. Each Sprite has a position property where it is drawn on the screen. Sometimes, an entity can contain a lot of Sprites in which case it is much easier to set the position property on the parent Entity.

Here's a simplified example of creating an Entity with a list of Sprites:

var player = new Entity( new List<Sprite>
                         {
                             new Sprite( "characters/walkcycle/body", new Vector2( 128, 128 ) ),
                             new Sprite( "characters/walkcycle/halo", new Vector2( 128, 120 ),
                             new Sprite( "characters/walkcycle/shirt", new Vector2( 128, 128 ) ),
                             new Sprite( "characters/walkcycle/pants", new Vector2( 128, 128 ) ),
                             new Sprite( "characters/walkcycle/shoes", new Vector2( 128, 128 ) )
                         } );

Most of the Sprites have the same position, so I do this instead:

var player = new Entity( new Vector2( 128, 128 ),
                         new List<Sprite>
                         {
                             new Sprite( "characters/walkcycle/body" ),
                             new Sprite( "characters/walkcycle/halo", new Vector2( 128, 120 ),
                             new Sprite( "characters/walkcycle/shirt" ),
                             new Sprite( "characters/walkcycle/pants" ),
                             new Sprite( "characters/walkcycle/shoes" )
                         } );

Notice the halo sprite is in a different position. Later in the code, I call the Load method on Entity, which passes its position property on to the children Sprites:

public class Entity
{
    // fields, constructor, properties ...

    public void Load()
    {
        foreach ( var sprite in this.sprites )
        {
            sprite.Load( this.position );
        }
    }
}

The Load method on Sprite will set its own position property to the value passed in if it doesn't already have one:

public class Sprite
{
    // fields, constructor, properties ...

    public void Load( Vector2 position )
    {
        if ( this.position == Vector2.Zero && position != Vector2.Zero )
        {
            this.position = position;
        }
    }
}

Is there a better way to do this? It smells a little to me.

Another approach would be to make the positions of sprites relative to the position of the entity. That way, the default of zero comes up naturally and when you move an entity, all of its sprites are automatically moved too.

One more advantage is that this means position zero now becomes a valid one.


Script to generate random password I made this random password script. It works, but i think it is kind of bloated. The part where random characters are being added is the area where I think it needs work. I don't want to declare if and else statement for every combination of lowercase list, uppercase list and digits list(I am taking input from user whether they want uppercase,lowercase,digits or any possible combination of these three). Is there any possible way in which I can just check which of these three values are yes and then just use only those.
 Also what is more efficient using lower() to turn the list called using string module into lower case or just calling the lowercase list itself.
import random
from  string import ascii_letters ,digits
while True:
    userchoice_size = int(input("Enter the length of the password : ")) # Asks for length of password
    userchoice_syntax_lower = input("Do you want lower case letter?(y or n) ")
    userchoice_syntax_upper = input("Do you want upper case letter?(y or n) ")
    userchoice_syntax_number = input("Do you want numbers?(y or n) ")
    counter=0
    password='' # String which stores the password
    choice_list=[1,2,3]
    while counter <= userchoice_size:
        choice_number = random.choice(choice_list)
        if choice_number == 1:
            if userchoice_syntax_upper == 'y': # Uppercase list
                password = password + str(random.choice(ascii_letters.upper()))
                counter += 1
        if choice_number == 2:
            if userchoice_syntax_number=='y': # Digits list
                password = password + str(random.choice(digits))
                counter += 1
        if choice_number == 3:
            if userchoice_syntax_lower == 'y': # Lower case list
                password= password + str(random.choice(ascii_letters.lower())) # Converts all letters into lowercase then randomly select and store in list
                counter += 1
    print (password)

Character distribution
You randomly choose between uppercase, lowercase and digit characters with an equal probability, but there are more letters than digit characters. That means characters will be under-represented in your password's probability distribution. Easier and more accurate is to make one string of all eligible characters and choose from it randomly.
Redundant str
No need for str here:
str(random.choice(ascii_letters.upper()))

Move your increment
counter += 1

should not be repeated three times in if statements. You can do it once at the end of the loop. Better yet,
for _ in range(userchoice_size):

Choice list
This:
choice_list=[1,2,3]
choice_number = random.choice(choice_list)

should not use a list, and should be replaced with a call to randrange.
Secure entropy
For password generation it's crucially important to use a secure source of entropy. Read this quote carefully:

Python uses the Mersenne Twister as the core generator. It produces 53-bit precision floats and has a period of 2**19937-1. The underlying implementation in C is both fast and threadsafe. The Mersenne Twister is one of the most extensively tested random number generators in existence. However, being completely deterministic, it is not suitable for all purposes, and is completely unsuitable for cryptographic purposes.

Emphasis mine. A better built-in option is secrets.


Simple HTML and CSS website I'm making a really simple html website with CSS and I was hoping on some feedback. A few notes:

There will be more content under this, but I want the starting screen to take up the full page and then scroll under it.
I've used a bit of a hack to remove the padding from the first list element
I've used a bit of a hack to remove spacing between list elements
I'm a bit unsure if I have to clear floats for the menu bar as it has no height at the moment. If so, clear:float didn't seem to work.

* {
  padding: 0;
  margin: 0;
}
html,
body {
  height: 100%;
}
a {
  text-decoration: none;
}
.header {
  height: 100%;
  width: 100%;
  background: lightgray;
  position: relative;
  text-align: center;
  min-height: 350px;
}
li {
  list-style: none;
}
.nav {
  display: block;
  padding: 20px;
}
.nav img {
  float: left;
}
.nav ul {
  float: right;
  font-size: 0;
}
.nav ul li {
  display: inline-block;
  font-size: 16px;
  padding-left: 10px;
}
.nav ul li:first-child {
  padding-left: 0px
}
.nav ul li a {
  padding: 0 10px;
}

Services

Contact

    Hello!

Test

Markup

Your markup has a case of divitis.  In years past, this was considered necessary, but now there are more semantic choices available like header, footer, nav, etc.  If you'd like to learn more about the HTML5 elements, I personally found HTML5 Doctor very helpful.

You have an image in your "nav" section, but it is unclear as to how it is related to navigation.  I don't know what it looks like so I used a picture of Bill Murray, which makes it look even more out of place.

Redundancy

Unless you're mixing ul and ol, there's no reason to specify ul in any selector containing li (eg. .nav ul li).  The li element must be a child of ul according to the rules of HTML.  The only exception is if you're trying to target a specific li of a specific ul in the case of nested lists (eg. .nav > ul > li).  Use the shortest selector necessary to target the element you want.

Navigation

There's nothing hacky about removing the margin the first element of your list.  If anything, you should be more concerned about your method for removing whitespace between your elements.  The font: 0 technique does not work on certain mobile browsers (plus it's terrible for not allowing you to use ems).

There are lots of ways to remove the undesirable whitespace between your inline elements.  Personally, I favor commenting it out.

Rather than setting paddings and unsetting them later, it is more efficient to only set the paddings on the elements you want:

.nav li ~ li {
    padding-left: 1em;
}

Though, if you're expecting your elements to wrap (eg. due to narrow viewport for mobile devices), negative margins would be better:

.nav ul {
    margin: 0 -.5em;
}

.nav li {
    margin: .5em;
}


Method has same erasure: How should I go about refactoring? Currently, in a utility class I have methods like so:

public static String getNextAvailableNumber(List<MyObject> currentObjects)
{
    int nextNumber = UtilClass.getNextAvailableNumberAsInteger(currentObjects, null);
    return UtilClass.formatNumber(nextNumber); //formats number as string
}

public static int getNextAvailableNumberAsInteger(List<MyObject> currentObjects, List<String> otherUnavailableNumbers)
{
    //returns nextNumber as int.
}

I now find myself wanting to add another method...

public static String getNextAvailableNumber(List<String> unavailableNumbers)
{
    int nextNumber = UtilClass.getNextAvailableNumberAsInteger(null, unavailableNumbers);
    return UtilClass.formatNumber(nextNumber);
}

You may notice that I would end up with a List argument in both places, causing a method has same erasure compiler error.

I'm considering the following solution..

//replace old getNextAvailableNumber() with generic version
public static String getNextAvailableNumber(List<?> objects)
{
    List<MyObject> objectList = new ArrayList<MyObject>();
    List<String> stringList = new ArrayList<String>();

    if (objects.get(0) instanceof MyObject)
    {
        objectList = (List<MyObject>) objects;
    }
    else if (objects.get(0) instanceof String)
    {
        stringList = (List<String>) objects;
    }
    int nextNumber = UtilClass.getNextAvailableNumberAsInteger(objectList, stringList);
    return UtilClass.formatNumber(nextNumber);
}

From what I've read, using instanceof is generally a sign that something can be done in a better way. Is there a better way? How should I go about refactoring this?

I strongly suspect that you have the wrong problem -- this feels like UtilClass.getNextAvailableNumberAsInteger is implemented incorrectly.  It's also not clear why you want to use the same method name when the argument has a completely different meaning.

That said... my recommendation would be to create a new function object.

class NumberGenerator {
    private List<MyObject> objects = null;
    private List<String> unavailableNumbers = null ;

    public NumberGenerator withObjects(List<MyObject> objects) {
        this.objects = objects;
        return this;
    }

    public NumberGenerator withUnavailableNumbers(List<String> unavailableNumbers) {
        this.unavailableNumbers = unavailableNumbers;
        return this;
    }

    public int getNext() {
        return UtilClass.getNextAvailableNumberAsInteger(objects, unavailableNumbers);
    }

    // OPTIONAL
    public static NumberGenerator newInstance () {
        return new NumberGenerator();
    }
}

Then your clients can use the NumberGenerator to get what they need, without having to worry about the details

int nextNumber = NumberGenerator.newInstance().withUnableNumbers(otherNumbers).getNext();

Other comments: it looks really odd that your "other unavailable numbers" are stored in a collection of String.  It looks somewhat odd that you are using a collection of MyObject to produce integers as well.  Why do the collections need to be List, as opposed to Collection or Iterable?

    if (objects.get(0) instanceof MyObject)

That's an awful idea; you're rooked if the array you are passed is empty.  If it's true that an empty list means you don't care what kind of list it is, that very strongly suggests that the entire interface is broken.

List<MyObject> objectList = new ArrayList<MyObject>();

If the list isn't supposed to by modified by the function, then you might be better off using Collections.EMPTY_LIST; it would help clarify your intent.


My grade calculator with special weighting My school has a special 65% formal 35% informal weighting, so I made something to find what your grade would be if you added or changed a grade.
http://retep-mathwizard.github.io/gradecalc/gradecalc.html

HTML:

<!DOCTYPE html>

    <title> Grade Calculator</title>
    <meta charset="utf-8"/>
    <link rel="stylesheet" type="text/css" href="gradecalc.css">

Grade Calculator

    <input id="ing" placeholder="Informal grades, seperate by commas.">

    <input id="fog" placeholder="Formal grades, seperate by commas.">

    <input type="radio" name='type' onclick="changeoldgradedisplay()" id='radio_new_grade' >New

    <input type="radio" name='type' onclick="changeoldgradedisplay()" id='radio_old_grade' checked>Change

    <input id="old_grade_input" display='none' placeholder="old grade">
    <input id="new_grade_input" placeholder="new grade">

    <input type="radio" name='gradetype' id='informal_radio'>Informal

    <input type="radio" name='gradetype' id='formal_radio' checked>Formal

    <button onclick="add_grades()" > Go </button>

CSS:

#fog, #ing {

  width: 300px;
}

JS:

function changeoldgradedisplay() {
    if (document.getElementById('radio_old_grade').checked) {
        document.getElementById('old_grade_input').style.display = 'block';
    } else {
        document.getElementById('old_grade_input').style.display = 'none';
    }
}
function get_average(items) {
    var sum = 0;
    var len = items.length;
    for (var i = 0; i < len; i++) {
        sum += parseFloat(items[i]);
    }
    return sum/len;
}

function add_grades() {
    var formal_grades = document.getElementById('fog').value.split(',');
    var informal_grades = document.getElementById('ing').value.split(',');
    var previous_grade = (get_average(formal_grades) * .65) + (get_average(informal_grades) * .35);

    document.getElementById('previous_grade').innerHTML = 'Estimate of previous grade - ' + previous_grade.toFixed(2);

    var is_formal = document.getElementById('formal_radio').checked;
    var gradelist = null;

    if (is_formal) {
        new_grade = document.getElementById("new_grade_input").value;
        if (document.getElementById('radio_old_grade').checked) {
            old_grade = document.getElementById("old_grade_input").value;
            formal_grades[formal_grades.indexOf(old_grade)] = new_grade
        } else {
            formal_grades.push(new_grade);
        }
    } else {
        new_grade = document.getElementById("new_grade_input").value;
        if (document.getElementById('radio_old_grade').checked) {
            old_grade = document.getElementById("old_grade_input").value;
            informal_grades[informal_grades.indexOf(old_grade)] = new_grade
        } else {
            informal_grades.push(new_grade);
        }
    }

    var new_grade = (get_average(formal_grades) * .65) + (get_average(informal_grades) * .35);
    document.getElementById('new_grade').innerHTML = 'Estimate of new grade - ' + new_grade.toFixed(2);
}

A couple of minor things:

fog and ing aren't very meaningful id names, they would be better as something like formal_grades and informal_grades instead.
This part of the code:

var is_formal = document.getElementById('formal_radio').checked;
var gradelist = null;

if (is_formal) {
    new_grade = document.getElementById("new_grade_input").value;
    if (document.getElementById('radio_old_grade').checked) {
        old_grade = document.getElementById("old_grade_input").value;
        formal_grades[formal_grades.indexOf(old_grade)] = new_grade
    } else {
        formal_grades.push(new_grade);
    }
} else {
    new_grade = document.getElementById("new_grade_input").value;
    if (document.getElementById('radio_old_grade').checked) {
        old_grade = document.getElementById("old_grade_input").value;
        informal_grades[informal_grades.indexOf(old_grade)] = new_grade
    } else {
        informal_grades.push(new_grade);
    }
}

repeats very similar code where the only difference is which array is used. Also, gradelist isn't used anywhere. You can avoid the repetition like this:

var is_formal = document.getElementById('formal_radio').checked;
var gradelist = is_formal ? formal_grades : informal_grades;

new_grade = document.getElementById("new_grade_input").value;
if (document.getElementById('radio_old_grade').checked) {
    old_grade = document.getElementById("old_grade_input").value;
    gradelist[gradelist.indexOf(old_grade)] = new_grade
} else {
    gradelist.push(new_grade);
}

by assigning the array that needs to be used to gradelist.

This works because when you assign an array (or any object) to a variable in javascript, it assigns the reference to the array instead of copying the value. For example, if formal_grades is assigned to gradelist they will both refer to the same array.


Camera Movement in a World Map using XNA and C# I built a game as a tutorial, but I have since added functionality to allow the player to move around on a world map which requires the camera to "move"  but I no longer like the look of the if statements.

is there some way that I can clean this up?

private static Rectangle scrollArea = new Rectangle(150, 150, 500, 300);

private void repositionCamera(GameTime gameTime, Vector2 moveAngle)
{
    float elapsed = (float)gameTime.ElapsedGameTime.TotalSeconds;
    float moveScale = playerSpeed * elapsed;

    if ((playerSprite.ScreenRectangle.X < scrollArea.X) && (moveAngle.X < 0))
    {
        Camera.Move(new Vector2(moveAngle.X, 0) * moveScale);
    }

    if ((playerSprite.ScreenRectangle.Right > scrollArea.Right) && (moveAngle.X > 0))
    {
        Camera.Move(new Vector2(moveAngle.X, 0) * moveScale);
    }

    if ((playerSprite.ScreenRectangle.Y < scrollArea.Y) && (moveAngle.Y < 0))
    {
        Camera.Move(new Vector2(0, moveAngle.Y) * moveScale);
    }

    if ((playerSprite.ScreenRectangle.Bottom > scrollArea.Bottom) && (moveAngle.Y > 0))
    {
        Camera.Move(new Vector2(0, moveAngle.Y) * moveScale);
    }
}

You can generalize bodies of ifs:

private static void MoveCamera(float x, float y, float moveScale)
{
    Camera.Move(new Vector2(x, y) * moveScale);
}

private void repositionCamera(GameTime gameTime, Vector2 moveAngle)
{
    float elapsed = (float)gameTime.ElapsedGameTime.TotalSeconds;
    float moveScale = playerSpeed * elapsed;

    if ((playerSprite.ScreenRectangle.X < scrollArea.X) && (moveAngle.X < 0))
    {
        MoveCamera(moveAngle.X, 0, moveScale);
    }

    if ((playerSprite.ScreenRectangle.Right > scrollArea.Right) && (moveAngle.X > 0))
    {
        MoveCamera(moveAngle.X, 0, moveScale);
    }

    if ((playerSprite.ScreenRectangle.Y < scrollArea.Y) && (moveAngle.Y < 0))
    {
        MoveCamera(0, moveAngle.Y, moveScale);
    }

    if ((playerSprite.ScreenRectangle.Bottom > scrollArea.Bottom) && (moveAngle.Y > 0))
    {
        MoveCamera(0, moveAngle.Y, moveScale);
    }
}

Also I suggest to extract all those conditions to properties with clear names like:

private static bool IsCameraOutOfLeft =>
    playerSprite.ScreenRectangle.X < scrollArea.X && moveAngle.X < 0;

private static bool IsCameraOutOfRight =>
    playerSprite.ScreenRectangle.Right > scrollArea.Right && moveAngle.X > 0;

...

Then finally the repositionCamera method will be

private void repositionCamera(GameTime gameTime, Vector2 moveAngle)
{
    float elapsed = (float)gameTime.ElapsedGameTime.TotalSeconds;
    float moveScale = playerSpeed * elapsed;

    if (IsCameraOutOfLeft || IsCameraOutOfRight)
    {
        MoveCamera(moveAngle.X, 0, moveScale);
    }

    if (IsCameraOutOfTop || IsCameraOutOfBottom)
    {
        MoveCamera(0, moveAngle.Y, moveScale);
    }
}

Also a method's name should be PascalCased – RepositionCamera.


Checking if a given string is in alphabetical order I wrote this function to check if a given string is in alphabetic order. How can I improve?

def alphacheck(x):
    '''This function checks if a given string is in Alphabetical order'''

    # Creating a variable to allow me to efficiently index a string of any length
    i = len(x) - 1
    # Using a recursive formula and checking for the base case when the string is of less than one character
    if len(x) <= 1:
        return True
    # This the recursive portion of the formula
    else:
        # This code checks if the last two characters are in alphabetical order
        # By utilizing the property that the letters of the alphabet increase in "value" accroding to python
        if x[i - 1] <= x[i]:
            # I remove the last letter of the string to reduce the probelm size
            x = x[:i]
            # I then repeat this on the smaller string until it reaches the base case or
            # Finds out the string is not in alphabetical order and returns False
            return alphacheck(x)
        else:
            return False

Sorting is idempotent, sorting an already sorted list leaves it unchanged.

So checking if a list is ordered equals checking if it equals itself when sorted:

def is_sorted_alphabetically(string):
    return string == ''.join(sorted(string))

If you do not want to have a \$O(NlogN)\$ runtime of sorting or you want to implement this yourself without using the sorted built-in, I suggest you avoid recursion and use the all and pairwise helper functions:

def is_sorted(iterable):
    return all(x1 <= x2 for x1, x2 in pairwise(iterable))

This is general and works for any iterable such as string but also lists of numbers or lists of anything of comparable (the type is Iterable[Comparable] -> bool)


PHP Register Form - OOP I'm trying to build my first object oriented system. I'm not sure that my register function below is the best way of approaching this within OOP.

class User {
  public $username;
  private $email;
  private $password;

  public function register() {
   //Check if the username input is set.
   if(isset( $_POST['username'] )) {
     //Assign the variables.
     $this->username = $_POST['username'];
     $this->email = $_POST['email'];
     $unhashed_pass = $_POST['password'];

     //Hash the password
     $this->password = password_hash($unhashed_pass, PASSWORD_BCRYPT);

     $sql = "INSERT INTO 'users' ('id', 'username', 'email', 'password') VALUES (NULL, '$this->username', '$this->email', '$this->password')";
     $results = mysql_query($sql);

     if($results) {
      //Query was successful
      echo "Success";
     } else {
      echo mysql_error();
     }
   }
  }
}

Please give me any feedback. I'm okay with PHP; I'm just trying to learn how to effectively use OOP.

When approaching object oriented programming, I always try to adhere to a few best practices:

Do one thing, and do it well
Don't Repeat Yourself (DRY)
Favor composition over inheritance

There are many more, but I've found these three practices to be the most useful in creating modular code. That being said, let's explore how your class compares.

Do one thing and do it well

Your User class is doing many things:

It is the Domain Model representing a User in the database along with some business logic (hashing the password)
It is getting data from the $_POST, coupling this class to the HTTP request layer of your application.
It is echo-ing a success flag, coupling this call to the standard output in PHP. Furthermore, if something goes wrong on insert, you can't handle that error. You should be throwing exceptions here instead. See Returning status codes from business layer for additional info
It is inserting data into the database.

Worse yet, you appear to have a SQL injection vulnerability since you are using double quotes and variable interpretation in PHP (see Little Bobby Tables for a humorous explanation). You want MYSQL Prepared Statements.

Don't Repeat Yourself

While no code is repeated inside this class, because it is doing multiple jobs it becomes difficult to reuse the code, which could lead to repetitive code later on.

Favor Composition over Inheritance

Your class doesn't inherit from anything, which is good. It doesn't really need to inherit from a super class. Composition (e.g. holding references to other objects specializing in a different but related task) won't help you here either because about 4 layers of an application have been mashed together into one class.

Splitting your class up into multiple classes

While this will seem like overkill at first, the following code will lay the ground work for a modular and testable application.

user.php

This is your domain model representing the database data. This will also contain methods that implement business logic, for instance, hashing the password. It is both data and behavior.

<?php

class User
{
    private const PASSWORD_BCRYPT = "...";

    private $username;
    private $email;
    private $password;

    public User()
    {
    }

    public User($username, $email, $password)
    {
        $this->set_username($username);
        $this->set_email($email);
        $this->set_password($password);
    }

    public function get_username()
    {
        return $this->username;
    }

    public function set_username($value)
    {
        $this->username = $value;
    }

    public function get_email()
    {
        return $this->email;
    }

    public function set_email($value)
    {
        $this->email = $value;
    }

    public function get_password()
    {
        return $this->password;
    }

    public function set_password($value)
    {
        $this->password = password_hash($value, User::PASSWORD_BCRYPT);
    }
}

users_repository.php

The "Users Repository" is a class whose sole purpose is to perform CRUD operations on the users table (Create, Read, Update and Delete). It implements an interface that you can use later on as a test harness.

<?php

interface IUsersRepository
{
    public function add(User $user);
}

class UsersRepository implements IUsersRepository
{
    private static $connection_string;

    public static function set_connection_string($value)
    {
        UsersRepository::$connection_string = $value;
    }

    private $connection;

    public UsersRepository()
    {
        // $connection = new mysqli(...);
    }

    public function add(User $user)
    {
        $statement = $this->connection->prepare('INSERT INTO "users" ("id", "username", "email", "password") VALUES (?, ?, ?, ?)');
        $statement->bind_param("id", NULL);
        $statement->bind_param("username", $user->get_username());
        $statement->bind_param("email", $user->get_email());
        $statement->bind_param("password", $user->get_password());

        if (!$statement->execute())
            throw new Exception("Execute failed: ({$stmt->errno}) {$stmt->error}");
    }
}

If the INSERT into the database fails, an Exception gets thrown. Failing to save data to the database is a hard stop. You really can't go any further, so an exception is the ideal way to communicate this catastrophic error (yes, failed database operations are catastrophic and your application should blow up in your face).

users_controller.php

This begins the Poor Man's MVC. The UsersController handles the HTTP request.

Notice that it has two constructors. The first one, which takes no arguments, instantiates UsersRepository as the object responsible for database operations. The second constructor accepts any object implementing the IUsersRepository interface, which is the interface implemented by UsersRepository. We'll use this later on as a test harness so we can write unit tests for this class without a database.

<?php

class UsersController
{
    private $repository;

    public UsersController()
    {
        $this->repository = new UsersRepository();
    }

    public UsersController(IUsersRepository $repository)
    {
        $this->repository = $repository;
    }

    public function register()
    {
        return new User();
    }

    public function register($params)
    {
        if (isset($params['username'])) {
            $user = new User($params['username'], $params['email'], $params['password']);
            $this->repository->add($user);

            return '/users/index.php';
        }
    }
}

users/create.php

Lastly, this file, which technically does too much here, is the actual "Create new User" page:

<?php

$controller = new UsersController();

if (isset($_POST))
{
    $redirect = $controller->create($_POST);
    header($redirect);
}
else
{
    $user = $controller->create(); ?>

<!DOCTYPE HTML>
<html>
    ...
    <body>
        <form method="post" action="./create.php">
            <input name="username" type="text" value="<?= $user->get_username() ?>">
            <input name="email" type="text" value="<?= $user->get_username() ?>">
            <input name="password" type="password" value="<?= $user->get_username() ?>">
            <button type="submit">Submit</button>
        </form>
    </body>
</html>

<?php } ?>

It instantiates a new UsersController. If the $_POST is set, then it passes that array into the $controller->create method, which performs the actual logic for extracting the user information from the HTTP request and inserting it into the database. Furthermore, it directs the application flow, for instance redirecting the user to another page upon successful insert (which prevents a double-POST of the same information as well).

The UsersController also has a method just called create() which takes no parameters and returns a User object, which gets displayed in the HTML form when the $_POST is not set.

Really, you'd want to take advantage of one of the many fantastic MVC frameworks out there for PHP for the users_controller.php and create.php files, because the MVC application should be responsible for routing an HTTP request to a controller, and then a specific PHP file for the view should provide the web page.

By splitting up your code into layers, you create a modular and testable application:

User - Business logic
UsersRepository - Data storage and retrieval
UsersController - Handle application flow
create.php - a not-so-perfect view

Testing your code

Notice that the UsersController has a constructor which accepts an instance of IUsersRepository. This is the interface implemented by UsersRepository. This allows you to write test code that uses a mock or stub user repository instead of going out to a database.

First, create the "mock users repository" class, which implements IUsersRepository

<?php

class MockUserRepository implements IUsersRepository
{
    public $added_users = array();

    public function add(User $user)
    {
        $this->added_users[] = $user;
    }
}

Next, create your unit test. In this case, I'm using PHPUnit:

<?php

class UsersControllerTest extends PHPUnit_Framework_TestCase
{
    public function addsANewUser()
    {
        $repository = new MockUserRepository();
        $controller = new UsersController($repository);
        $params = array(
            'username' => 'testable',
            'email' => 'test@example.com',
            'password' => 'abc123'
        );

        $redirect = $controller->create($params);

        $this->assertFalse(empty($repository->added_users)));
        $this->assertEquals('testable', $repository->added_users[0].get_username());
        $this->assertEquals('/users/index.php', $redirect);
    }
}

Now you can verify the behavior of your application in an automated and repetitive way. Covering your with unit tests allows you to refactor code or change business rules later on with confidence that you won't introduce bugs.


Can my Less CSS code be improved? I just started getting into the Less CSS framework and I am wondering if I'm doing it right and how the code can be improved if possible.

global.less

//variables
@themeRed: #cc1111;
@themeColor: @themeRed;
@marginBottom: 10px;
@contentRadius: 5px;

//mixins
.border-radius(@args) {
  -webkit-border-radius: @args;
  -moz-border-radius: @args;
  border-radius: @args;
}

//styles
body {
    font: 12px Arial;
    color: #333;
    background: #e9eaed;
}

a {
    &:link, &:visited {
        color: @themeColor;
        text-decoration: none;
    }
    &:hover {
        border-bottom: solid 1px;
    }
    &:active {
        //color: lighten(@themeColor, 10%);
    }
}

#topbar {
    background: @themeColor;
    width: 100%;
    height: 2px;
}

#container {
    width: 750px;
    margin: 0 auto;
    padding: 10px 0;
}

#header {
    display: table;
    height: 100px;
    width: 100%;
    margin-bottom: @marginBottom;
    .wrapper {
        vertical-align: middle;
        display: table-cell;
        text-align: center;
    }
}

#sitename {
    font-size: 50px;
    font-weight: bold;
    text-transform: uppercase;
    text-shadow: 2px 2px @themeColor;
    a {
        color: #fff;
        border: none;
        text-decoration: none;
    }
}

.section-heading {
    font-size: 1.1em;
    padding-left: 10px;
    margin-bottom: @marginBottom;
    text-transform: uppercase;
    font-weight: bold;
    color: @themeColor;
    border-bottom: 1px solid #e5e5e5;
    line-height: 29px;
}

p {
    line-height: 1.5em;
    margin-bottom: 1em;
}

.last {
    margin-bottom: 0;
}

#nav {
    position: absolute;
    width: 90px;
    text-align: right;
    margin-top: @contentRadius;
    li {
        border-right: 2px solid lighten(@themeColor, 30%);
        padding: 2px 5px 2px 0;
        margin-bottom: 2px;
        &:hover {
            border-color: @themeColor;
        }
        &.active {
            border-color: @themeColor;
            font-weight: bold;
        }
    }
}

.content {
    width: 518px;
    padding: 15px;
    margin: 0 auto;
    margin-bottom: @marginBottom;
    background: #fff;
    border: 1px solid;
    border-color: #e5e6e9 #dfe0e4 #d0d1d5;
    border-bottom: solid 2px @themeColor;
   .border-radius(@contentRadius);
    box-shadow: 0 1px 5px #d1d1d1;
    a {
        border-bottom: 0;
        &:hover {
            text-decoration: underline;
        }
    }
}

#advertisement {
    margin-bottom: @marginBottom;
    .adsbygoogle {
        display: block;
        margin: 0 auto;
    }
}

#footer {
    text-align: center;
    p {
        margin-bottom: 0;
    }
}

Looks just fine, I think. Only some really minor things come to mind

For font-family, it's good to always end with a generic font face style; in this case it'd be sans-serif. Arial is of course as "web-safe" as it gets, but still.
Why does #nav have a position: absolute rule, but no actual positioning (left, top, etc.) values? I imagine you're doing some JS positioning, but you should still have a fallback position (or just set position: absolute in the JS, so it's only there when needed).
You could consider using :last-of-type instead of explicitly giving elements a last class. Depends on what browsers you're targeting
Remove the a:active rule; you've already commented it out of the code.
You may want to do a box-shadow function that does the vendor-prefix magic, similar to your border-radius
You have one @themeColor, but also a bunch of different hard-coded colors (e.g. border colors on .content). So while it's easy to change the @themeColor, you'll probably still have to go through and change every other color to better match the new theme. If you can, use the color functions to derive the rest of the palette from @themeColor or make a more comprehensive list of color variables (even if they're only used once, it's still nice to be able to define the whole color scheme in 1 place).


Is this a correct knockout ViewModel setup? This is my first time using knockout. I'm reading/writing a JSON file with a fair bit of nested data. I had no problem creating the ViewModel to just write to the file, but now that I'm reading the data into my model also, I've had to write a few if/else statements to handle the different cases. Not sure if this is the right way to do it.

I've got a ViewModel with a function to add a new record, and to save the data. The 3 functions above are used for mapping the data.

function Survey(name, panes, tester)
{
  this.name = ko.observable(name);
  this.panes = ko.observableArray([new Pane(panes)]);
  this.tester = ko.observable(tester);
}

function Pane(panes)
{
  //var panesCount = panes.length;
  //console.log(panes[0].type);
  if (panes) {
    this.type = ko.observable(panes[0].type);
    this.head = ko.observable(panes[0].head);
    this.content = ko.observable(panes[0].content);
    this.options = ko.observableArray([new PaneOptions(panes[0].options)]);
  }
  else {
    this.type = ko.observable();
    this.head = ko.observable();
    this.content = ko.observable();
    this.options = ko.observableArray([new PaneOptions()]);
  }
}

function PaneOptions(options)
{
  //console.log(options);
  if (options) {
    this.data = ko.observable(options[0].data);
    this.target = ko.observable(options[0].target);
  }
  else {
    this.data = ko.observable();
    this.target = ko.observable();
  }
}

function SurveyViewModel()
{
  var self = this;
  self.surveys = ko.observableArray([]);

  $.getJSON("create/fetch", function(data) {
    //console.log(data.surveys[0].name);
    if (data) { // check if json file is empty
      for (var i=0; i < data.surveys.length; i++) {
        console.log(i);
        self.surveys.push(new Survey(data.surveys[i].name, data.surveys[i].panes, data.surveys[i].tester))
      }
    }
  });

  self.addSurvey = function ()
  {
    self.surveys.push(new Survey());
  };

  self.saveUserData = function()
  {
    var data_to_send = ko.toJSON(self);
    $.post("create/save", data_to_send, function(data) {
      console.log("Your data has been posted to the server!");
    });
  }

}

var vm = new SurveyViewModel();
ko.applyBindings(vm);

Here you can see the JSON representation:

{
    "surveys": [
        {
            "name": "My First Survey1",
            "panes": [
                {
                    "type": "question1",
                    "head": "Heading Text1",
                    "content": "multichoice1",
                    "options": [
                        {
                            "data": "Time",
                            "target": 2
                        }
                    ]
                }
            ],
            "tester": "default"
        },
        {
            "name": "My Second Survey2",
            "panes": [
                {
                    "type": "response2",
                    "head": "Heading Text2",
                    "content": "multichoice2",
                    "options": [
                        {
                            "data": "Time",
                            "target": 2
                        }
                    ]
                }
            ],
            "tester": "default2"
        },
        {
            "name": "My Third Survey3",
            "panes": [
                {
                    "type": "thanks3",
                    "head": "Heading Text3",
                    "content": "multichoice3",
                    "options": [
                        {
                            "data": "Time",
                            "target": 2
                        }
                    ]
                }
            ],
            "tester": "default3"
        }
    ]
}

Any input is greatly appreciated!

Overall, it looks really good and serves as a pretty great model of what Knockout models should look like, but, internally there are some strange things here like arrays that you are using [0] only on (this could be the result of poor data format via your backend web services) and some ambiguity between Panes, Pane, and PaneOptions.

(1) Use the guard or ternary operator to create defaults

This will help you avoid some of these logic blocks in all of your constructors.

(2) Fix ambiguity with singular and plural constructors (Planes, Plane)

The relationship between Panes and Pane are a little strange! In the Survey constructor, you are create a variable called panes (this.panes) and then instantiating and passing a new Pane to which you are passing an array of panes? A little bit recursive. In the Pane constructor I would not expect to take in panes in the arguments. I think your intention is for the Survey to create an array of panes, like this:

function Survey(name, panes, tester)
{
  var _this = this;

  // 1
  this.name = ko.observable(name || '');
  // 2
  this.panes = ko.observableArray();
  this.tester = ko.observable(tester);

  if(panes){
    panes.forEach(function(pane){
      _this.panes.push(new Pane(pane));
    });
  }
}

(3) Fix improper use of arrays with fixed [0] element

Why is Panes plural in the Pane constructor, if you are only using array item 0? I think the Pane constructor should just take in an parameter like options which would be used to return an individual pane:

function Pane(config)
{
  this.type = ko.observable(config.type);
  this.head = ko.observable(config.head);
  this.content = ko.observable(config.content);
  this.options = ko.observableArray([new PaneOptions(config.options)]);

  /*
   * You could return "this" but in JS that is the default behavior.
   */
}

Additionally, this will help you avoid JS errors when you might attempt to access a null array. For example, if panes in panes[0].options were null, you would have an error at pane[0] before you even got to the options property.

I am not sure why you are operating on options[0] here - at the point when you are instantiating a Pane, you should be passing in a config (see above) which has an options property, which will most likely be an object like { data: "", target: "" } since that is what you are expecting in the PaneOptions constructor:

function PaneOptions(options)
{
  // Setup defaults with ternary operator
  this.data = ko.observable(options ? options.data : '');
  this.target = ko.observable(options ? options.target : '');
}

Applicable original code with numbered callouts corresponding to items above

function Survey(name, panes, tester)
{
  // 1
  this.name = ko.observable(name || '');
  // 2
  this.panes = ko.observableArray([new Pane(panes)]);
  this.tester = ko.observable(tester);
}

// 2
function Pane(panes)
{
  if (panes) {
    // 3
    this.type = ko.observable(panes[0].type);
    this.head = ko.observable(panes[0].head);
    this.content = ko.observable(panes[0].content);
    this.options = ko.observableArray([new PaneOptions(panes[0].options)]);
  }
  else {
    this.type = ko.observable();
    this.head = ko.observable();
    this.content = ko.observable();
    this.options = ko.observableArray([new PaneOptions()]);
  }
}

function PaneOptions(options)
{
  if (options) {
    // 3
    this.data = ko.observable(options[0].data);
    this.target = ko.observable(options[0].target);
  }
  else {
    this.data = ko.observable();
    this.target = ko.observable();
  }
}


Simplify Oracle SQL: Treat null date as 'N' and non-null date as 'Y' This works, but nvl AND case seem redundant.
Basically, if null then 'N' else 'Y.

CHICKEN_HATCH_DATE is a DATE data type.

  select  stricken_chicken_id,
          case nvl(to_char(chicken_hatch_date),'N')
          when 'N' then 'N'
          else 'Y'
          end chicken_hatch_date
   from   stricken_chicken;

I'm not sure what's wrong with your code, but if you don't like all of those 'N''s would

  select  stricken_chicken_id,
          case when to_char(chicken_hatch_date) is null then
             'N'
          else
             'Y'
          end chicken_hatch_date
   from   stricken_chicken;

work for you?


String-splitting function This function was hard to write as a Clojure newbie, and I don't like the result.
Can you help me find a better (more readable) way to do it?

(defn split-seq
  "Splits a seq into blocks defined by start-fn and stop-fn.
  Returns a lazy seq of seqs"
  [start-fn stop-fn lines]
  (let [step (fn [c state]
                 (when-let [s (seq c)]
                   (if (stop-fn (first s))
                     (cons state (split-seq start-fn stop-fn (rest s) ))
                     (recur (rest s)
                        (if (start-fn (first s))
                            '()
                            (cons (first s) state))))))]
    (lazy-seq (step lines '()))))

(defn post-start? [l] (.startsWith l "#ENTRY_START"))
(defn post-end? [l] (.startsWith l "#ENTRY_END"))

(defn split-lines
    "Split a line-seq into entries based on #ENTRY_START and #ENTRY_END."
    [data]
    (split-seq post-start? post-end? data))

;
; Test
;
(def test-data [
    "Header line"
    "#ENTRY_START"
    "entry line 1 "
    "entry line 2"
    "#ENTRY_END"
    "This line should be filtered out"
    "#ENTRY_START Having data here shouldn't make a difference."
    "entry line 1 "
    "entry line 2"
    "#ENTRY_END"
    "This should be gone too"])

(split-lines test-data)

; yields (("entry line 2" "entry line 1 ") ("entry line 2" "entry line 1 "))
; The order of elements doesn't matter in my case because I'm making a map with this data

With Arthurs help, the final code looks like this:

(defn entry-seq [data]
    (let [[f r] (split-with #(not (post-end? %))
                  (rest (drop-while #(not (post-start? %)) data)))]
      (when (not-empty f) (lazy-seq (cons f (entry-seq2 r))))))

Still lazy and a lot more readable!

So the idea in this new approach is to write two expressions:

one that extracts the next expresstion:
(take-while #(not= "#ENTRY_END" %)
                            (rest (drop-while #(not= "#ENTRY_START" %) data)))
one that extracts everything after the next expression
(rest (drop-while #(not= "#ENTRY_END" %)
                        (rest (drop-while #(not= "#ENTRY_START" %) data))))

and then wrap them up into a lazy sequence:

first lets expand the test data to include some additional edge cases:

user> (def test-data [
       "Header line"
       "#ENTRY_START"
       "entry line 1 "
       "entry line 2"
       "#ENTRY_END"
       "not part of an entry"
       "also not part of an entry"
       "#ENTRY_START"
       "entry line 1 "
       "entry line 2"
       "#ENTRY_END"
       "footer1"
       "footer2"])

then wrap our two expressions into a function:

user> (defn entry-seq [data]
        (let [f (take-while #(not= "#ENTRY_END" %)
                            (rest (drop-while #(not= "#ENTRY_START" %) data)))
              r (rest (drop-while #(not= "#ENTRY_END" %)
                            (rest (drop-while #(not= "#ENTRY_START" %) data))))]
      (when (not-empty f) (lazy-seq (cons f (entry-seq r))))))
#'user/entry-seq

and we test it:

user> (take 4  (entry-seq test-data))
(("entry line 1 " "entry line 2") ("entry line 1 " "entry line 2"))


A good management of Java code I'm a beginner in JEE development.  I have just finished a web application that I'm going to deploy in a server.

It's a new level for me; it's not just a simple application that I can run for 10 min, etc.

My question is for the quality of code.  Of course we all know that a piece of code that works does not mean it's correct.  In my managed beans, I use @postConstruct, for example:

@PostConstruct
public void post() {
    listChaletss = chaletService.getAllChalet();

    for (Chalet it : listChaletss) {

        if (it.getType().equals("permanent")) {

            listChaletp.add(it);
        }
    }
    allPeriodes = periodeService.getAllPeriodes();
    // System.out.println("size periode" + allPeriodes.size());
    for (Chalet order : listChaletss) {
        order.setEditable(false);
    }
}

I'm wondering if someone can determine if we can face any memory problems using this application.  Some piece of code and any help from some Java experts will be welcomed.

Your variables listChaletss, listChaletp, and allPeriodes appear to be instance variables.  We can't tell whether you're leaking memory without further context — what is the expected lifetime of this object?  If this object only has a brief existence, then you probably don't have to worry about how it uses memory.  On the other hand, if it's a long-lived object, you have to be careful.

There is cause for concern, though.  You store the result of .getAllChalet() in instance variable listChaletss.  Why an instance variable?  Is listChaletss acting as some kind of cache?  If so, then the code would likely be:

public void post() {
    if (null == listChaletss) {
        listChaletss = chaletService.getAllChalet();
    }
    ...
}

… or better yet:

private Chalet[] getChalets() {
    if (null == listChaletss) {
        listChaletss = chaletService.getAllChalet();
    }
    return listChaletss;
}

public void post() {
    for (Chalet it : getChalets()) {
        ...
}

… or better yet, use a WeakReference.

On the other hand, if listChaletss is not meant to be a cache, then you should just make it a local variable so that you don't hang on to the result.  Unintentionally keeping a reference to the result leads to a "lingering" memory leak: you waste memory storing listChaletss, but you recover that memory the next time you call .post(), only to waste it again.

Similar considerations apply to your other two instance variables.


Reverse each group of k elements in a list Challenge
Given a list of numbers and a positive integer k, reverse the elements of the list, k items at a time.
Specifications

The first argument is a path to a file.
The file contains multiple lines.
Each line is a test case represented by a comma separated list of numbers and the number k, separated by a semicolon.
Print the comma separated list of numbers after reversing.
If the number of elements is not a multiple of k, then the remaining elements should be unaltered.

Sample Input

1,2,3,4,5;2
1,2,3,4,5;3

Sample Output

2,1,4,3,5
3,2,1,4,5

Source
My Solution
import java.io.File;
import java.io.FileNotFoundException;
import java.util.Scanner;

public class Solution {
    public static void main(String[] args) {
        if (args.length == 0) {
            System.out.println("No file path provided.");
            System.exit(1);
        }

        if (args.length >= 2) {
            System.out.println("Excessive arguments only the first will be considered.");
        }

        try (Scanner file = new Scanner(new File(args[0]))) {
            while (file.hasNextLine()) {
                System.out.println(reversed(file.nextLine()));
            }
        } catch (FileNotFoundException fnfe) {
            System.err.println("Could not find file.");
        }
    }

    private static String reversed(String line) {
        String[] helper = line.split(";");
        int k = Integer.parseInt(helper[1]);
        if (k == 0) {
            return helper[0];
        }
        return reversedByK(helper[0].split(","), k);
    }

    private static String reversedByK(String[] nums, int k) {
        StringBuilder result = new StringBuilder();
        for (int i = k - 1; i < nums.length; i += k) {
            for (int j = i; i - j <= k - 1; j--) {
                result.append(',').append(nums[j]);
            }
        }

        for (int i = nums.length - nums.length % k; i < nums.length; i++) {
            result.append(',').append(nums[i]);
        }
        return result.substring(1);
    }
}

The core of your solution doesn't suit my taste:

private static String reversedByK(String[] nums, int k) {
    StringBuilder result = new StringBuilder();
    for (int i = k - 1; i < nums.length; i += k) {
        for (int j = i; i - j <= k - 1; j--) {
            result.append(',').append(nums[j]);
        }
    }

    for (int i = nums.length - nums.length % k; i < nums.length; i++) {
        result.append(',').append(nums[i]);
    }
    return result.substring(1);
}

My main gripe is that it works at two different levels: it accepts an array as input, but returns a string.  It's a mild violation of the Single Responsibility Principle.  One consequence is that it uses the assumption that a comma should be used as a delimiter.  It would be more elegant if whichever function performed .split(",") were also responsible for doing String.join(",", …).

Also consider taking advantage of the Scanner more to help you with parsing the input.

private static final Pattern DELIM = Pattern.compile(";|\n|\r");

public static void main(String[] args) {
    …
    try (Scanner file = new Scanner(new File(args[0])).useDelimiter(DELIM)) {
        while (file.hasNextLine()) {
            System.out.println(reversedByK(file.next(), file.nextInt()));
            file.nextLine();
        }
    } catch (FileNotFoundException fnfe) {
        System.err.println("Could not find file.");
    }
}

private static String reversedByK(String list, int k) {
    String[] array = list.split(",");
    reversedByK(array, k);
    return String.join(",", array);
}

private static void reversedByK(String[] array, int k) {
    for (int i = 0; i + k <= array.length; i += k) {
        for (int a = i, b = i + (k - 1); a < b; a++, b--) {
            String swap = array[a];
            array[a] = array[b];
            array[b] = swap;
        }
    }
}

One bonus benefit of this swap-in-place solution is that there is no need for a second loop in reversedByK() to handle the remaining elements.


Reading, echoing, and returning integers in an array I was wanting some clarification on my code, as I am new to pointers in C. I am trying to return an array from a simple function just so I can understand exactly what it is that I'm doing.

Here is the code I have so far and, to me, it seems as if I haven't even used the pointers correctly.

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

int *myFunction(int size)
{
    int i;
    int *myArray = malloc(sizeof(int)*size);
    for(i=0;i<size;i++)
    {
        printf("Enter a number: ");
        scanf("%d",&myArray[i]);
    }
    for(i=0;i<size;i++)
    {
        printf("%d\n",myArray[i]);
    }
    return myArray;
}
int main()
{
    int *p;
    int size;
    printf("Enter the size: ");
    scanf("%d",&size);
    p = myFunction(size);
    return 0;
}

Someone suggested I turn my comment into an answer, so here goes.

First, I simply ran your code through astyle and added line numbers to make it easier to
reference in the text. Hopefully you can see from the output how a little bit of white-space
and consistent formatting/indentation can make your code clearer.

 1  #include <stdio.h>
 2  #include <stdlib.h>
 3
 4  int *myFunction(int size)
 5  {
 6      int i;
 7      int *myArray = malloc(sizeof(int) * size);
 8
 9      for (i = 0; i < size; i++)
10      {
11          printf("Enter a number: ");
12          scanf("%d", &myArray[i]);
13      }
14
15      for (i = 0; i < size; i++)
16      {
17          printf("%d\n", myArray[i]);
18      }
19
20      return myArray;
21  }
22
23  int main()
24  {
25      int *p;
26      int size;
27      printf("Enter the size: ");
28      scanf("%d", &size);
29      p = myFunction(size);
30      return 0;
31  }

First, full marks for lines 23 and 30: many people still code as if main can be declared void main() and
fail to return an exit code. This would be just a little better for line 30.

return EXIT_SUCCESS;

As I said above, there is nothing wrong with your use of pointers (which was your primary concern). But there are
other issues with the code if this was ever to be used for anything other than exploring concepts.

Now, let's deal with dynamic memory.

MORTAL suggested zeroing all dyamically allocated memory. That can be useful (for example, if you are going
to build a c-style string in the buffer) but is not necessarily a "good thing" in all cases. However, we can
clean up line 7 a little and get that for free while we're at it. In this line, you are allocating memory to
hold an array of int - rather than just a chunk of memory.

There's a function just for that purpose, so this

int *myArray = calloc(size, sizeof(int))

very clearly expresses your need for enough memory to store size ints.

It also sets the memory block to all zeroes before returning it.

As ChrisWue mentioned, size could be a very large number such that the call to malloc or calloc could fail.
So it's a best practice to always check the returned pointer to see if it is NULL (that's how memory allocation
routines report failure). So now we have:

int *myArray = calloc(size, sizeof(int))

if (!myArray)
{
    fprintf(stderr, "Failed to allocate memory to hold your input. Exiting.\n");
    exit(EXIT_FAILURE);
}

Note that I simply bail out (indicating an error via the exit status) rather than try and deal with an out of memory
condition. In fact, there's an even chance that the fprintf won't work if your system is this low on resources
but it's worth trying to say something rather than just present the user with a commmand line prompt.

Having got over that hurdle, you should always return any dynamically allocated memory to the pool as soon as you
are finished with it. Most operating systems will release all resources when a process ends ... but what you are
looking at in your code is a classic "memory leak".

As the caller does nothing with the returned array, I would actually make a few changes.

Change the return type of myFunction to void
Replace line 20 with free(myArray); to release the allocated memory
Delete line 25, and modify the call on line 29 not to save the (now non-existent) return value

Also, as a general rule, int is the wrong type for indexing arrays. First, it is signed and
myArray[-1] makes no sense. Second, and int may not be large enough to index all of memory.
The correct type to use is size_t, and indeed that's what calloc and malloc expect.

So your variables size and i should be size_t, and line 28 should change to read

scanf("%zu", &size);

where "%zu" is the correct format code for a size_t.

Your code now looks like this:

 1  #include <stdio.h>
 2  #include <stdlib.h>
 3
 4  void myFunction(size_t size)
 5  {
 6      size_t i;
 7      int *myArray;
 8
 9      myArray  = calloc(size, sizeof(int));
10      if (!myArray)
11      {
12          fprintf(stderr, "Failed to allocate memory to hold your input. Exiting.\n");
13          exit(EXIT_FAILURE);
14      }
15
16      for (i = 0; i < size; i++)
17      {
18          printf("Enter a number: ");
19          scanf("%d", &myArray[i]);
20      }
21
22      for (i = 0; i < size; i++)
23      {
24          printf("%d\n", myArray[i]);
25      }
26
27      free(myArray);
28  }
29
30  int main()
31  {
32      size_t size;
33      printf("Enter the size: ");
34      scanf("%zu", &size);
35      printf("You need to enter %zu numbers\n", size);
36      myFunction(size);
37      return EXIT_SUCCESS;
38  }

Note that I added line 35 to give a little more feedback.

There is a bigger problem with your use of scanf that goes beyond the scope of this review. Just be
aware that scanf (like gets()) is considered EVIL. Just as a simple example, try these
responses to your "Enter the size:" prompt (leave out the quotes)

"a very long string"
"10 followed by some text"
"-1"


Collaboration of the two functions through multithreading I'm going to turn them (multi-threading) to the first function after the end of the operation to notify the second function that has already finished work and the second function it received through multi-threading.

#include "stdafx.h"
#include <cstdint>
#include <iostream>
#include <mutex>
#include <queue>
#include <condition_variable>
#include <fstream>
#include <sstream>
#include <algorithm>
std::mutex mutex;
std::queue<std::string> queue;
std::condition_variable conditionVariable;

void ProduceData()
{
   const char* pathToFile = "D:\\text.txt";
   std::ifstream stream(pathToFile);
   if (!stream)
   {
      std::cout << "Can not open file\n";
      return;
   }

   const std::uint8_t maxWordCount = 5;
   std::string word;
   while (stream >> word)
   {
      std::lock_guard<std::mutex> lockGuard(mutex);
      for (std::uint8_t count = 0; (count < maxWordCount) || (count != '\0'); ++count)
        queue.push(word);
      conditionVariable.notify_one();
   }
 }

 void ConsumeData()
 {
   while (true)
   {
    std::unique_lock<std::mutex> uniqueLock(mutex);
    conditionVariable.wait(uniqueLock, [] {return !queue.empty(); });
    while (!queue.empty())
    {
        const std::string& str = queue.front();
        std::size_t numVowels = std::count_if(str.begin(), str.end(), isVowel);
        std::cout << str << "-" << numVowels;
        queue.pop();
    }
    uniqueLock.unlock();
   }
}

int main()
 {
std::thread t1(ProduceData);
std::thread t2(ConsumeData);
t1.join();
t2.join();

  return 0;
 }

Your consumer thread locks out the producer for extended periods of time.

void ConsumeData()
 {
   while (true)
   {
    std::unique_lock<std::mutex> uniqueLock(mutex);
    conditionVariable.wait(uniqueLock, [] {return !queue.empty(); });
    while (!queue.empty())
    {
        // DO WORK
        queue.pop();
    }
    uniqueLock.unlock();
   }
}

I would rather see the consumer grab a single item from the queue then release the queue (so the producer can continue to add work while the consumer is processing a single job.

void ConsumeData()
{
    while (true)
    {
      {
        std::unique_lock<std::mutex> uniqueLock(mutex);
        conditionVariable.wait(uniqueLock, [] {return !queue.empty(); });

        // Note: Don't need to test queue.empty()
        //       If it is empty then the thread is re-queued on the
        //       condition variable it is only released when the test
        //       above is true.
        work = queue.front();
        queue.pop();
      }
      // You have the work item so you don't need the lock.
      // So out here you work on the item you just retrieved from
      // queue. When done you loop back around to get more work.

      // DO WORK
    }
  }
}

Your producer adds lots of items to the queue. But only signals once. You should signal once for every item added to the queue.

  for (std::uint8_t count = 0; (count < maxWordCount) || (count != '\0'); ++count) {
    queue.push(word);
    conditionVariable.notify_one();
  }

As a side note:

An indent of 2 is a bit on the low side (acceptable by some). But in my opinion makes the code too cluttered.


Decorator pattern with a builder in a booking application I'm working on a booking application and am in the process of porting some legacy code to a more SOLID and testable architecture. Right now I'm working on the process of canceling bookings. Over the years, we have changed the steps involved here. The core has been the same (update booking entity status, make the timeslot open again, etc), but we've been adding and removing some other steps, and we do some steps depending on where the cancellation is made from (support UI, customer UI etc).

My current solution involves the decorator pattern for dynamically adding extra steps to the cancellation process. In order to hide the concrete classes responsible for those steps, as well as the fact that this pattern is used at all from client code, I've wrapped this up in a builder. I now wonder if there are some pitfalls I should look out for here.

public class OrderCancellationProcessBuilder
{
    private readonly ITimeProvider _timeProvider;
    private IOrderProcessor _cancellationProcessor;

    public OrderCancellationProcessBuilder(ITimeProvider timeProvider, string cancelationMessage)
    {
        _timeProvider = timeProvider;
        _cancellationProcessor = new CancelBookingsAndRevokePayments(cancelationMessage);
    }

    public IOrderProcessor Build()
    {
        return _cancellationProcessor;
    }

    public OrderCancellationProcessBuilder SendConfirmationEmails(IConfirmationEmailSender emailSender)
    {
        _cancellationProcessor = new SendConfirmationEmails(
            _cancellationProcessor,
            emailSender,
            _timeProvider);
        return this;
    }

    public OrderCancellationProcessBuilder RemovePendingMessages(IPendingMessagesService pendingMessagesService)
    {
        _cancellationProcessor = new RemovePendingMessages(_cancellationProcessor, pendingMessagesService);
        return this;
    }
}

The order processor can later on be consumed by the client code by something like someOrder.Accept(cancellationProcessor);. SendConfirmationEmails and others are the decorators for the order processor. The decoration steps are being hidden behind the builder pattern in a 1:1 way (although this could of course change).

If I were to voice any concern I might have at this stage, it is that it might be overly complicated. But then again, I think this way of thinking might come partly from the fact that the current code LOOKS pretty simple until you look into things and realize how much global state and newing of dependencies there are. If this looks complicated, it might just be because it IS complicated, and this pattern just makes it more visible.

I really think something like this solution could work to make it a lot easier to add/remove steps without breaking existing code, but I wonder if there's a simpler way

You can possibly try Domain Events. Or just Event Broker patterns?

What I usually use is something like this:

static class Event
{
   public static void Raise<T>(this T e) ...
}

interface IHandler<T>
{
   void Handle<T>(T e);
}

So I can define in my code:

class OrderCancelled
{
   public OrderCancelled(int orderId) ...
   public int OrderId { get; }
}

class MoneyReturn : IHandler<OrderCancelled> ...
class LastOfferSender : IHandler<OrderCancelled> ...
etc...

My IoC container was setup to construct and invoke a transient instances of all implementations of IHandler<T> on a T event raising.

new OrderCancelled(id).Raise();

You can chain events.


PHP Status class : pattern or anti-pattern I want to abstract some code which does a bunch of checks and returns a boolean - since I am using the business logic in several places. But I also want to get back the reason why, if it was returned FALSE. I have this in mind...
class ContactCanBeContactedCheck
{
    public $not_contactable_reason;

    const REASON_DECEASED = 0;
    const REASON_OPTED_OUT = 1;

    public function __invoke(Contact $contact)
    {
        // Deceased
        if ($contact->is_deceased) {
            $this->not_contactable_reason = static::REASON_DECEASED;
            return FALSE;
        }

        // Opted out
        if ($contact->opt_out) {
            $this->not_contactable_reason = static::REASON_OPTED_OUT;
            return FALSE;
        }

        return TRUE;
    }
}

I would then call the code like so...
$contact_can_be_contacted_check = new ContactCanBeContactedCheck;
$contact_can_be_contacted_check($contact);
$reason = $contact_can_be_contacted_check->not_contactable_reason;

I think this must be a well-trodden path: needing a decision, plus a reason for said decision. Is this a suitable approach or is there another more widely accepted?

I want to abstract some code...

What is it that you actually want to abstract?
Unless you are going to pass an instance of ContactCanBeContactedCheck class to someone who expects a callable, you shouldn't implement it through __invoke method.
My first though was, why not make it a method of the Contact class?
class Contact
{
  public const NOT_CONTACTABLE_REASON_DECEASED = 1;
  public const NOT_CONTACTABLE_REASON_OPTED_OUT = 2;

  // ...

  public function getNotContactableReason(): ?int
  {
        if ($this->is_deceased) {
            return static::NOT_CONTACTABLE_REASON_DECEASED;
        }

        if ($this->opt_out) {
            return static::NOT_CONTACTABLE_REASON_OPTED_OUT;
        }

        return null;
  }
}

Now it is contained in the class that actualy has the data to tell the reason. Encapsulation is one of the fundamental principles of OOP.
Still, if you need a callable, you can now do just
$callback = fn (Contact $contact): ?int => $contact->getNotContactableReason();

Also your method is trying to do 2 things

tell if the contact is contactable
tell the reason why it is not contactable

My method only tells the reason why it is not contactable, and null if there is no reason, because the contact is contactable. Using this approach you are actually able to tell both those things using just this one method as well. But it is not intent to do two things like in your method. It's just a side effect. And that makes a big difference.
You could add another method that would just tell whether it is contactable regardless of reason, but it is now easy enough to tell from the first method as well, so the second method would be just sugar for $this->getNotContactableReason() === null.
Notice I started the constants at 1, not 0. Zero is falsey, just like null.
And using zero could then easily lead to mistakes if you use weak comparision.
if ($reason) {...}

vs.
if ($reason !== null) {...}

I am wondering though, what you are actually doing with the class and the resulting reason. If the reason ends up in a switch or an if, then the entire method is useless and you should branch your code based directly on the $is_deceased and $opt_out properties.
Those 3 lines of usage are far from enough to tell what you really need. If I had more, I could target my review much better.


Left or right padding a string with the specified character Can I improve performance of below method? Is there anything I need to optimize or any redundancy I need to remove to make it more efficient? It's a simple method used to pad strings by left or right.

public static String padString(String input, boolean leftPad, char padChar, int maxLength) {
    String output = CommonBaseConstants.EMPTY_STRING;
    final int defaultSize = 8;
    String filledStr = null;
    if (CommonBaseConstants.CHAR_ZERO == padChar) {
        filledStr = StringUtils.getZeroes(defaultSize);
    }
    else if (CommonBaseConstants.CHAR_SPACE == padChar) {
        filledStr = StringUtils.getSpaces(defaultSize);
    }

    if (null != filledStr && null != input) {
        if (maxLength > input.length()) {
            StringBuilder value = new StringBuilder();
            int remainder = maxLength % defaultSize;
            if (0 != remainder) {
                value.append(filledStr, 0, remainder);
            }

            int loopSize = (maxLength - remainder) / defaultSize;
            for (int index = 0; index < loopSize; index++) {
                value.append(filledStr);
            }

            output = value.toString();
            if (leftPad) {
                output = output.substring(0, maxLength - input.length()) + input;
            }
            else {
                output = input + output.substring(0, maxLength - input.length());
            }
        }
        else {
            output = input;
        }
    }

    return output;
}

The API you expose is very weird. Consider the following call:

padString("foo", true, '\t', 5);

The result being "" is not really obvious.

Let's just rewrite this from scratch and then compare it to the existing code.

First we're going to have some requirements:

padString should not make the input smaller.
The code you present correctly deals with that, albeit only comparatively late in the process.
padString should expose left and right padding
padString should allow customization of the character used for padding.

With this we can start pulling changes into your code.

The easiest of these to fulfil (incidentally simplifying your code) is the first requirement:

    if (input.length() >= maxLength) {
        return input;
    }

Note how I removed the assignment to output and instead directly returned the value to simplify the control stucture.

The next requirement is to enable leftPadding and rightPadding. The cleanest way to deal with that is to create separate methods for that, avoiding the boolean leftPad argument turning the method into a state automaton.

Instead I suggest you expose leftPad and rightPad. If you must keep using the API you present here, (which I highly doubt), you can implement padString as:

return leftPad ? leftPad(input, padChar, maxLength) : rightPad(input, padChar, maxLength);

Now that we have that sorted we want to deal with the actual implementation.

Consider the following simplification:

int padSize = maxLength - input.length();
// generate an "infinite" stream of padChar, limit it to the padSize and collect as String
String padding = Stream.generate(() -> padChar).limit(padSize).collect(Collectors.joining());
StringBuilder builder = new StringBuilder(padding);
if (leftPad) {
    // padding is supposed to be on the left, so append the input
    builder.append(input);
} else {
    // padding is supposed to be on the right, put input before
    builder.insert(0, input);
}
assert (builder.length() == maxLength);
return builder.toString();

At this point you should notice something that can be further simplified. The StringBuilder is just a glorified string concatenation:

return leftPad ? padding + input : input + padding;

Note that the padding generation can be slightly simplified using Arrays.fill:

char[] paddingChars = new char[padSize];
Arrays.fill(paddingChars, padChar);
String padding = new String(paddingChars);

If you want to enforce some more raw performance you could even go so far as to use System.arraycopy, though I have not done any performance testing.
Note also that I'm reasonably sure this does not correctly handle unicode codepoints outside of the Basic Multilingual Plane (BMP)...

    if (input.length() >= maxLength) { return input; }
    char[] result = new char[maxLength];
    int padSize = maxLength - input.length();
    if (leftPad) {
        Arrays.fill(result, 0, padSize, padChar);
        input.getChars(0, input.length(), result, padSize + 1);
    } else {
        input.getChars(0, input.length(), result, 0);
        Arrays.fill(result, input.length(), maxLength - 1, padChar);
    }
    return new String(result);

Sidenote: the existing code also does not correctly handle unicode codepoints outside of the BMP. I assume you were not aware of that because most people are not. The gist of it is: Some "characters" require more than 16 bits to be uniquely identifiable. These supplementary characters are ... weird around length and a handful of other String functions.


Update PHP function GetSQLValueString The old function was:

<?php
function GetSQLValueString($theValue, $theType, $theDefinedValue = "", $theNotDefinedValue = "")
{
    $theValue = (!get_magic_quotes_gpc()) ? addslashes($theValue) : $theValue;

    switch ($theType) {
        case "text":
          $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
          break;
        case "long":
        case "int":
          $theValue = ($theValue != "") ? intval($theValue) : "NULL";
          break;
        case "double":
          $theValue = ($theValue != "") ? "'" . doubleval($theValue) . "'" : "NULL";
          break;
        case "date":
          $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
          break;
        case "defined":
          $theValue = ($theValue != "") ? $theDefinedValue : $theNotDefinedValue;
          break;
    }
    return $theValue;
}
?>

I have changed in this way:

<?php
function GetSQLValueString($theValue, $theType, $theDefinedValue = "", $theNotDefinedValue = "")
{
    $conn = new mysqli('localhost', 'username', 'password', 'dbname');

    $theValue = (!get_magic_quotes_gpc()) ? addslashes($theValue) : $theValue;

    $theValue = $conn->real_escape_string($theValue);
    switch ($theType) {
        case "text":
          $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
          break;
        case "long":
        case "int":
          $theValue = ($theValue != "") ? intval($theValue) : "NULL";
          break;
        case "double":
          $theValue = ($theValue != "") ? "'" . doubleval($theValue) . "'" : "NULL";
          break;
        case "date":
          $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
          break;
        case "defined":
          $theValue = ($theValue != "") ? $theDefinedValue : $theNotDefinedValue;
          break;
    }
    return $theValue;
}
?>

Is what I've made correct?

Can I remove get_magic_quites_gpc() if I use PHP version 5.3.2?

PHP is not my area of expretise, so just some generic notes:

The function create a new MySQL connection on every call which could be a bottleneck in your application. You could use persistent connections with a p: prefix but others usually suggest PDO as a better alternative of database handling.
The $the prefix on most of the variables seem redundant (it doesn't add too much to them), I'd avoid it.
If you intended to return the original value when its not text, long, int etc. you should make it explicit with a default case inside the switch statement. If not, throw an exception in the default case and sign the error. (The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)
case "text" and case "date" contains the same logic, you could use fall through there too.


Calculate the Fibonacci sequence using bash I would like your opinion on my shell script that prints the Fibonacci sequence.

#!/bin/bash

first=0
second=1
third=1
echo -n "Term: "
read term

fub=$((second+third))
echo $second
echo $third
echo $fub

for ((i=1; i<term; i++))
do
  third=$((fub+third))
  echo $third
  fub=$((third+fub))
  echo $fub
done

I tried to calculate the Fibonacci sequence in the simplest way (without the use of recursive functions).

What does "Term" mean? I'd prefer to see 5 numbers if the user entered 5.

#!/bin/bash

echo -n 'Term: '
read term

i=0
j=1
for (( c=0; c<term; c++ )) ; do
    (( n=i+j ))
    (( i=j ))
    (( j=n ))
    echo $i
done