Number base manipulation in Haskell I am picking up good Haskell practices!

In this instance, I wanted to pick a file which contains a title in the first line and then every line is a string.

Each string represents a number in base equal to the number of distinct characters that appear on the string, but we do not know what characters correspond to each digit.

We want to find out the difference between the max and min number a string could represent, with the added restriction that a number cannot have leading 0s.

I feel like my code is quite complicated for such a simple task. Can I get any pointers on how to improve it so its more readable and elegant?

import qualified Data.Map as Map
import Data.List
import Data.Maybe

main = do
    contents <- (tail . lines) <$> readFile "submitInput" :: IO [String]
    let input = contents :: [String]
    let result = max_range <$> input
    let decorated_result = ["Case #" ++ show i ++ ": " ++ show s | (i,s) <- zip [1..] result]
    writeFile "output.txt" $ unlines $ decorated_result

-- Computes the difference between the max and min possible decoding
max_range word = max_decoding word - min_decoding word

-- Computes the minimal possible decoding of word
min_decoding :: String -> Integer
min_decoding word =
    let number = fix_decoding $ min_aux word (Map.empty :: Map.Map Char Integer) 0
    in base_conversion (toInteger . length . nub $ number) $ catMaybes number

-- The decodification cannot have leading zeros
fix_decoding :: [Maybe Integer] -> [Maybe Integer]
fix_decoding [] = []
fix_decoding (Just 0:ds) = Just 1 : fix_decoding ds
fix_decoding (Just 1:ds) = Just 0 : fix_decoding ds
fix_decoding (d:ds) = d : fix_decoding ds

min_aux :: [Char] -> Map.Map Char Integer -> Integer -> [Maybe Integer]
min_aux [] _ _ = []
min_aux (c:cs) map next
    | c `Map.member` map    = Map.lookup c map : min_aux cs map next
    | otherwise             = Just next : min_aux cs (Map.insert c next map) (next + 1)

-- Computes the maximal possible decoding of word
max_decoding :: String -> Integer
max_decoding word =
    let number = max_aux word (Map.empty :: Map.Map Char Integer) ((toInteger . length . nub $ word)-1)
    in base_conversion (toInteger . length . nub $ number) $ catMaybes number

max_aux :: [Char] -> Map.Map Char Integer -> Integer -> [Maybe Integer]
max_aux [] _ _ = []
max_aux (c:cs) map next
    | c `Map.member` map    = Map.lookup c map : max_aux cs map next
    | otherwise             = Just next : max_aux cs (Map.insert c next map) (next - 1)

-- Interprets a list of digits as a number in a given base
base_conversion :: Integer -> [Integer] -> Integer
base_conversion base []     = 0
base_conversion base (d:ds) = d * base ^ (length ds) + base_conversion base ds

Type signatures and naming

While it's good practice to use type signatures on top-level bindings, it's usually not necessary in expressions unless type classes are involved. For example, the two following lines don't need type signatures:

contents <- (tail . lines) <$> readFile "submitInput" :: IO [String]
let input = contents :: [String]

readFile always returns IO String, and tail . lines always changes a String into a [String]. Furthermore, input = contents is a nop, so it's not clear why it's there.

Next, you usually use camelCase instead of snake_case in names in Haskell and hide the functionality in local bindings if you don't want to reuse functions, e.g.

min_decoding :: String -> Integer
min_decoding word =
    base_conversion (toInteger . length . nub $ number) $ catMaybes number
  where
    number = fix_decoding $ min_aux word Map.empty 0
    min_aux = ...

Feel free to use let … in … instead. By the way, max_range is missing a type signature.

Recursion and Horner's method

Your base_conversion isn't really optimal. We can use Horner's method to get rid of length. For this we need a helper:

baseConversion :: Num a => a -> [a] -> a
baseConversion base = go 0
  where
    go acc []     = acc
    go acc (d:ds) = go (acc * base + d) ds

You can verify that this yields the same result:

baseConversion 10 [1,2,3,4,5]
   = go 0 [1,2,3,4,5]
   = go (0  * 10 + 1) [2,3,4,5]
   = go (1  * 10 + 2) [3,4,5]
   = go (12 * 10 + 3) [4,5]
   = go (123 * 10 + 4) [5]
   = go (1234 * 10 + 5) = 12345

But there's already a function for this, foldl':

baseConversion :: Num a => a -> [a] -> a
baseConversion base = foldl' (\acc x -> acc * base + x) 0

Either way, we got rid of the ^ (length ds) part.

Don't repeat yourself

Both min_decoding and max_decoding look very similar, and so do max_aux and min_aux.

We're repeating a lot of logic here. So instead, let us use a single function:

toDigit :: (Integer -> Integer) -> [Char] -> Map.Map Char Integer -> Integer -> [Maybe Integer]
toDigit _    []     _   _    = []
toDigit step (c:cs) m next =
  | c `Map.member` m    = Map.lookup c m : toDigit cs m next
  | otherwise           = Just next : toDigit cs (Map.insert c next m) (step next)

Now

min_aux = toDigit (+1)
max_aux = toDigit (subtract 1)

Use appropriate types

But toDigit is still not optimal. We never return Nothing in our list. Therefore, there is no reason to use [Maybe Integer] as a result type:

toDigit :: (Integer -> Integer) -> [Char] -> Map.Map Char Integer -> Integer -> [Integer]
toDigit _    []     _   _    = []
toDigit step (c:cs) m next = case Map.lookup c m of
  Just digit -> digit : toDigit step cs map next
  Nothing    -> next  : toDigit step cs (Map.insert c next m) (step next)

While we're at it, you probably want to replace Integer with Int for the digits.


Add two digit strings and return the result as string I haven't done C in a while.
This looks like a simple problem to start.

Leetcode problem 415

  Given two non-negative integers num1 and num2 represented as string, return the sum of num1 and num2.
  …
  Note: [1. … 2. … 3. … ] 4. You must not use any built-in BigInteger library or convert the inputs to integer directly.

It is working but I would like the comment on the style and being proper C code.

char * addStrings(char * num1, char * num2){
    int len1 = strlen(num1);
    int len2 = strlen(num2);

    int result_len = len1 > len2? len1: len2;
    result_len += 1;

    char *result = malloc(result_len+1);

    char *p1 = num1 + len1-1;
    char *p2 = num2 + len2-1;

    char *rp = result + result_len;

    *rp = '\0';
    rp--;

    int carry = 0;
    char zero='0';
    int digit;
    int d1;
    int d2;

    while (len1 || len2) {
        if (len1) {
            d1 = (*p1)-zero;
            p1--;
            len1--;
        }
        if (len2) {
            d2 = (*p2)-zero;
            p2--;
            len2--;
        }
        int tmp = d1 + d2 + carry;
        digit = tmp % 10;
        carry = tmp / 10;
        *rp = digit+zero;
        rp--;
        d1 = 0;
        d2 = 0;
    }

    if (carry) {
        *rp = carry+zero;
    } else {
        rp++;
    }
    return rp;
}

Always Initialize Variables

There is one possible bug in the code, the variables d1 and d2 assume that both num1 and num2 have values, this may not be true. The C programming language does not initialize variables by default like some other languages, especially local variables. All of the variables should be initialized before the loop.

This also creates multiple paths through the code if either num1 or num2 do not have a value and error checking is added.

char * addStrings(char * num1, char * num2){
    int len1 = strlen(num1);
    int len2 = strlen(num2);

    if (!len1 && !len2)
    {
        fprintf(stderr, "Neither string has an integer value.");
        return "0";
    }
    else
    {
        if (!len1)
        {
            return num2;
        }
        if (!len2)
        {
            return num1;
        }
    }

    ...

    return rp;
}

Complexity

With the added error check the function addStrings() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

  that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

Probably the best thing to do here is to break addStrings() into 2 functions, one that does the error checking, and one that calculates the value of the string. The addStrings() function  should do the error checking itself, and then call a function to calculate the value of the string.

Missing Headers

In addition to the missing header files noted by vnp an include of the header file string.h is necessary to allow strlen() to compile, at least on my computer with my C compiler (Windows 10, Visual Studio 2019).

DRY Code

There is a programming principle called the Don't Repeat Yourself Principle sometimes referred to as DRY code. If you find yourself repeating the same code multiple times it is better to encapsulate it in a function. If it is possible to loop through the code that can reduce repetition as well.

This code is repetitive and the manipulation of the pointers and counters isn't necessary at this point.

    if (len1) {
        d1 = (*p1)-zero;
        p1--;
        len1--;
    }
    if (len2) {
        d2 = (*p2)-zero;
        p2--;
        len2--;
    }

It might be better to just check if pN has a value and use what it points to if it does, change the pointers and the counters at the end of the loop.

    if (p1) {
        d1 = (*p1) - zero;
    }

    if (p2) {
        d2 = (*p2) - zero;
    }

Scope of Variables

The integer variables digit, d1 and d2 are never used outside the loop so they can be declared within the loop itself. Only declare the variables when they are needed.

In Summation

As noted by vnp there is a memory leak.

An example of what how I would write the code:

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

static const unsigned char zero = '0';

char addCharacters(char *p1, char *p2, int *carry)
{
    int digit = 0;
    int d1 = 0;
    int d2 = 0;

    if (p1) {
        d1 = (*p1) - zero;
    }

    if (p2) {
        d2 = (*p2) - zero;
    }

    int tmp = d1 + d2 + *carry;
    digit = tmp % 10;
    *carry = tmp / 10;

    return digit + zero;
}

char* calculateStringValue(char *result, char num1[], int len1, char num2[], int len2, int result_len)
{
    char *rp = result + result_len;
    *rp = '\0';
    rp--;

    int carry = 0;
    char *p1 = num1 + len1-1;
    char *p2 = num2 + len2-1;

    while (len1 || len2)
    {
        *rp = addCharacters(p1, p2, &carry);
        --len1;
        --len2;
        p1 = (len1 > 0) ? --p1 : NULL;
        p2 = (len2 > 0) ? --p2 : NULL;
        --rp;
    }

    if (carry) {
        *rp = carry + zero;
    } else {
        rp++;
    }

    return rp;
}

char * addStrings(char * num1, char * num2){
    int len1 = strlen(num1);
    int len2 = strlen(num2);

    if (!len1 && !len2)
    {
        fprintf(stderr, "Neither string has an integer value.");
        return "0";
    }
    else
    {
        if (!len1)
        {
            return num2;
        }
        if (!len2)
        {
            return num1;
        }
    }

    int result_len = len1 > len2? len1: len2;
    result_len += 1;    // allow for carry.

    char *result = malloc(result_len + 1);
    if (result == NULL)
    {
        fprintf(stderr, "malloc failed in addStrings\n");
        return NULL;
    }

    return calculateStringValue(result, num1, len1, num2, len2, result_len);
}

int main() {
    char num1[] = "2048";
    char num2[] = "9999";
    printf("AddStrings returns %s\n", addStrings(num1, num2));
    return 0;
}


Delegate that selects a writable property on a generic In the following contrived example, I have a collection of PropertyManagers that each contains selector and assigner delegates to read from and write to a property on the generic type. It feels clunky for lots of reasons, not least of which is that both delegates are basically there to serve the same purpose: To identify the property on the generic that I want to read from then write back into.

using System;
using System.Collections.Generic;
using System.Linq;
using NUnit.Framework;

namespace Tests
{
    [TestFixture]
    public class ReadWritePropertySelectorTests
    {
        public void Round<T>(IEnumerable<T> collection,
            params PropertyManager<T>[] propertyManagers)
        {
            foreach (var item in collection)
            {
                foreach (var propertyManager in propertyManagers)
                {
                    var valueToRound = propertyManager.Selector(item);
                    propertyManager.Assigner(item, Math.Round(valueToRound, 1));
                }
            }
        }
        public class PropertyManager<T>
        {
            public Func<T, decimal> Selector { get; set; }
            public Action<T, decimal> Assigner { get; set; }
        }
        public class Combatant
        {
            public decimal Attack { get; set; }
            public decimal Defense { get; set; }
            public decimal Health { get; set; }
        }
        [Test]
        public void Test()
        {
            // Arrange
            var combatants = new[]
            {
                new Combatant { Attack = 33.12M, Defense = 12.771M, Health = 60.1181M }
            };

            // Act
            Round(combatants,
                new PropertyManager<Combatant>
                {
                    Selector = c => c.Attack, Assigner = (c, v) => c.Attack = v
                },
                new PropertyManager<Combatant>
                {
                    Selector = c => c.Defense, Assigner = (c, v) => c.Defense = v
                },
                new PropertyManager<Combatant>
                {
                    Selector = c => c.Health, Assigner = (c, v) => c.Health = v
                });

            // Assert
            var array = combatants.ToArray();
            Assert.AreEqual(33.1M, array[0].Attack);
            Assert.AreEqual(12.8M, array[0].Defense);
            Assert.AreEqual(60.1M, array[0].Health);
        }
    }
}

Is there any way to redo this to make the call to Round() more concise, preferably removing the need for a strongly typed PropertyManager? I think ultimately calling it as follows would be awesome, but I can't figure out the language constructs to use to get there:

Round(combatants, c => c.Attack, c => c.Defense, c => c.Health);

Yup, it is possible, but you'll have to delve into the dark world of Expression Trees.

This function creates getter and setter functions using the out parameters. It's ugly, and you usually don't want to use two out parameters, but hopefully it'll get you moving in the right direction.

public static bool CreateGetterSetter<T, V>(Expression<Func<T, V>> getterExpression, out Func<T, V> getter, out Action<T, V> setter)
{
    if (getterExpression == null)
        throw new ArgumentNullException("getterExpression");

    var memberExpression = getterExpression.Body as MemberExpression;
    if (memberExpression == null || memberExpression.NodeType != ExpressionType.MemberAccess || memberExpression.Member == null)
        throw new ArgumentException("The expression must get a member (property or field).", "getterExpression");

    // The expression passed in is the getter, so just compile it.
    getter = getterExpression.Compile();

    // The setter function takes two parameters as input.
    var paramT = Expression.Parameter(typeof(T));
    var paramV = Expression.Parameter(typeof(V));

    // Create the setter function
    if (memberExpression.Member.MemberType == MemberTypes.Field)
    {
        // The getter retrieves a field.
        var field = memberExpression.Member as FieldInfo;
        if (field == null)
            throw new ArgumentException("Could not get field info for member " + memberExpression.Member.ToString());

        // This expression represents the field on the parameter.
        var fieldExpr = Expression.Field(paramT, field);

        // This expression is what the field will be set to.
        Expression rightExpr;
        if (paramV.Type == field.FieldType)
            rightExpr = paramV;
        else
            rightExpr = Expression.Convert(paramV, field.FieldType);

        // This is the assignment expression (sets the field to the parameter value).
        var assign = Expression.Assign(fieldExpr, rightExpr);

        // Compile the expressions into a lambda.
        setter = Expression.Lambda<Action<T, V>>(assign, paramT, paramV).Compile();
    }
    else if (memberExpression.Member.MemberType == MemberTypes.Property)
    {
        // The getter retrieves a property.
        var property = memberExpression.Member as PropertyInfo;
        if (property == null)
            throw new ArgumentException("Could not get property info for member " + memberExpression.Member.ToString());

        // This expression represents what the property will be set to.
        Expression paramSet;
        if (paramV.Type == property.PropertyType)
            paramSet = paramV;
        else
            paramSet = Expression.Convert(paramV, property.PropertyType);

        // Handle read-only properties (have no setter).
        if (!property.CanWrite)
        {
            setter = (t, v) => Debug.Write("Cannot set read-only property: " + property.ToString());
            return false;
        }

        // This expression represents calling the property setter.
        var call = Expression.Call(paramT, property.SetMethod, paramSet);

        // Compile the expressions into a lambda.
        setter = Expression.Lambda<Action<T, V>>(call, paramT, paramV).Compile();
    }
    else
    {
        // The member was not a field or a property.
        setter = (t, v) => Debug.Write("Setter invoked for invalid expression");
        return false;
    }
    return true;
}

Usage:

public class A
{
    public string Property { get; set;}
}

// Define the variables to hold the out parameters.
Func<A, string> getter;
Action<A, string> setter;

// Call function to use expression trees to create the getter and setter.
CreateGetterSetter<A, string>(a => a.Property, out getter, out setter);

// Now you can use "getter" to get A.Property...
var inst = new A { Property = "value" };
var value = getter(inst); // value = "value"

// ... and "setter" to set A.Property...
setter(inst, "new"); // inst.Property = "new"
value = getter(inst); // value = "new";

And to implement Round this way...

public static void Round<T>(IEnumerable<T> collection, params Expression<Func<T, decimal>>[] getterExpressions)
{
    // Create all the lambdas up-front. This is expensive, so you should cache this somewhere rather than
    // re-compiling all the expressions every time.
    var gettersSetters = new Tuple<Func<T, decimal>, Action<T, decimal>>[getterExpressions.Length];
    for(int i=0;i<getterExpressions.Length;i++)
    {
        Func<T, decimal> getter;
        Action<T, decimal> setter;
        CreateGetterSetter<T, decimal>(getterExpressions[i], out getter, out setter);
        gettersSetters[i] = Tuple.Create(getter, setter);
    }

    foreach (var item in collection)
    {
        foreach (var tuple in gettersSetters)
        {
            var getter = tuple.Item1;
            var setter = tuple.Item2;

            var value = getter(item);
            value = Math.Round(value, 1);
            setter(item, value);
        }
    }
}

Hope that helps!


Python Decorator - inspecting function argument values Some of my functions use a "fail_silently" flag.  It is used in the following way:

def test(a, b, c=1, fail_silently = False)
    try:
        return int(a) + c
    except:
       if fail_silently:
           return None
       raise

Therefore, if there is an error, we catch it and fail gracefully. The key here is that it can be toggled dynamically by whoever is calling it.

I am using this for a many different functions and class methods and thought to make it a decorator.

There are a few problems:

I want to be able name the flag "raise_exception" or "fail_silently" or "whatever"...
The flag may or may not have a default value
The flag may or may not be passed in (usually not)
It needs to work with class methods too

Thus my decorator needs to look for (assuming the flag is called "fail_silently")
the flag in the following locations in this order

**kwargs (the passed in function arguments), simple dictionary get on flag
*args - get the positional argument of the flag, then scan for index in args (which might not be there)
Get the default value of the flag from the function

The problem is the code is now getting really messy and has many points of failure.

def try_except_response(parameter = 'fail_silently'):
    def real_decorator(func):
        @functools.wraps(func)
        def wrapper(*args, **kwargs):
            try:
                return func(*args, **kwargs) # the function itself,
            except: # if it raises an Exception!
                # check to see if the Flag is in passed in kwargs!
                if kwargs.get(parameter)== True:
                    return None
                elif kwargs.get(parameter) == False:
                    raise
                else:
                    # Flag is not in kwargs, check to see if it is in args
                    function_args, vargs, kewords, defaults = inspect.getargspec(func) # get the index of argument of interest
                    try:
                        argument_index = function_args.index(parameter)  # get the index of argument of interest
                    except ValueError:
                        raise ValueError('The inputted decorator for "fail_silently" was not found!  The behavior will not work.')

                    if len(args) > argument_index: # check to see if it is inputted

                        keyword = args[argument_index] # get the value!!
                        if keyword == True:
                            return None
                        elif kwargs == False:
                            raise
                        else:
                            raise ValueError('The "fail_silently" flag did not return a value that we understand.  Must be True or False')

                    else:
                        # not in args either! let's go get the default value.
                        # TODO MORE STUFF!!!
                        raise
                    raise
                raise
        return wrapper

What is the best way this can be cleaned up?  Or alternatives?  I am thinking about implementing a version of locals() that will create a dictionary of all parameters passed into the function and then check from there...

There are simplifying assumptions we can make, but that would require the person writing the function to know something ('flag is always last argument, or default value is always False...'), but I am hoping to get the decorator to be as transparent as possible so it just "works" and lets the function be a function (or class method)

I'm not a fan of this design. @TheBlackCat is right. Don't do this.

If you are to do this, rethink your design. A more sensible one might look like

import functools
from contextlib import suppress

def try_except_response(func):
    @functools.wraps(func)
    def wrapper(*args, fail_silently, **kwargs):
        if fail_silently:
            with suppress(Exception):
                return func(*args, **kwargs)
        else:
            return func(*args, **kwargs)

    return wrapper

The key principle here is KISS.

Note that I'm suppressing Exception, not BaseException, since catching SystemExit and KeyboardInterrupt is almost always a bad idea.

If you want to make fail_silently a default argument, just use another wrapper:

def default_fail_silently(default):
    def decorator(func):
        @functools.wraps(func)
        def wrapper(*args, fail_silently=default, **kwargs):
            return func(*args, fail_silently=fail_silently, **kwargs)
        return wrapper
    return decorator

Then you can do

@default_fail_silently(False)
@try_except_response
def test(a, b, c=1)
    return int(a) + c

This does mean there's more boilerplate, since it exists for each piece of functionality you add, but the logic itself is an order of magnitude simpler.


Algorithm optimisation to get first parent with specific kind of class I am writing an extension for UIViewController to search all the parents and return a specific parent which is a kind of specific class. here is my try :

extension UIViewController {

    var rootViewController : RootViewController? {
        var parentController: UIViewController? = parent
        while let currentParent = parentController {
            if (currentParent is RootViewController) {
                return (currentParent as? RootViewController)!
            }
            else if let parentOfParent = currentParent.parent, parentOfParent != currentParent {
                parentController = parentOfParent
            }
            else {
                parentController = nil
            }
        }
        return nil
    }

}

Is there a better approach to solve the issue, maybe a recursive way to handle that. And if it's better in term of performance.

I wouldn't care much about performance here, since a view controller
hierarchy does not have hundreds of elements. But some things can
be simplified. First,

if (currentParent is RootViewController) {
    return (currentParent as? RootViewController)!
}

can be simplified to

if let rootVC = currentParent as? RootViewController {
    return rootVC
}

which also has the advantage that no forced unwrapping operator is
needed. Even if we know that it cannot fail here (because the type has
been checked in the if-statement) it is better to write the code
in a way that it becomes obvious that it cannot crash.

Next, the loop itself can be simplified:

var rootViewController: RootViewController? {
    var currentViewController = self
    while case let parentController? = currentViewController.parent,
        parentController != currentViewController {
        if let rootVC = parentController as? RootViewController {
            return rootVC
        }
        currentViewController = parentController
    }
    return nil
}

Here the "optional pattern"

    case let parentController? = currentViewController.parent

is used to check that there is a parent, and the constraint

    parentController != currentViewController

ensures that the parent is different from the current view controller.

But I don't think that a view controller can be its own parent.
If you don't have a special reason to consider this case, the code
can further be simplified using optional binding:

var rootViewController: RootViewController? {
    var currentViewController = self
    while let parentController = currentViewController.parent {
        if let rootVC = parentController as? RootViewController {
            return rootVC
        }
        currentViewController = parentController
    }
    return nil
}

Another suggestion is to make the method generic, so that it can
be used to find a parent view controller of any given class:

func parent<T : UIViewController>(ofType: T.Type) -> T? {
    var currentViewController = self
    while let parentController = currentViewController.parent {
        if let parent = parentController as? T {
            return parent
        }
        currentViewController = parentController
    }
    return nil
}

Finally: You can replace the loop by recursion, using the
flatMap method of Optional and the nil-coalescing operator ??:

func parent<T : UIViewController>(ofClass: T.Type) -> T? {
    return parent.flatMap { ($0 as? T) ?? $0.parent(ofClass: T.self) }
}

Here parent.flatMap { ... } returns nil if parent == nil,
this terminates the recursion eventually. Otherwise $0 is the
(unwrapped) parent. If the conditional cast $0 as? T succeeds
then this will be the return value. Otherwise the method is called
recursively on the parent view controller.

I doubt that this will be faster, and it is more difficult to read
and to debug. So it's up to you which approach you prefer.


node.js library for extracting words from a text I'm looking for feedback on my library for extracting words from a text: https://npmjs.org/package/uwords

The extracted word is defined as sequence of Unicode characters from Lu, Ll, Lt, Lm, Lo groups. So the code of the main part is (https://github.com/AlexAtNet/uwords/blob/master/index.js#L9):

module.exports = function (text) {
    var words, word, index, limit, code;

    words = [ ];
    word = null;

    for (index = 0, limit = text.length; index < limit; index += 1) {
        code = text.charCodeAt(index);
        if (-1 === _.indexOf(letters, code, true)) {
            if (null !== word) {
                words.push(word.join(''));
                word = null;
            }
        } else {
            if (null === word) {
                word = [ ];
            }
            word.push(String.fromCharCode(code));
        }
    }

    if (null !== word) {
        words.push(word.join(''));
    }

    return words;
};

and the array letters was created as follows (https://github.com/AlexAtNet/uwords/blob/master/gruntfile.js#L59):

    grunt.registerTask('create-letters-json', 'letters.json', function () {
        var letters, compacted;

        letters = [
            require('unicode/category/Lu'),
            require('unicode/category/Ll'),
            require('unicode/category/Lt'),
            require('unicode/category/Lm'),
            require('unicode/category/Lo')
        ].reduce(function (list, item) {
            list.push.apply(list, Object.keys(item).map(function (value) {
                return parseInt(value, 10);
            }));
            return list;
        }, [ ]).sort(function (a, b) { return a - b; });

        compacted = (function (list) {
            var result, item, idx, value;

            result = [ ];

            item = { begin : list[0], end : list[0] };
            result.push(item);

            for (idx = 1; idx < list.length; idx += 1) {
                value = list[idx];
                if (item.end + 1 === value) {
                    item.end = value;
                } else {
                    item = { begin : list[idx], end : list[idx] };
                    result.push(item);
                }
            }

            for (idx = 0; idx < result.length; idx += 1) {
                item = result[idx];
                if (item.begin === item.end) {
                    result[idx] = item.begin;
                } else {
                    result[idx] = [ item.begin, item.end ];
                }
            }

            return result;
        }(letters));

        require('fs').writeFileSync(__dirname + '/letters.json',
            JSON.stringify(compacted, null, 2));
    });

It is quite naive approach but I think that it will work in most of the cases. What do you think?

The top part looks clean, personally I would

Not compare with null all the time, just check word.length and have word be an array at all times.
Not initialize word and words separately
Not use String.fromCharCode(code), I would use text[index] instead
I would use the ~ operator instead of comparing to -1
I would first deal with finding a match , and then with not finding a match ( switch the if blocks in other words ), my mind had to do a double take when I was reading your code
if would add a space to the end of text, so that I would not need the last if statement

All that would give me something like:

module.exports = function (text){
 text += " ";

 var words = [],
     word  = [],
     limit = text.length, code, index;

    for (index = 0; index < limit; index += 1) {
        code = text.charCodeAt(index);
        if (~_.indexOf(letters, code, true)) {
          word.push( text[index] )
        } else {
          if (word.length) {
            words.push(word.join(''));
            word = [];
          }
        }
    }
    return words;
};

Finally, I think collecting char-codes in the 2nd script, then taking chars, converting those to char-codes and then use the chars again might not be the best approach.

Personally, I would letters be an object where each letter ( not the char-code ) would be a property of the object set to true. No more char-code conversions, and most likely it would beat the sorted lookup table. ( To be tested.. )


Putting a border around the selected list item I have an HTML list. The goal is that:

When an li is clicked, the .my-list--selected class will be applied. And, since the inner div also has a border-bottom, the .border-bottom class must be removed (or else it appears as though there is a 2px wide border along the bottom if both classes are applied).
When another li is clicked, the previously selected li's .my-list--selected is removed and the .border-bottom class is reinstated (there can only be 1 '.my-list--selected' at a time).

I've written the below code and it works, but it seems really messy and overly complicated to me.

Is there a better way to go about achieving this goal?

$("li").click(function() {
  //Remove the my-list--selected class from any elements that already have it
  $('.my-list--selected').removeClass('my-list--selected');
  //Add the .border-bottom class back to any element that is missing it
  $('.my-list--selected').addClass('border-bottom');
  //Add the my-list--selected class to the clicked element
  $(this).addClass('my-list--selected');
  //Remove the border-bottom class from the clicked element
  $(this).find('.border-bottom').removeClass('border-bottom');
});
li div {
  padding: 0 10px;
}

.border-bottom {
  border-bottom: 1px solid #ccc;
}

.my-list--selected {
  border: 1px solid #ccc;
}

Content

Content

Content

Are you required to have the .border-bottom class on the div elements?  I'd move those up to the li elements so you don't have to bother with the add/remove .border-bottom and you could use first-child to cleanly add a top border for the selected element if it's first (and avoid a double border for the other items).  See below snippet with the changes.

$("li").click(function() {
  //Remove the my-list--selected class from any elements that already have it
  $('.my-list--selected').removeClass('my-list--selected');
  //Add the my-list--selected class to the clicked element
  $(this).addClass('my-list--selected');
});
.border-bottom {
  border-bottom: 1px solid #ccc;
}

.my-list--selected {
  border-left: 1px solid #ccc;
  border-right: 1px solid #ccc;
}

.my-list--selected:first-child {
  border-top: 1px solid #ccc;
}

Content

Content

Content


Store Float16 variables I'm currently rolling a little Float16 implementation. It's minimalistic and can't handle infinity, NaN and will neither work properly with overflows nor with underflows. (That's not the goal btw.)

Is there something particular wrong with the following implementation which could lead to major issues?

class Float16
{
public:
    Float16(float other)
    {
        IEEESingle f32;
        f32.Float = other;

        if (other == 0.0f)
        {
            bits.IEEE.Exp = 0;
            bits.IEEE.Man = 0;

            return;
        }

        bits.IEEE.Sign = f32.IEEE.Sign;
        bits.IEEE.Exp  = f32.IEEE.Exp - 112;
        bits.IEEE.Man  = f32.IEEE.Man >> 13;
    }

    operator float() const
    {
        if (bits.IEEE.Exp == 0 && bits.IEEE.Man == 0)
        {
            return 0.0f;
        }

        IEEESingle f32;

        //112-->F32 Exponent = N + 127, F16 Exponent = N + 15;

        f32.IEEE.Sign = bits.IEEE.Sign;
        f32.IEEE.Exp  = bits.IEEE.Exp + 112;
        f32.IEEE.Man  = bits.IEEE.Man << 13;

        //<< 13 since Mantisse f32 = 23 and Mantisse F16 = 10

        return f32.Float;
    }

private:
    union IEEEHalf
    {
        uint16_t bits;
        struct
        {
            uint16_t Man  : 10;
            uint16_t Exp  : 5;
            uint16_t Sign : 1;
        } IEEE;
    } bits;

    union IEEESingle
    {
        float Float;
        struct
        {
            uint32_t Man  : 23;
            uint32_t Exp  : 8;
            uint32_t Sign : 1;
        } IEEE;
    };
};

int main()
{
    Float16 f16 = 1.5f;
    float   f32 = (float)f16;

    return 0;
}

I am aware that my calculations will happen at F32 level and I don't intend to implement direct F16 calculations. I am planning to add some operators to the class, but the question focuses mostly on the conversion between f16 and f32. I am aware of the precision and rounding issues of Float16 values.

Is there something particular wrong with the following implementation which could lead to major issues?

Yes; the union seems to be used in a way that is actually Undefined Behavior.  Let’s see…

    f32.IEEE.Sign = bits.IEEE.Sign;
    f32.IEEE.Exp  = bits.IEEE.Exp + 112;
    f32.IEEE.Man  = bits.IEEE.Man << 13;

    return f32.Float;

Yes, you are reading from the Float member that is never written to, the optimizer can know that it is not the active member, and act as if the program never reaches this point.  Anything is possible, including crashing.

We have seen the optimizers get more and more aggressive, taking advantage of U.B., breaking “bad” code that worked historically.  So, it is a real concern.  In any case, the layout of the bit fields is up to the implementation; that is only an issue if you plan on saving these 16-bit values to files.

See Scott Schurr’s presentation on this.  Your specific case is handled toward the end, and he shows that of all the work-arounds, using intrinsic memcpy with the items known to be properly aligned will be completely optimized out by the compiler, generating the code you really intended with the union!

See also ⧺ES.9.

float   f32 = (float)f16;

Don’t use C-style casts.  Any appearance of such a cast is a flag for code review; casts that are actually needed should be function style, specialized, or use clearly named helper functions.

You supplied an implicit conversion, so you don’t need this cast at all.


On right click draw triangle to the canvas This is a simple program that using easelJS adds functionality to the right click, when the right mouse button is clicked a triangle is drawn to the screen. I am just wanting to get the opinions of others on my coding style and how it could be improved.

body{
    margin: 0;
    background-color: #d3d3d3;
    font-family: Sans-Serif;
}

h1{
    font-weight: 100;
    text-align: center;
}

canvas{
    display: block;
    margin: auto;
    background-color: #ffffff;
    border: solid 1px #000000;
}
<!DOCTYPE html>

        <title>Draw Triangle</title>
        <link rel="stylesheet" href="styles.css"/>

Right click to draw a Triangle to the canvas

        <canvas id="myCanvas" width="900" height="700"></canvas>

General problems

It is better to add events via addEventListener rather than in the HTML, this keeps the source code together making it easier to maintain. It also protects your code from 3rd party code overwriting the event handler. If some code later sets window.onload = theirFunction; it overwrites your event that will not get called.
Keep your variables out of the global scope if possible. stage and triangle do not need to be in global scope. triangle is only used in the one function so define it there.
Use const rather than var if you do not intend to modify a variable.
Don't let lines get too long. 80 character is the usual limit. If a line gets longer than that break it into multiple lines.
Right click will bring up the context menu. You can prevent this by listening to the contextmenu event and calling preventDefault on the event object.
Why the defaulting to keyCode? is that even defined for the easelJS event ?
Don't add needless code. You define a variable for evt.nativeEvent, and e.which. These are not really needed.
Use correct names for variables. You use key for which button is pressed, this can be confusing when code gets longer.
You say you want to add a triangle on the right click but the code you had will add a triangle for any button except the left button.
Make your code easier to read by using spaces after commas, between operators, after statements and befor { eg if(bar===poo){foo(a,b+2,c);} is better if (bar === poo) { foo(a, b + 2, c); }

The rewrite

Rewrite of your code using the above.

body{
    margin: 0;
    background-color: #d3d3d3;
    font-family: Sans-Serif;
}

h1{
    font-weight: 100;
    text-align: center;
}

canvas{
    display: block;
    margin: auto;
    background-color: #ffffff;
    border: solid 1px #000000;
}
<!DOCTYPE html>

        <title>Draw Triangle</title>
        <link rel="stylesheet" href="styles.css"/>

Right click to draw a Triangle to the canvas

        <canvas id = "myCanvas" width = "900" height = "700"></canvas>

Additional notes.

I don't know what your intention is but the triangle seems a little odd not being centered on the mouse click. Also I would define the triangle as a abstract path rather than hard coded to the function.

Is there a reason you put the code in the head of the document. You could have placed it at the bottom of the page and avoided having to wait for the page load event.

Also, maybe you are wanting to learn easelJS, but the load time of easelJS is substantial and what you do can just as easily be done using the native 2D API.

I strongly believe that you should have a very good reason to use a 3rd party library, especially when you are learning. You are best spending your time learning the native (and very good and performant) APIs. You may not always be able to use the library, but you will always have access to the native API

Rewrite without easelJS

To match your code I have implemented a very simple stage. Though what you do does not require a stage object and could be a lot simplier.

// Please note this code uses a compact style for single line statements
// There is no ; before the closing } to comply with JSLint rule
// "Semicolan must be followed by newline."

;(() => { // wrapping your code inside a function alows you to keep the global scope clean
          // the ; befor the ( is to prevent code above it without a ; from
          // mistaking the ( as a function call

  const canvas = document.getElementById("myCanvas");
  const ctx = canvas.getContext("2d");

  const shapes = { // you could add more shapes here
    triangle: [ [0, -20], [20, 20], [-20, 20] ], // coordinates of triangle
  };

  addEventListener("load", init); // add the load event listener

  // implement a stage object to hold shapes
  const stage = {
    shapes: [],
    addShape(x, y, color, path) { this.shapes.push({x,  y, color, path }) },
    update() {  // function to redraw all shapes on the stage
      ctx.clearRect(0, 0, canvas.width, canvas.height);
      this.shapes.forEach((shape) => {
        var i;
        ctx.fillStyle = shape.color;
        ctx.beginPath();
        i = 0;
        while (i < shape.path.length) {
          ctx.lineTo(  // sometimes it is best to break up complex lines
              shape.path[i  ][0] + shape.x,
              shape.path[i++][1] + shape.y
          )
        }
        ctx.closePath();  // this is really only needed if you are using stroke
        ctx.fill();
      });
    }
  };

  function init() {
    canvas.addEventListener("mousedown", mouseDown);
    canvas.addEventListener("contextmenu", event => event.preventDefault() );
    function mouseDown(event) {
      const bounds = event.target.getBoundingClientRect();
   const x = event.pageX - bounds.left - scrollX;
   const y = event.pageY - bounds.top  - scrollY;
      if (event.which === 3) {
        stage.addShape(x, y, "black", shapes.triangle);
        stage.update();
      }
    }
  }
})();
body {
  margin: 0;
  background-color: #d3d3d3;
  font-family: Sans-Serif;
}

h1 {
  font-weight: 100;
  text-align: center;
}

canvas {
  display: block;
  margin: auto;
  background-color: #ffffff;
  border: solid 1px #000000;
}
<!DOCTYPE html>

  <title>Draw Triangle</title>

Right click to draw a Triangle to the canvas

  <canvas id="myCanvas" width="900" height="700"></canvas>

The simplest way to achieve the same functionality.

const canvas = document.getElementById("myCanvas");
const ctx = canvas.getContext("2d");
const triangle = [ [0, -20], [20, 20], [-20, 20] ];
canvas.addEventListener("mousedown", mouseDown);
canvas.addEventListener("contextmenu", e => e.preventDefault() );
function mouseDown(event) {
  const bounds = event.target.getBoundingClientRect();
  const x = event.pageX - bounds.left - scrollX;
  const y = event.pageY - bounds.top  - scrollY;
  if (event.which === 3) { drawShape(x, y, triangle) }
}
function drawShape(x, y, shape) {
  var i = 0;
  ctx.fillStyle = "black";
  ctx.beginPath();
  while (i < shape.length) { ctx.lineTo( shape[i][0] + x, shape[i++][1] + y ) }
  ctx.fill();
}
body {
  margin: 0;
  background-color: #d3d3d3;
  font-family: Sans-Serif;
}

h1 {
  font-weight: 100;
  text-align: center;
}

canvas {
  display: block;
  margin: auto;
  background-color: #fff;
  border: solid 1px #000;
}

Right click to draw a Triangle to the canvas

  <canvas id="myCanvas" width="900" height="700"></canvas>


Converting a query result into an key=>value pairs for a lookup table My app has a number of lookup tables stored in a MySQL database that are used for various purposes such as a select element.

Unfortunately, the query results aren't in an easily-digestible format and need some massaging to be more useful to me.

For example, when you query one of the lookup tables for all values, you get an array like this:

 0 => ['level_id' => 1, 'level' => 'Trivial'],
 1 => ['level_id' => 2, 'level' => 'Moderate']
 2 => ['level_id' => 3, 'level' => 'Challenging'],
 3 => ['level_id' => 4, 'level' => 'Formidable']

But for my purposes, this is what I want:

 1 => 'Trivial',
 2 => 'Moderate',
 3 => 'Challenging',
 4 => 'Formidable'

(Note that the array keys are the same as the record id).

If the array has more than one additional column, the result will be contained in sub-arrays:

 3 => ['level' => 'Challenging', 'description' => 'Foobar'],
 4 => ['level' => 'Formidable', 'description' => 'Bazbat']

This is all very trivial, but I wrote this function that I'd like you to review to know if there is a better way to do this in PHP.

The function needs to be generic so it can use any lookup array without having to know the column names.

<?php
// Turn an array into a key=>value pair. Assumes the key is the first item in the sub-array.
public function column_into_keys(array $array): array {

    // get the name of the column that contains the record id
    $key = key($array[0]);

    foreach($array as $row) {

        // pop the new key off the top of the array
        $id = array_shift($row);

        // is there only one item left in the array?
        if (count($row) == 1)
            // get the first value
            $result[$id] = current($row);
        else
            // get all of the values
            $result[$id] = $row;
    }

    return $result;
}

Given you are working with PHP, there is no reason to neglect such a feature that already exists in the language.

For example when you query one of the lookup tables for all values,

$data = $pdo->query("SELECT * FROM lookup")->fetchAll(PDO::FETCH_KEY_PAIR);

you get an array like this:

 1 => 'Trivial',
 2 => 'Moderate',
 3 => 'Challenging',
 4 => 'Formidable'

all thanks to PDO::FETCH_KEY_PAIR fetch mode.

Of course it works only with PDO, but you are supposed to use this driver anyway.


Robust, know-entropy password generator My use case is  I want a password generator that generates memorable and robust passwords for users, to give me a known minimum entropy.

For this - I'm approximating set of Consonant-Vowel-Consonant words, because then they are both memorable (ish) and have a known entropy level. The reason for this is because I'm not confident that when asked to pick a password, rules based 'systems' lead to at least some of the passwords being horribly weak.

So my core concern is whether the below has any security flaws - I believe that mapping /dev/urandom onto a flat character space should generally accomplish this, and the CVC groupings give me a known minimum entropy (44.9 bits in this case) over user-set passwords that I believe are often quite a lot worse across a whole set of user accounts.

cvc_gen.pl:

#!/usr/bin/env perl

use strict;
use warnings;

#uses /dev/urandom to fetch bytes.
#generates consonant-vowel-consonant groupings.
#each are 11.22 bits of entropy, meaning a 4-group is 45 bits.
#( 20 * 6 * 20 = 2400, which is 11.22 bits of entropy log2 2400
#log2(2400 ^ 4) = 44.91
#but because it's generated 'true random' it's a know entropy string.

my $num    = 4;
my $format = "CVC";

my %letters = (
    V => [qw ( a e i o u y )],
    C => [ grep { not /[aeiouy]/ } "a" .. "z" ],
);

my %bitmask_for;
foreach my $type ( keys %letters ) {
   #find the next power of 2 for the number of 'letters' in the set.
   #So - for the '20' letter group, that's 31. (0x1F)
   #And for the 6 letter group that's 7.  (0x07)
   $bitmask_for{$type} =  ( 2 << log ( @{$letters{$type}} ) / log 2 ) - 1 ;
}

open( my $urandom, '<:raw', '/dev/urandom' ) or die $!;

for ( 1 .. $num ) {
    for my $type ( split //, $format ) {
        my $value;
        while ( not defined $value or $value >= @{ $letters{$type} } ) {
            my $byte;
            read( $urandom, $byte, 1 );
            #byte is 0-255. Our key space is 20 or 6.
            #So rather than modulo, which would lead to an uneven distribution,
            #we just bitmask at '1F' giving 0-31, and discard and 'too high'.
            $value = (unpack "C", $byte ) & $bitmask_for{$type};
        }
        print $letters{$type}[$value];
    }
    print " ";
}
print "\n";
close($urandom);

You don't need the bitmasks here, you can do with just basic arithmetic.
You're right that a naive modulo would generate biased letter combinations. Therefore the typical pattern is:

$letters = "aeiouy";
$len = length($letters);
$limit = 255 / $len * $len;   # this must be integer arithmetic
$limit = 255 - 255 % $len;    # alternative spelling for the above line

while (($byte = next_byte()) >= $limit) {
    next;
}
return $letters[$byte % $len];

This pattern consumes less random numbers than yours and still generates a fair distribution.

Your code should not output a space at the end of the line.

To make your code testable, you should write a sub that generates a password from a given stream of bytes. Since this is the completely deterministic part, it's easy to test.

Make sure that the call to read succeeds, since otherwise your randomness gets replaced with undefinedness.

You should use the same code pattern to generate the consonants and vowels, so that the code looks more symmetric. But that's a minor point.


Most basic fantasy combat simulator I was wondering if anyone had anything to recommend for how to clean up the code I made below. Essentially I am asking if my code is messy or not and if there are any recommendations for my improvement.

# Most basic fantasy combat simulator
import random

p_health = 5
g_health = 5
goblin_alive = True

def status():
    if g_health == 5:
        return "\nA menacing goblin stands before you..."
    elif g_health >= 3:
        return "\nThe goblin is looking a little tired, and is bleeding..."
    elif g_health >= 1:
        return "\nThe goblin is bleeding horribly and looks enraged..."

while goblin_alive:
    if g_health <= 0:
        goblin_alive = False
        print("\nCongrats you slayed the goblin!")
        again = input("Play again? Y/N: ")

        if again == 'y' or again == 'Y':
            goblin_alive = True
        elif again == 'N' or again == 'n':
            print("\nGoodbye")
            exit()
    if p_health <= 0:
        print("Oh dear you have died horribly and the goblin cuts your head off for a trophy...")
        again = input("Play again? Y/N: ")

        if again == 'y' or again == 'Y':
            p_health = 5
            g_heath = 5
            goblin_alive = True
        elif again == 'N' or again == 'n':
            print("\nGoodbye")
            exit()

    desc = status()
    print(desc)
    print("You have " + str(p_health) + ' hit points.')

    attack = input("Press enter to attack: ")
    if attack == '':
        print("\nYou swing your sword fiercely at the goblin!")
        hit_type = random.randint(1, 2)
        if hit_type == 1:
            damage = random.randint(1, 3)
            print("You deal a fierce blow for " + str(damage) + " damage to the goblin.")
            g_health = g_health - damage
        elif hit_type == 2:
            damage = random.randint(1, 3)
            print("The goblin slashes you for " + str(damage) + " damage, uh oh...")
            p_health = p_health - damage
    else:
        print("\nYou better do something...")

Your code looks good and was easy to understand, at least for me :)

A few things you can do to improve it:

Python String Format and Upper

Consider using python format() method, instead of:

print("You have " + str(p_health) + ' hit points.')

try:

print('You have {} hit points.'.format(p_health))

And when checking if a char equals to a given char, you don't have to check both cases you can just use Python upper/lower function so instead of:

if again == 'y' or again == 'Y':

try:

if again.lower() == 'y':
    do stuff

Better Practice

The status() function could be improved by assigning a value to result and returning it, this is just better practice:

def status():
    res = ''
    if g_health == 5:
        res = "\nA menacing goblin stands before you..."
    elif g_health >= 3:
        res = "\nThe goblin is looking a little tired, and is bleeding..."
    elif g_health >= 1:
        res = "\nThe goblin is bleeding horribly and looks enraged..."
    return res

Python Class

Perhaps this code will be better in a GoblinGame class, this class will have properties such as goblin_health, player_health and goblin_alive that can be assigned when initializing the class instance, and a run() method to play a round of the game, I've wrote a quick sample class you can follow along from there:

class GoblinGame:
    def __init__(self):
        self.player_health = 5
        self.goblin_health = 5

    def goblin_alive(self):
        return self.goblin_health > 0

    @property
    def status(self):
        res = ''
        if self.goblin_health == 5:
            res = "\nA menacing goblin stands before you..."
        elif self.goblin_health >= 3:
            res = "\nThe goblin is looking a little tired, and is bleeding..."
        elif self.goblin_health >= 1:
            res = "\nThe goblin is bleeding horribly and looks enraged..."
        return res

    def run(self):
        while self.goblin_alive:
         # write the game logic...

Good Luck!


Grading students: proper encapsulation I am discussing with one of my peers about proper encapsulation. We had to write this code. One of the specifications is: "The Student object has to have a Grade object called finalGrade. Code hint: Grade finalGrade;"

This is my code

public class Student
{
  private String firstName;
  private String lastName;
  private String id;

  public Grade finalGrade = new Grade(); // THIS IS THE QUESTIONABLE CODE
...
...
}

public class TestStudent
{

  public static void main(String[] args)
  {
   ...
   ...
   ...
    student.finalGrade.setGradeValue(85);
    student.finalGrade.setLetterGrade('B');
    student.setFinalGrade('B');

    System.out.println(student.finalGrade.getGradeValue());
    System.out.println(student.finalGrade.getLetterGrade());
  }
}

This is their code

public class Student {
...
Grade finalGrade;  // THIS IS THE QUESTIONABLE CODE
public Grade getFinalGrade() {
return finalGrade;
}

public void setFinalGrade(Grade finalGrade) {
this.finalGrade = finalGrade;
}

....
public String toString() {
String display = String.format("Student: id= %d name= %s letterGrade = %s", ID, getName(), finalGrade );
return display;
}
}

public class TestStudent {
public static void main(String[] argv) {
...
...
Grade g = new Grade();
g.setGradeValue(85);
g.setLetterGrade('B');
s.setFinalGrade(g);
System.out.println(s.toString());
}
}

Ultimately, which code is better given the specification?  My argument is that finalGrade is a class and should be accessed with the dot operator. My peer states that it is an attribute and should be private. I see it as a class accessing a class such as SuperClass.class.method();

One of the most important concept of Object Oriented Programming (and programming in general) is information hiding and encapsulation. A class should never expose its attributes unless there are some very valid reasons to do so (for example to share a constant). The fields of a class should be made available providing getters and setters. The reasons of that are many. For example if you have that field finalGrade public and a day you will want to change some details of that field, e.g. use a subclass of Grade, you will cause a long cascade of errors wherever you made a reference to that object without a getter method. We could spend a lot of time talking about how information hiding is important in OOP, but in short...your peer is right!


Solve Project Euler #4 in PHP A palindromic number reads the same both ways.

The largest palindrome made from the product of two 2-digit numbers is 9009 = 91 × 99.

Find the largest palindrome made from the product of two 3-digit numbers.

<?php

function isPalindrome($input) {
    return (string) $input === strrev($input);
}

$max = 999;
$min = 100;
$found = 0;

for ($i = $max; $i >= $min; $i--) {
    for ($j = $max; $j >= $min; $j--) {
        $result = $i * $j;
        if (isPalindrome($result)) {
            if ($result > $found) {
                $found = $result;
                $a = $i;
                $b = $j;
            }
        }
    }
}

print sprintf("Found %d times %d = %d", $a, $b, $found);

Your outer loop starts at 999 and the inner loop multiplies that by all the numbers from 999 down to 100.

Then your outer loop decrements $i to 998, and the inner loop multiplies that by all the numbers from 999 down to 100.  The first test 998*999 has already been considered, since multiplication is commutative.  Not a huge inefficiency at this first step, but it gets worse.

When the outer loop gets down to 100, the inner loop again multiplies that by all the number from 999 down to 100 ... only 1 combination which is new.

You should start your inner loop at $i, instead of at $max.

Early termination.  After finding a palindrome from the product of (say) 901 and 856, you don’t have to test whether the product is a palindrome (expensive operation) unless $result > $found is true (cheap test).  And if $result is less than $found, you can break out of your inner loop, since any successive product will be smaller.   Finally, if $i * $i < $result (combined with the first optimization), you can also exit your outer loop.


Collision Detection in a 2D shooter game I have the following code that works just fine for collision detection. The only problem is its size and its performance. I'm trying to do it and many other things 60 times per second, which doesn't work out very well. Is there any way to condense it as it's much the same stuff over and over again?

for(int x=0; x<40; x++) {
     for(int y=0; y<28; y++) {
        if(tiles[x][y]==0) {
            if(keyManager.up) {
                Rectangle player = new Rectangle(you.getX(), you.getY()-1, 10, 10), tile = new Rectangle(x*25, y*25, 25, 25);
                if(player.intersects(tile)) {
                    return;
                }
                if(x==39 && y==27) {
                    you.setY(you.getY()-1);
                }
            } if(keyManager.down) {
                Rectangle player = new Rectangle(you.getX(), you.getY()+1, 10, 10), tile = new Rectangle(x*25, y*25, 25, 25);
                if(player.intersects(tile)) {
                    return;
                }
                if(x==39 && y==27) {
                    you.setY(you.getY()+1);
                }
            } if(keyManager.left) {
                Rectangle player = new Rectangle(you.getX()-1, you.getY(), 10, 10), tile = new Rectangle(x*25, y*25, 25, 25);
                if(player.intersects(tile)) {
                    return;
                }
                if(x==39 && y==27) {
                    you.setX(you.getX()-1);
                }
            } if(keyManager.right) {
                Rectangle player = new Rectangle(you.getX()+1, you.getY(), 10, 10), tile = new Rectangle(x*25, y*25, 25, 25);
                if(player.intersects(tile)) {
                    return;
                }
                if(x==39 && y==27) {
                    you.setX(you.getX()+1);
                }
            }
        }
    }
}

You are creating a new Player and Tile every time this code executes. That is likely your performance hit. Try moving the existing player rather than creating a new one in a new location.

Does performance slow down further the longer the game runs? This is because your objects are piling up.

For further reading I recommend looking at AABB Collision Detection and Quad Trees.


If statement with 2 conditions On my webform, there are two kinds of address-fields a user can give. But since it's a complex webform (Drupal) it's kind of hard to achieve. Every time a user adds an extra address, the form is regenerated and there will be a new item added to the $address_afl and $address_bev-array. Is it possible to compress this code so the foreach-loop starts looping with the highest value (either $address_afl or$address_bev )

$address_afl = $form['field_afl_dienst_adressen']['und'];
$i = 0;
foreach($address_afl as $value) {
    if (is_array($value)) {
        $test = array_keys($value);
    if(is_array($test)) {
        foreach ($test as $z) {
            if (strpos($z, '#') === 0) continue;
            if (isset($form['field_afl_dienst_adressen']['und'][$i][$z])) {
                $form['field_afl_dienst_adressen']['und'][$i][$z]['#prefix'] = "
";
                $form['field_afl_dienst_adressen']['und'][$i][$z]['#suffix'] = "
";
            }
        }
    }
    }
    $i++;
}

$address_bev = $form['field_bevoegde_adressen']['und'];
$i = 0;
foreach($address_bev as $value) {
    if (is_array($value)) {
        $test = array_keys($value);
    if(is_array($test)) {
        foreach ($test as $z) {
            if (strpos($z, '#') === 0) continue;
            if (isset($form['field_afl_dienst_adressen']['und'][$i][$z])) {
                $form['field_bevoegde_adressen']['und'][$i][$z]['#prefix'] = "
";
                $form['field_bevoegde_adressen']['und'][$i][$z]['#suffix'] = "
";
            }
        }
    }
    }
    $i++;
}

I don't quite exactly understand the logic, but you could extract out at least one method which removes lots of code duplication. I've inverted some conditions (which improves code readability) and placed some comments into the code too.

function processValue(&$form, $value, $i, &$form_sub) {
    if (!is_array($value)) {
        return;
    }

    // TODO: rename test to $keys or something more meaningful
    $test = array_keys($value);
    if (!is_array($test)) {
        // TODO: is it possible? according to the documentation
        // array_keys() always returns array
        return;
    }
    foreach ($test as $z) {
        if (strpos($z, '#') === 0) {
            continue;
        }
        if (!isset($form['field_afl_dienst_adressen']['und'][$i][$z])) {
            continue;
        }
        $form_sub[$i][$z]['#prefix'] = "
";
        $form_sub[$i][$z]['#suffix'] = "
";
    }
}

$address_afl = $form['field_afl_dienst_adressen']['und'];
$i = 0;
foreach($address_afl as $value) {
    processValue($form, $value, $i, $form['field_afl_dienst_adressen']['und']);
    $i++;
}

$address_bev = $form['field_bevoegde_adressen']['und'];
$i = 0;
foreach($address_bev as $value) {
    processValue($form, $value, $i, $form['field_bevoegde_adressen']);
    $i++;
}

A possible bug: both foreach has the following condition with the same $form index:

if (isset($form['field_afl_dienst_adressen']['und'][$i][$z])) ...

Maybe you want to change the second one to

if (isset($form['field_bevoegde_adressen']['und'][$i][$z])) ...

It would make possible some other refactorings (fewer parameters etc.), as Victor T. suggested.


An OEIS Lookup tool in Python I'm from PPCG so I was making an esolang and I decided to write it in Python. Eventually it went from an esolang to an OEIS (Online Encyclopedia of Integer Sequences) lookup tool. I'm very new to Python.

Essentially this program takes an OEIS sequence number (e.g. 55 for sequence A000055) and the nth number in the sequence to return.

The code gets the OEIS page, parses it with BeautifulSoup, and returns the result, if it doesn't exist it returns "OEIS does not have a number in the sequence at the given index".

import sys, re
from urllib2 import*
from bs4 import BeautifulSoup

# Main logic
def execute(code, nth):
  # Decode input stuff
  num = int(code)
  try:
    f = urlopen("http://oeis.org/A%06d/list" % num)
    # global tree, data # Debugging
    # >:D I'm sorry but I have to use RegEx to parse HTML
    print {key: int(value) for key, value in
      re.findall( r'(\d+) (\d+)', re.sub(r'\D+', " ", re.sub(r'<[^>]+>', "",
        str(BeautifulSoup(f, "lxml").find(lambda tag:
          tag.name == "table" and # A table
          not tag.get('cellspacing') == "0" and # A table table
          len(tag.contents) > 1 # A two column table
        ))
      )) )
    }.get(nth, "OEIS does not have a number in the sequence at the given index")
  except HTTPError:
    print "Could not find sequence A%06d" % num
  except URLError:
    print "Could not connect to sources";
  except:
    print "Verify your numbers are correct"
    raise

if __name__ == '__main__':
  if len(sys.argv) > 1:
    execute(sys.argv[1], sys.argv[2])
  else:
    print """This is the OEIS lookup tool
You haven't entered the sequence""" % (LANGNAME)

As I'm new to Python. I'm using things I only kind of understand. My main concern is how I laid out this program. Especially the HTML parsing, I really doubt I'm doing that in the best way.

import*:

Please avoid importing all modules at all costs. Import each module you use separately

Regex ಠ_ಠ

Regex is evil, the worst. seriously. stop it. right now. kill python.exe right now and change it.

You use beautifulsoup, a dom parsing library, literally a few lines later, but you choose to use regex.

You sir, are evil.

I would suggest looking further at beautifulsoup, or taking a look at Scrapy, a scraping library for Python that makes use of generators to scrape large element sets (and even has cloud support!)

Lisp

Your code reads like lisp.

    ))
  )) )

Seriously, you need to format this better.

Try using Python's style guide, PEP8. when running your code through the online analyser, it threw 20 errors immediately.

When coding good Python, PEP8 should be your goto style document.

Looping

You shouldn't be looping like this:

print {key: int(value) for key, value in

If you want to reduce the code indentation level, then you should use separate variables to store each step in the process, or just simply use a standard loop.

String formatting

Instead of using % (myString), you should use string.format, as it's an improvement for these reasons:

less independant of parameter order
More readable

"My name is {name}, and I work for {work_name} as a {job_name}".format(name="Batman", work_name="Gotham City", job_name="Crimestopper")


Serialize name/value pairs I have an array of objects that each have a name/value property.  The array can contain multiple objects with the same name property. I want to serialize this array into the form:

name:value,value|name:value|name:value,value,value

So basically each property is separated by a | and each value by a ,. The above example is not exactly how I want it to look, just an example of the syntax, results will vary based upon input.

Here is an example of the array:

var objs = [
    {name:"name1", value: "value1"},
    {name:"name4", value: "value4"},
    {name:"name3", value: "value3"},
    {name:"name2", value: "value2"},
    {name:"name2", value: "value2"},
    {name:"name2", value: "value3"}
];

I wrote some code to perform this serialization, but I wanted to see if anyone could suggest improvements to this code, I know there are some Javascript experts on this site and wanted to get their opinion:

function serialize(objs){
    var out = "";
    for(var i = 0; i < objs.length; i++){
        var propKey = objs[i].name + ":";

        if (out.indexOf(propKey) == -1){
            out += "|" + propKey;
        }

        var position = out.indexOf(propKey) + propKey.length;
        out = out.substring(0, position) + objs[i].value + "," + out.substring(position);
    }
    return out.substring(1,out.length-1).replace(/\,\|/g,"|");
}

JS Fiddle: http://jsfiddle.net/BHsuM/

BTW jQuery is acceptable.

Why do you want it serialized specifically in that form? Just use the universal, built-in JSON serialization:

var serialized = JSON.stringify(myvar);
var reconstituted = JSON.parse(serialized);

Done, reconstituted is a normal object again.


Download file and connect: if one of them fails, go to step one My program....

Downloads a file
Connects to a server

If one of these operations were to fail, the program stalls for a minute and goes back to step one.

So, I would avoid recursion and the use of global variables. I think I did the first one right, but I need help to avoid the second one. What would I do if I need to "receive" variables from a function to a while loop?

public class Globals
{
    public static int sockConn = 0;
    public static bool sockRetry = false;
}

static void getInfo()
{
    while (true)
    {
        try
        {
            WebRequest request = WebRequest.Create(INFO_HOST + INFO_PATH);
            WebResponse response = request.GetResponse();

            string content;
            using (var sr = new StreamReader(response.GetResponseStream()))
            {
                content = sr.ReadToEnd();
            }

            string[] contentArray = content.Split(':');

            if (contentArray.Length == 2)
            {
                string serverIp = contentArray[0];
                string serverPortStr = contentArray[1];

                int serverPort = 5000;
                Int32.TryParse(serverPortStr, out serverPort);

                while (Globals.sockConn == 0)
                {
                    if (Globals.sockRetry == false)
                    {
                        connect(serverIp, serverPort);
                    }
                    else
                    {
                        Globals.sockRetry = false;
                        Thread.Sleep(60000);
                        break;
                    }
                }

                continue;
            }
            else
            {
                Thread.Sleep(60000);
                continue;
            }
        }
        catch
        {
            Thread.Sleep(60000);
            continue;
        }
    }
}

static void connect(string serverIp, int serverPort)
{
    Globals.sockConn = 1;

    try
    {
        TcpClient client = new TcpClient(serverIp, serverPort);
        NetworkStream nwStream = client.GetStream();
        StreamReader reader = new StreamReader(nwStream);
        StreamWriter writer = new StreamWriter(nwStream);

        Globals.sockConn = 2;

        string inputLine;

        while ((inputLine = reader.ReadLine()) != null)
        {
            ParseMessage(writer, serverIp, inputLine);
        }
    }
    catch
    {
        Globals.sockRetry = true;
        Globals.sockConn = 0;
        return;
    }
}

public static void ParseMessage(StreamWriter Writer, string serverIp, string msg)
{
    // simple parsing
    // function to upload conf file with using serverIp string
}

Globals & Public Static Fields

public class Globals
{
    public static int sockConn = 0;
    public static bool sockRetry = false;
}

You put a beginner tag on your post, so I'm going to try do be gentle: please kill this, with fire! :)
A non-static class that's called Globals, that only contains public static fields, smells. Why? Because in an object-oriented paradigm language, globals stink and make everything more complicated than need be.

Side note: naming.
C# coding standards recommend using camelCase for local variables and parameters. Public members should be PascalCase.

OOP Basics #1: Encapsulation
Avoid exposing public fields. Always. Not because they're static - because they're fields. Fields belong to a class instance (or, when they're static, they belong to a type), and should be considered implementation details that the outside world never needs to know about. Consider systematically encapsulating fields with properties:
private int sockConn = 0;
public int SocketConnectionCount { get { return sockConn; } set { sockConn = value; } }

In this case, there isn't really a gain with the backing field - so you can have the C# compiler make one for you, leaving you with a cleaner code file:
public int SockConnectionCount { get; set; }

This is called an "auto-property". Compiler magic! :)

Thinking in Objects
Think in terms of responsibilities - what is your code accomplishing?

Creating and sending a web request, receiving a web response.
Streaming the response into a string.
Splitting the string into an array; element 0 being the IP, element 1 being the port.
Connecting to the IP/port and receiving a message through TCP/IP.
Parsing the received message.

Break this down into their own methods/functions that do one thing, so simply that they can't possibly do it wrong. Then regroup the related ones into their own classes, and instantiate them.

Wait, something caught my eye and needs to be addressed:

int serverPort = 5000;
Int32.TryParse(serverPortStr, out serverPort);

out serverPort is an out parameter - the method it's passed into has to initialize it before it returns. In other words, the 5000 is never used. And you're never using the TryParse method's return value either (it's a Boolean that tells you whether the parse succeeded or not). out parameters are the only time you can pass an uninitialized variable into a method and have nothing to worry about. The above lines should read something like this:
int serverPort;
var result = int.TryParse(serverPortStr, out serverPort);
// do something if result is true

Avoid Hungarian-ish postfixes (and prefixes as well); If you need to specify that a variable is a String, it's because you haven't found a truly meaningful name for it. Notice I called the TryParse method on the int type. It's the very same thing as Int32 - in fact, int is a C# language shortcut for System.Int32; both are perfectly interchangeable... but it's better to choose one, and stick to it, for consistency's sake! :)


Printing a score sheet for baseball batters I've been learning Python 2.7 for about a week now and have written a quick program to ask for names of batters and printing a score sheet. I have then added the names to a list which I will use more in other future functions.

I have a few questions:

I feel like there is too much repetition in my code, such as the prompting for each batter and all the printing. I am trying to figure out how I could use a loop to do these for me. Since I know I need 11 batters a for loop would be best but I need to make a new variable each time, how can I do this?
I have been reading around about the use of global. I can't seem to get a definite answer but is this the correct use if I want to use a variable from one function in multiple others?

def Input_Players():

    global First, Second, Third, Fourth, Fith, Sixth, Seventh, Eigth, Ninth, Tenth, Eleventh

    First = [raw_input("Name of 1st Batsman > ")]
    Second = [raw_input("Name of 2nd Batsman > ")]
    Third = [raw_input("Name of 3rd Batsman > ")]
    Fourth = [raw_input("Name of 4th Batsman > ")]
    Fith = [raw_input("Name of 5th Batsman > ")]
    Sixth = [raw_input("Name of 6th Batsman > ")]
    Seventh = [raw_input("Name of 7th Batsman > ")]
    Eigth = [raw_input("Name of 8th Batsman > ")]
    Ninth = [raw_input("Name of 9th Batsman > ")]
    Tenth = [raw_input("Name of 10th Batsman > ")]
    Eleventh = [raw_input("Name of 11th Batsman > ")]

def Print_Score_Sheet():
    total_chars = 56

    Batsman = First + Second + Third + Fourth + Fith + Sixth + Seventh + Eigth + Ninth + Tenth + Eleventh

    print "1." + Batsman[0] + "." * (total_chars - len(Batsman[0]) + 2)
    print "2." + Batsman[1] + "." * (total_chars - len(Batsman[1]) + 2)
    print "3." + Batsman[2] + "." * (total_chars - len(Batsman[2]) + 2)
    print "4." + Batsman[3] + "." * (total_chars - len(Batsman[3]) + 2)
    print "5." + Batsman[4] + "." * (total_chars - len(Batsman[4]) + 2)
    print "6." + Batsman[5] + "." * (total_chars - len(Batsman[5]) + 2)
    print "7." + Batsman[6] + "." * (total_chars - len(Batsman[6]) + 2)
    print "8." + Batsman[7] + "." * (total_chars - len(Batsman[7]) + 2)
    print "9." + Batsman[8] + "." * (total_chars - len(Batsman[8]) + 2)
    print "10." + Batsman[9] + "." * (total_chars - len(Batsman[9]) + 1)
    print "11." + Batsman[10] + "." * (total_chars - len(Batsman[10]) + 1)

Input_Players()
Print_Score_Sheet()

I would structure the code like this:

def input_players(total=11):
  players = []

  for index in range(total):
    player = raw_input('Name of Batsman {} > '.format(index + 1))
    players.append(player)

  return players

def print_score_sheet(batsmen, total_chars=56):
  for index, batsman in enumerate(batsmen, 1):
    print '{}. {} '.format(index, batsman).ljust(total_chars, fillchar='.')

if __name__ == '__main__':
  players = input_players(11)
  print_score_sheet(players)

Some tips:

If you use a set of variables like Player1, Player2, PlayerN, you should be using a list to store them.
Your functions should do what they say. input_players() should return a list of players, nothing more, nothing less.
Get into the habit of including the if __name__ == '__main__': block. The stuff inside of the block is executed only if you run the Python script directly, which is good if you have multiple modules.


Byte-to-binary conversion function My implementation of a binary-to-bits converting function:

#include <stdio.h>
#include <limits.h>

unsigned char *return_byte (unsigned char source)
{
    static unsigned char byte[CHAR_BIT];
    unsigned char *p = byte, i;

    for(i = 0; i < CHAR_BIT; i++)
        p[i] = '0' + ((source & (1 << i)) > 0);

    return p;
}

int main(void)
{
    unsigned char val = 200;

    printf("Value:\t%i\nBinary:\t%s", val, return_byte(val));

    return 0;
}

Output:

Value:  200
Binary: 00010011

return_byte doesn't tell much what the function is intended to do. btoa would be more in line with C naming convention (akin to itoa)
A p variable is pretty much useless. Consider

static unsigned char byte[CHAR_BIT];
unsigned char i;

for(i = 0; i < CHAR_BIT; i++)
    byte[i] = '0' + ((source & (1 << i)) > 0);

return byte;

Using an unsigned char for indexing is questionable. In any case it doesn't buy you much against a native int.
A boolean expression as + operand may raise some eyebrows (true is guaranteed to be a non-zero in an arithmetic context, but it is not guaranteed to be 1). I recommend to force an arithmetic value by right-shifting source instead of left-shifting the mask:

for(i = 0; i < CHAR_BIT; i++) {
    byte[i] = '0' + (source & 1);
    source >>= 1;
}

You do not null-terminate the resulting string. The fact that your test succeeded is a sheer (un)luck.
You probably know that returning static makes the code non-reentrant and thread-unsafe. You may want to let client provide a space for a resulting string.


Simple PHP project to store inputs in MySQL and output them As my first simple PHP project, I've created a simple project that allows you to just enter your name and two numbers, where it displays them on the page and stores them in a database. It consists of 3 files: index.php, db.php & process.php. I wonder whether I have created the code efficiently.

I'm curious about all the talk behind procedural PHP vs OOP. I initially used some deprecated PHP5 functions and was corrected by the stack overflow community, but this has left me confused as to what specifically I should learn with PHP as a beginner. It sounds like procedural and OOP are very different, and that I should only pick one and learn that.

So my questions are as follows:

Is the coding in my project acceptable?
Is it similar to how most PHP projects are built? (i.e. are most procedural or OOP)
What should I learn as a beginner? (procedural or OOP?)
Do you have any beginner advice?

Any advice here would be very much appreciated as I'm new to PHP. Thanks for any help here. The files and code (excluding HTML & CSS) are below:

index.php:

<?php include 'db.php'; ?>

<?php
// Fetch data from db
$query = mysqli_query($con, 'SELECT * FROM data');
?>

            <?php while ($row = $query->fetch_assoc()) : ?>

<?php echo $row['t_name'] ?> entered <?php echo $row['t_firstNo'] ?> and <?php echo $row['t_secondNo'] ?> (result =
                    <?php
                        $first = $row['t_firstNo'];
                        $second = $row['t_secondNo'];
                        $result = $first * $second;
                        echo $result;
                    ?>)

            <?php endwhile ?>

            <?php if(isset($_GET['error'])) : // if error variable is there ?>
                <?php echo $_GET['error']; // get the error ?>
            <?php endif ?>

db.php:

<?php

// Connect
$con = mysqli_connect("localhost", "tnnick", "PHPprojects", "timesNumbers");

// Test Connection
if (mysqli_connect_errno()) {
    echo 'Failed: '.mysqli_connect_error();
}

?>

process.php:

<?php include 'db.php' ; ?>

<?php
    if(isset($_POST['submit'])) { // if form submitted
        $nickname = mysqli_real_escape_string($con, $_POST['nickname']); // get the inputs
        $firstNo = mysqli_real_escape_string($con, $_POST['firstNo']);
        $secondNo = mysqli_real_escape_string($con, $_POST['secondNo']);
    }
?>

<?php
// validate inputs
if(!isset($nickname) || $nickname=='' || !isset($firstNo) || $firstNo=='' || !isset($secondNo) || $secondNo=='' ) {
    // if invalid input, send error to index.php
    $error = 'Bad input - try again.'; // create error message
    header("Location: index.php?error=".urlencode($error)); // attach to URL & send to index.php
    exit();
} else {
    // insert into DB
    $putIntoDB = "INSERT INTO data (t_name, t_firstNo, t_secondNo) VALUES ('$nickname','$firstNo','$secondNo')";

    // check it inserted
    if(!mysqli_query($con, $putIntoDB)) {
        die('Error');
    } else {
        header("Location: index.php");
        exit();
    }
}
?>

Is the coding in my project acceptable?

More or less. There are no essential flaws but the structure is quite questionable.

Is it similar to how most PHP projects are built? (i.e. are most procedural or OOP)

Depends on the project's maturity. Your code is quite similar to what a usual first project looks like, and even above the average. Speaking of mature projects, they indeed are mostly written using OOP and essentially different from what you wrote here.

What should I learn as a beginner? (procedural or OOP?)

First of all, there is nothing wrong with procedural programming, it's perfectly OK to start from it. Eventually you will learn some OOP and may be even master it.
Besides, it looks like you are confusing OOP which is indeed opposite to procedural programming with PDO which is just a PHP extension to work with databases (though using OOP syntax).
PDO is better than mysqli which you are using at the moment, so to answer your questions about procedural/OOP/PDO:

Learn procedural PHP with occasional use of built-in objects
Better to use PDO for the database access instead of mysqli

Do you have any beginner advice?

Just write a code and post it here for the review.
Now to your code
db.php
this file is better to be rewritten (in case you are going to stick with mysqli) according to my article How to properly connect to Mysql database using mysqli.
index.php
is better to be split into two parts, the data gathering part and the output part. Such a separation will make both parts cleaner:
<?php
include 'db.php';
// Fetch data from db
$query = mysqli_query($con, 'SELECT *, t_firstNo * t_secondNo as result FROM data');
// the following function will give you an array
$data = mysqli_fetch_all($query);
?>
<?php foreeach ($data as $row) : ?>
    <li>
        <?= $row['t_name'] ?> entered
        <?= $row['t_firstNo'] ?> and
        <?= $row['t_secondNo'] ?>
        (result = <?= $row['result'] ?>)
    </li>
<?php endwhile ?>

as you can see, we calculated the result right in the query, which made our output code cleaner and focused on the output. Getting data into array before output is also a good practice, completely separating the database interaction from the output.
process.php
<?php
if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    if(empty($_POST['nickname']) || empty($_POST['firstNo']) || empty($_POST['secondNo'])) {
        echo 'Bad input - try again.';
    } else {
        include 'db.php';
        // insert into DB
        $sql = "INSERT INTO data (t_name, t_firstNo, t_secondNo) VALUES (?,?,?)";
        $stmt = mysqli_prepare($sql);
        $stmt->bind_param("sss", $_POST['nickname'], $_POST['firstNo'], $_POST['secondNo']);
        $stmt->execute();
        header("Location: index.php");
        exit();
    }
}
?>
<form>
Here goes your input form

some notes

it's better to use $_SERVER['REQUEST_METHOD'] if POST method is used.
empty() is essentially a shorthand of isset($var) || $var==''.
it's better to keep the form and its processing on the same page. it can let you to fill the entered values for the convenience.
it is much, much better to use a prepared statement as opposite to the manual escaping you were using, so you should stick with the former.
there is no need to check for the success, your application should be just configured to say "Error" in case of error automatically.


Converting pseudocode for max subarray into working Java code I have been tasked with converting some pseudocode to working Java code and I would like someone to review my code for correctness.

The pseudocode is as follows:

Algorithm MaxsubFaster(A):
Input: An n-element array A of numbers, indexed from 1 to n.
Output: The maximum subarray sum of array A.
S0 ← 0 // the initial prefix sum
for i ← 1 to n do
    Si ← Si−1 + A[i]
m ← 0 // the maximum found so far
for j ← 1 to n do
    for k ← j to n do
        s = Sk − Sj−1
        if s > m then
            m ← s
return m

My code is as follows:

public static int maxSubFaster(int[] array)
    {
        Integer S[] = new Integer[array.length];
        Arrays.fill(S,0);

        //initial prefix sum
        S[0] = 0;
        for (int i = 1; i < array.length; i++)
        {
            //S_i = S_i-1 + A[i]
            S[i] = S[i - 1] + array[i];
        }
        //maximum found so far
        int max = 0;
        for (int j = 1; j < array.length; j++)
        {
            for (int k = j; k < array.length; k++)
            {
                //s = S_k - S_j-1
                int s = S[k] - S[j - 1];
                //if s > m, m <- s
                if (s > max)
                {
                    max = s;
                }
            }
        }
        return max;
    }//end maxSubFaster

The algorithm seems to work, but any pointers or corrections would be great!

Your translation is pretty faithful to the already low-level pseudocode but you could:

The pseudocode talks about an array of numbers and not integers, but that is just a simple type change, and doesn't affect the logic at all
copy array.length into variable n near the top, and use n throughout as the original code does,
remove the initial filling of S with 0's, since your first loop will overwrite them anyway (and that was not in the original code)
change max to m, and array to A, but you could argue this makes your code more readable

This probably isn't the point of the exercise, but it is possible to solve this problem with a single loop that scans through the array just once, for a more efficient solution.


Image upload script more cleaner and secure using php 7 oop style. I want to utilize namespaces and make the code on this file uploading script more secure, and cleaner looking, here is what i have so far

it uploads an image and inserts it into a database.

I want to be able to use namespaces within view files, without it looking messy.

i know i can use laravel blade etc, but for the sake of sharpening my php skills, i want to be able to do this from scratch.

Image.php

<?php

require 'Db.php';

class Image{

    private $dbh;
    private $connect;
    public $directory;
    public $uploadfile;

    public function __construct()
    {
        $this->dbh = new Db();
        $this->connect =  $this->dbh->connect();
        $this->directory = "uploads/";
    }

    public function upload_image($filename, $image_name)
    {

        $this->uploadfile = $this->directory . basename($filename);

        try{
            $stmt = $this->connect->prepare("INSERT INTO images (img, image_name) VALUES (:img, :imagename) ");
            $stmt->bindparam(':img', $this->uploadfile);
            $stmt->bindparam(':imagename', $image_name);
            $stmt->execute();
            return $stmt;
        }
        catch(PDOExeception $e)
        {
            echo $e->getMessage();
        }

    }

    public function get_image()
    {
        return $this->uploadfile;
    }

}

upload_image.php (view file)

    <form action="/ImgUpload.php" enctype="multipart/form-data" method="post">

            File input
            <input type="file" name="profile_img" class="form-control-file" id="exampleInputFile" aria-describedby="fileHelp">
            <input type="text" name="image_name" placeholder="Enter Name Of File" class="mt-3">
            This is some placeholder block-level help text for the above input. It's a bit lighter and easily wraps to a new line.
            <button type="submit" class="btn btn-primary">Submit</button>

        </form>

ImgUpload.php

<?php
session_start();
require_once 'Image.php';

if(isset($_FILES['profile_img'])){

    $image = new Image();

    $filename = $_FILES["profile_img"]["name"];
    $realname = "uploads/" . basename($filename);
    $directory = "/Applications/MAMP/htdocs/eliphp1/uploads/";

    $path = $directory . basename($filename);

    $image_name = $_POST['image_name'];

    if($image->upload_image($realname, $image_name)){

        move_uploaded_file($_FILES["profile_img"]["tmp_name"], $path);

        echo 'get_image().'">';

    };

};

?>

To elaborate on the answer from Stexxe...

  it can have connection to database ($this->connect of your Image class) as dependency

A database class is different from all other classes. It cannot be created any time on demand. It has to be created only once, connect to a database, and hold this connection all the time. Otherwise numerous connections from the same script will crash your database server.

So it has to be:

class ImageMapper{

    private $connect;

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

and called like this:

$dbh = new Db();
$connect =  $dbh->connect();

$image = new Image($connect);

  manipulation images classes should not expose any information about their inner workings

Among other things, it means that error reporting should never reveal the actual error to a site user. Therefore you should never ever do anything like die($error) -- which also makes the whole try..catch business misplaced in this code.  So it should be:

public function upload_image($filename, $image_name)
{
    $sql = "INSERT INTO images (img, image_name) VALUES (?, ?)";
    $this->connect->prepare($sql)->execute([$filename, $image_name]);
}

whereas a possible exception thrown by PDO must be caught elsewhere. Learn more from my article on PHP error reporting.

Some other stuff has also been fixed, mostly your extremely confusing way assigning a filename, when you are adding, removing and re-adding the directory name. Some useless variables like $directory and $filename are also removed.


Using if statement in PHP I would like to do these actions step by step:

first DB update
copy file
unlink file
second DB update

It is working, but I don't know if my code is correct/valid:

$update1 = $DB->query("UPDATE...");

  if ($update1)
  {
    if (copy("..."))
    {
      if (unlink("..."))
      {
        $update2 = $DB->query("UPDATE ...");
      }
    }
  }

Is it possible to use if statement this way?

I found that it is usually used with PHP operators and PHP MySQL select, for example:

$select = $DB->row("SELECT number...");
  if ($select->number == 2) {
  ...
  }

Is it possible to use if statement this way?

Yes.

Whether it is a good style depends on other factors.

Let us look at a few alternatives. First, your code (with cuddled braces):

if ( $update1 ) {
    if ( copy( "..." ) ) {
        if ( unlink( "..." ) ) {
            $update2 = $DB->query( "UPDATE ..." );
        }
    }
}

Then the same code using logical AND (&&).

if ( $update1 && copy( "..." ) && unlink( "..." ) ) {
    $update2 = $DB->query( "UPDATE ..." );
}

And another one without the braces {};

if ( $update1 && copy( "..." ) && unlink( "..." ) )
    $update2 = $DB->query( "UPDATE ..." );

And finally, an example that doesn't use if.

$update1
    && copy( '...' )
    && unlink( '...' ) )
    && $update2 = $DB->query( "UPDATE ..." );

All these examples perform the same operations.

If the ... stuff is long, the latter methods can get very wide conditionals making it difficult to read the code to see what is going on.

if ( $update1 && copy( 'a/very/long/source/path', 'a/very/long/destination/path' ) && unlink( 'a/very/long/source/path' ) )
    $update2 = $DB->query( "UPDATE ..." );

You could rewrite it like this:

if (   $update1
    && copy( 'a/very/long/source/path', 'a/very/long/destination/path' )
    && unlink( 'a/very/long/source/path' ) )
    $update2 = $DB->query( "UPDATE ..." );

But it looks little clunky. It is hard to see where the conditional ends.

The last style makes this prettier:

$update1
    && copy( 'a/very/long/source/path', 'a/very/long/destination/path' )
    && unlink( 'a/very/long/source/path' ) )
    && $update2 = $DB->query( "UPDATE ..." );

I do not care for this style, but it looks pretty and it tells you exactly what is happening at a glance.

One problem with most of the alternatives to the first method is debugging and maintenance.

If I want to log the unlink error is some special way I can do this in the first method:

if ( $update1 ) {
    if ( copy( "..." ) ) {
        if ( unlink( "..." ) ) {
            $update2 = $DB->query( "UPDATE ..." );
        } else {
            log_unlink_error( '...' );
        }
    }
}

To quickly add that code to the other methods described here, may require more rewriting.

So, in this case, your style for handling multiple conditions might depend on the age of your code and how much testing you expect to be doing.

If it is well tested and you never need to edit it, then one of the alternatives is fine if you use that style consistently throughout all your code.
If this is new code, write it out the long way. It may be easier to test and maintain.