How to shorten this terrible HTTP header parser? I am trying to read a line ending in \r\n from a Handle. This is an HTTP header.

I’m fairly new to functional programming, so I tend to use a lot of case expressions and stuff. This makes the code very long and ugly.

handleRequest :: HostName -> Handle -> IO ()
handleRequest host handle = do
    requestLine <- readHeaderLine
    putStrLn $ requestLine ++ "\n-------------------"
    -- FIXME: This code is bad, and its author should feel bad.
    where
        readHeaderLine = do
            readHeaderLine' ""
            where
                readHeaderLine' s = do
                    chr <- hGetChar handle
                    case chr of
                        '\r' -> do
                            nextChr <- hGetChar handle
                            case nextChr of
                                '\n' -> return s
                                _    -> readHeaderLine' $ s ++ [chr, nextChr]
                        _    -> readHeaderLine' $ s ++ [chr]

How can I reduce the amount of case expressions in this code? I thought of using Parsec, but that seemed overkill to me for something this trivial, and I don’t know how well it works with Handles.

Here I assume you just want to see how your current code can be improved without changing the algoritm.

First of all, give HLint tool a chance to suggest you obvious improvements. In your case the only improvement was that do in do readHeaderLine' "" was redundant, so not much.

Second, in my opinion many small top-level definitions are better than few large ones. You can still control namespace pollution by not exporting definitions local to the module:

import System.IO

type HostName = String

handleRequest :: HostName -> Handle -> IO ()
handleRequest host handle = do
    requestLine <- readHeaderLine handle
    putStrLn $ requestLine ++ "\n-------------------"

-- FIXME: This code is bad, and its author should feel bad.
readHeaderLine handle = readHeaderLine' "" where
    readHeaderLine' s = do
        chr <- hGetChar handle
        case chr of
            '\r' -> do
                nextChr <- hGetChar handle
                case nextChr of
                    '\n' -> return s
                    _    -> readHeaderLine' $ s ++ [chr, nextChr]
            _    -> readHeaderLine' $ s ++ [chr]

Next, your nextChr <- hGetChar handle ; case nextChr of limes appear twice, so you can extract a function and reduce code duplication.

Thanks to purity, you can do pretty mechanically:

foo handle quux bar baz = do
    chr <- hGetChar handle
    if chr == quux then bar else baz

Now replace the two fragments with calls to foo. Let's start with inner one:

readHeaderLine handle = readHeaderLine' "" where
    readHeaderLine' s = do
        chr <- hGetChar handle
        case chr of
            '\r' -> do
                foo handle '\n' (return s) (readHeaderLine' $ s ++ [chr, nextChr])
            _    -> readHeaderLine' $ s ++ [chr]

Heh, it didn't work because nextChr is only known inside foo. No problem, pass it as parameter to baz branch and use lambda to catch it:

readHeaderLine handle = readHeaderLine' "" where
    readHeaderLine' s = do
        chr <- hGetChar handle
        case chr of
            '\r' -> do
                foo handle '\n' (return s) (\nextChr -> readHeaderLine' $ s ++ [chr, nextChr])
            _    -> readHeaderLine' $ s ++ [chr]

foo handle quux bar baz = do
    chr <- hGetChar handle
    if chr == quux then bar else baz chr

So now you can do the outer one.

readHeaderLine handle = readHeaderLine' "" where
    readHeaderLine' s = do
    foo handle '\r' (foo handle '\n' (return s) (\nextChr -> readHeaderLine' $ s ++ [chr, nextChr])) (\chr -> readHeaderLine' $ s ++ [chr])

No luck again as first chr is nowhere to get from. Fortunately we know it's always '\r', so

readHeaderLine handle = readHeaderLine' "" where
    readHeaderLine' s = do
    foo handle '\r' (foo handle '\n' (return s) (\nextChr -> readHeaderLine' $ s ++ ['\r', nextChr])) (\chr -> readHeaderLine' $ s ++ [chr])

As line got too long, we can split it, removing another redundant do:

readHeaderLine handle = readHeaderLine' "" where
    readHeaderLine' s = foo handle '\r' haveCR noCR where
        haveCR = foo handle '\n' (return s) haveCRnoLF
        noCR chr = readHeaderLine' $ s ++ [chr]
        haveCRnoLF nextChr = readHeaderLine' $ s ++ ['\r', nextChr]

Now there are more repeated patterns to eliminate: foo handle and readHeaderLine' $ s ++. To remove foo handle we move foo back in and remove its handle parameter both from applications and definition as it's now accessible from closure:

readHeaderLine handle = readHeaderLine' "" where
    readHeaderLine' s = foo '\r' haveCR noCR where
        haveCR = foo '\n' (return s) haveCRnoLF
        noCR chr = readHeaderLine' $ s ++ [chr]
        haveCRnoLF nextChr = readHeaderLine' $ s ++ ['\r', nextChr]
        foo quux bar baz = do
            chr <- hGetChar handle
            if chr == quux then bar else baz chr

To eliminate readHeaderline' repeated patterns we extract them into recurse local function:

readHeaderLine handle = readHeaderLine' "" where
    readHeaderLine' s = foo '\r' haveCR noCR where
        haveCR = foo '\n' (return s) haveCRnoLF
        noCR chr = recurse [chr]
        haveCRnoLF nextChr = recurse ['\r', nextChr]
        recurse x = readHeaderLine' $ s ++ x
        foo quux bar baz = do
            chr <- hGetChar handle
            if chr == quux then bar else baz chr

This is how far you can get with mechanical code deduplication. Now it's time for heavier weapons. You can still:

separate recursive code from non-recursive code
separate monadic code from non-monadic code

The initial redHeaderLine' call can be implemented using recurse with an extra parameter:

readHeaderLine handle = recurse [] [] where
    recurse s x = readHeaderLine' $ s ++ x
    readHeaderLine' s = foo '\r' haveCR noCR where
        haveCR = foo '\n' (return s) haveCRnoLF
        noCR chr = recurse s [chr]
        haveCRnoLF nextChr = recurse s ['\r', nextChr]
        foo quux bar baz = do
            chr <- hGetChar handle
            if chr == quux then bar else baz chr

Now we can inline readHeaderLine' as it is only applied once:

readHeaderLine handle = recurse [] [] where
    recurse s1 x = foo '\r' haveCR noCR where
        s = s1 ++ x
        haveCR = foo '\n' (return s) haveCRnoLF
        noCR chr = recurse s [chr]
        haveCRnoLF nextChr = recurse s ['\r', nextChr]
        foo quux bar baz = do
            chr <- hGetChar handle
            if chr == quux then bar else baz chr

And we can remove duplication of recurse s:

readHeaderLine handle = recurse [] [] where
    recurse s1 x = foo '\r' haveCR noCR where
        s = s1 ++ x
        rf = recurse s
        haveCR = foo '\n' (return s) haveCRnoLF
        noCR chr = rf [chr]
        haveCRnoLF nextChr = rf ['\r', nextChr]
        foo quux bar baz = do
            chr <- hGetChar handle
            if chr == quux then bar else baz chr

Now let's put return value of recurse into a local declaration g:

readHeaderLine handle = recurse [] [] where
    recurse s1 x = g where
        s = s1 ++ x
        rf = recurse s

        g = foo '\r' haveCR noCR
        haveCR = foo '\n' (return s) haveCRnoLF
        noCR chr = rf [chr]
        haveCRnoLF nextChr = rf ['\r', nextChr]
        foo quux bar baz = do
            chr <- hGetChar handle
            if chr == quux then bar else baz chr

Our goal is to divorse g from recurse. You can do it by adding parameters to both recurse and g and localizing identifiers used only in recurse and used only in g:

readHeaderLine handle = recurse g [] [] where
    recurse g s1 x = g rf s where
        s = s1 ++ x
        rf = recurse g s

g rf s = foo '\r' haveCR noCR where
    haveCR = foo '\n' (return s) haveCRnoLF
    noCR chr = rf [chr]
    haveCRnoLF nextChr = rf ['\r', nextChr]
    foo quux bar baz = do
        chr <- hGetChar handle
        if chr == quux then bar else baz chr

Now recurse is completely self-contained:

readHeaderLine handle = recurse g [] [] where
    g rf s = foo '\r' haveCR noCR where
        haveCR = foo '\n' (return s) haveCRnoLF
        noCR chr = rf [chr]
        haveCRnoLF nextChr = rf ['\r', nextChr]
        foo quux bar baz = do
            chr <- hGetChar handle
            if chr == quux then bar else baz chr

recurse g s1 x = g rf s where
    s = s1 ++ x
    rf = recurse g s

But g is still recursive: it has a nasty rf parameter which is an indirect recursive application. We need to move rf into recurse too. So here comes a trick: convert a function call into a constructor.

g can have only 3 return values: return s, rf [chr] and rf ['\r', nextChr]. We can represent them with a data type and return it instead of calling return or rf:

data Outcomes a b c = RF1 a | RF2 b | Return c

readHeaderLine handle = recurse g [] [] where
    g rf s = foo '\r' haveCR noCR where
        haveCR = foo '\n' (return $ Return s) haveCRnoLF
        noCR chr = return $ RF1 [chr]
        haveCRnoLF nextChr = return $ RF2 ['\r', nextChr]
        foo quux bar baz = do
            chr <- hGetChar handle
            if chr == quux then bar else baz chr

recurse g s1 x = analyzeOutcomes $ g rf s where
    s = s1 ++ x
    rf = recurse g s
    analyzeOutcomes outcomeM = do
        outcome <- outcomeM
        case outcome of
            RF1 a -> rf a
            RF2 a -> rf a
            Return a -> return a

Now rf parameter is unused, so we can clean the definitions of g and recurse:

data Outcomes a b c = RF1 a | RF2 b | Return c

readHeaderLine handle = recurse g [] [] where
    g s = foo '\r' haveCR noCR where
        haveCR = foo '\n' (return $ Return s) haveCRnoLF
        noCR chr = return $ RF1 [chr]
        haveCRnoLF nextChr = return $ RF2 ['\r', nextChr]
        foo quux bar baz = do
            chr <- hGetChar handle
            if chr == quux then bar else baz chr

recurse g s1 x = analyzeOutcomes $ g s where
    s = s1 ++ x
    rf = recurse g s
    analyzeOutcomes outcomeM = do
        outcome <- outcomeM
        case outcome of
            RF1 a -> rf a
            RF2 a -> rf a
            Return a -> return a

Now two more improvements: a) RF1 and RF2 outcomes can be joined into one outcome as they are handled uniformly and have the same types; b) the only reason we pass s is to return it in Return outcome, so we can eliminate s argument of g too.

data Outcomes a = RF a | Return

readHeaderLine handle = recurse g [] [] where
    g = foo '\r' haveCR noCR where
        haveCR = foo '\n' (return Return) haveCRnoLF
        noCR chr = return $ RF [chr]
        haveCRnoLF nextChr = return $ RF ['\r', nextChr]
        foo quux bar baz = do
            chr <- hGetChar handle
            if chr == quux then bar else baz chr

recurse g s1 x = analyzeOutcomes g where
    s = s1 ++ x
    rf = recurse g s
    analyzeOutcomes outcomeM = do
        outcome <- outcomeM
        case outcome of
            RF a -> rf a
            Return -> return s

Now rf and analyzeOutcomes are used only once and Outcomes type became the same as Maybe. So:

readHeaderLine handle = recurse g [] [] where
    g = foo '\r' haveCR noCR where
        haveCR = foo '\n' (return Nothing) haveCRnoLF
        noCR chr = return $ Just [chr]
        haveCRnoLF nextChr = return $ Just ['\r', nextChr]
        foo quux bar baz = do
            chr <- hGetChar handle
            if chr == quux then bar else baz chr

recurse outcomeM s1 x = do
    outcome <- outcomeM
    let s = s1 ++ x in case outcome of
        Just a -> recurse outcomeM s a
        Nothing -> return s

Now s1 and x are only used in recurse to construct s. We can then construct s outside of recurse and pass it. Also, g now can be inlined.

readHeaderLine handle = recurse (foo '\r' haveCR noCR) [] where
    haveCR = foo '\n' (return Nothing) haveCRnoLF
    noCR chr = return $ Just [chr]
    haveCRnoLF nextChr = return $ Just ['\r', nextChr]
    foo quux bar baz = do
        chr <- hGetChar handle
        if chr == quux then bar else baz chr

recurse outcomeM s = do
    outcome <- outcomeM
    case outcome of
        Just a -> recurse outcomeM (s ++ a)
        Nothing -> return s

Now note that outcomeM is just a constant and it is not changed across recursive calls. So we can proceed further with our splitting of recursive and non-recursive code:

recurse outcomeM s = f s where
    f s = do
        outcome <- outcomeM
        case outcome of
            Just a -> f (s ++ a)
            Nothing -> return s

And duplicate return $ Just can be moved inside foo:

readHeaderLine handle = recurse (foo '\r' haveCR noCR) [] where
    haveCR = foo '\n' (return Nothing) haveCRnoLF
    noCR chr = [chr]
    haveCRnoLF nextChr = ['\r', nextChr]
    foo quux bar baz = do
        chr <- hGetChar handle
        if chr == quux then bar else return (Just $ baz chr)

After renaming of nonsense identifiers in definition of foo we get:

readHeaderLine handle = recurse (match '\r' haveCR noCR) [] where
    haveCR = match '\n' (return Nothing) haveCRnoLF
    noCR chr = [chr]
    haveCRnoLF nextChr = ['\r', nextChr]
    match expectedChar onSuccess onFailure = do
        actualChar <- hGetChar handle
        if actualChar == expectedChar
            then onSuccess
            else return (Just $ onFailure actualChar)

recurse outcomeM s = f s where
    f s = do
        outcome <- outcomeM
        case outcome of
            Just a -> f (s ++ a)
            Nothing -> return s


Simple text DataBase So I made this simple text database system for storing a list of objects because I hate using other database programs. But I was just wondering what you guys think I could do to make this faster and less resource intensive.

package com.gmail.welsar55.WDB;

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.File;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.io.PrintWriter;
import java.util.ArrayList;

public class Managing {

    public static String mime = "wtdb";

    public static boolean addObject(File file, Object object) throws IOException
    {
        if(file.getName().endsWith(mime))
        {
            PrintWriter pw = new PrintWriter(new BufferedWriter(new FileWriter(file, true)));
            pw.println(object.toString());
            pw.close();
            return true;
        }
        else
        {
            return false;
        }
    }

    public static boolean containsObject(File file, Object object) throws IOException
    {
        if(file.getName().endsWith(mime))
        {
            BufferedReader br = new BufferedReader(new FileReader(file));
            boolean IsGood = false;
            while(br.ready())
            {
                String line = br.readLine();
                if(line.trim().equals(object.toString()))
                {
                    IsGood = true;
                }
            }
            br.close();
            return IsGood;
        }
        else
        {
            return false;
        }
    }

    public static boolean removeObject(File file, Object object) throws IOException
    {
        if(file.getName().endsWith(mime))
        {
            BufferedReader br = new BufferedReader(new FileReader(file));
            boolean IsGood = false;
            ArrayList<String> linestoadd = new ArrayList<String>();
            while(br.ready())
            {
                String line = br.readLine();
                if(!line.trim().equals(object.toString()))
                {
                    linestoadd.add(line.trim());
                }
                else
                {
                    IsGood = true;
                }
            }
            br.close();
            BufferedWriter fileclearer = new BufferedWriter(new FileWriter(file));
            fileclearer.write("");
            fileclearer.close();
            PrintWriter pw = new PrintWriter(new BufferedWriter(new FileWriter(file, true)));
            for(String linetoadd : linestoadd)
            {
                pw.println(linetoadd);
            }
            pw.close();
            return IsGood;

        }
        else
        {
            return false;
        }
    }

    public static ArrayList<String> getContents(File file) throws IOException
    {
        ArrayList<String> contents = new ArrayList<String>();
        if(file.getName().endsWith(mime))
        {
            BufferedReader br = new BufferedReader(new FileReader(file));
            while(br.ready())
            {
                contents.add(br.readLine());
            }
            br.close();
        }
        return contents;
    }
}

Reduce duplicated logic by using helper methods

Instead of repeating this condition:

if (file.getName().endsWith(mime)) {

Use a helper method:

private static boolean isValidFile(File file) {
    return file.getName().endsWith(mime);
}

The advantage is that this hides the implementation details of what is a valid file to you. Without this, all you methods need to be aware that the file name should end with mime, which is too much detail and duplicated logic. With the isValidFile helper, if you need to change something about the condition on the File object, you will be able to make the change in one place, inside isValidFile, without having to touch the rest of the program.

Bugs and improvements in containsObject

You don't exit the loop even after you found a match, iterating until the end of the file, which is a waste:

while (br.ready()) {
    String line = br.readLine();
    if (line.trim().equals(object.toString())) {
        IsGood = true;
    }
}

After you set IsGood = true, add a break;

IsGood is a bad name for 2 reasons:

Variables should be camelCase
It doesn't explain it self

foundMatch would make more sense.

Simplify and clean up removeObject

The "file clearer" is completely pointless. Instead of this:

BufferedWriter fileclearer = new BufferedWriter(new FileWriter(file));
fileclearer.write("");
fileclearer.close();
PrintWriter pw = new PrintWriter(new BufferedWriter(new FileWriter(file, true)));

You can write simply this for the same effect:

PrintWriter pw = new PrintWriter(new BufferedWriter(new FileWriter(file)));

Notice that I dropped the second true parameter from the the FileWriter when creating pw. This makes the writer truncate the file.

Instead of this:

ArrayList<String> linestoadd = new ArrayList<String>();

You should declare variables with interface types, in this case a List:

List<String> linestoadd = new ArrayList<String>();

And instead of linestoadd, I would name the variable content, or contentLines, but that may be a matter of taste.

Again, IsGood doesn't tell what it's about. If you rename to foundMatch, its purpose becomes much clearer.

Improve getContents

As I noted earlier about linestoadd in removeObject, you should declare variables, and also methods, using interface types. So instead of:

public static ArrayList<String> getContents(File file) throws IOException {
    ArrayList<String> contents = new ArrayList<String>();

This would be better:

public static List<String> getContents(File file) throws IOException {
    List<String> contents = new ArrayList<String>();

Inverting the if-else branches

All your methods have this pattern:

if (isValidFile(file)) {
    // ... (the main code)
} else {
    return false;
}

In cases, the main code is relatively long. As such, when you look at the else block, it's not visible what it is "the else of", I have to scroll up to see. In such situations, it would make the code more readable to invert the logic in the if-else: bring the much shorter block up, like this:

if (!isValidFile(file)) {
    return false;
} else {
    // ...
}

In fact, since in this case the block is a return statement, you can even eliminate the else block, reducing the nesting level, which is usually easier to read:

if (!isValidFile(file)) {
    return false;
}
// ...


Design/Architecture for MVC WinForms Dialog Control/Use All I have built an MVC WinForms application where Views are blind and deaf of their controllers. To facilitate this I have used Ninject as my Dependency Injection (DI) Inversion of Control (IoC) container. I have a question regarding my design/architecture of the way I handle showing dialogs (remembering I want full testability for all components).

I have two "marker" interfaces for the controllers and views of dialogs. The controller interface is:

IDialogController.cs:

public interface IDialogController : IDisposable { }

Made disposable to enusure the view is cleaned up when we use using(saveDialogController){ ... }. The view interface is IDialogView.cs:

public interface IDialogView { }

I then have my ISaveDialogView.cs as:

public interface ISaveDialogView
{
    /// <summary>
    /// Load the items to be displayed in the ListView.
    /// </summary>
    /// <param name="items">The items to display.</param>
    void LoadItemList(IEnumerable<string> items);
}

and the actual view that uses this interface is SaveDialogView.cs:

public partial class SaveDialogView : Form, ISaveDialogView, IDialogView
{
    public SaveDialogView()
    {
        InitializeComponent();
    }

    private void buttonYes_Click(object sender, EventArgs e)
    {
        DialogResult = DialogResult.Yes;
        Close();
    }

    private void buttonNo_Click(object sender, EventArgs e)
    {
        DialogResult = DialogResult.No;
        Close();
    }

    private void buttonCancel_Click(object sender, EventArgs e)
    {
        DialogResult = DialogResult.Cancel;
        Close();
    }

    public void LoadItemList(IEnumerable<string> items)
    {
        listBoxItems.DataSource = items;
    }
}

and the SaveDialogController.cs is:

public class SaveDialogController : IDialogController
{
    private ISaveDialogView view = null;

    public SaveDialogController(ISaveDialogView view)
    {
        if (view == null)
            throw new ArgumentNullException("view");

        this.view = view;
        Log.Trace("SaveDialogController.Ctor(): Initialized successfully");
    }

    public DialogResult ShowDialog(IWin32Window owner, IEnumerable<string> items)
    {
        view.LoadItemList(items);
        using ((Form)view)
        {
            var result = ((Form)view).ShowDialog();
            Log.Trace($"SaveDialogController.ShowDialog(): " +
                "DialogResult.{result.ToString()} returned from view");
            return result;
        }
    }

    #region IDisposable Members.
    private bool disposed = false;

    protected virtual void Dispose(bool disposing)
    {
        if (!disposed)
        {
            if (disposing)
            {
                if (view != null)
                {
                    ((Form)view).Dispose();
                    view = null;
                    Log.Trace("SaveDialogController.Dispose(): View disposed");
                }
            }
            disposed = true;
        }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
    #endregion // IDisposable Members.
}

I can then call the dialog from another controller as follows:

using (SaveDialogController c = DialogControllerFactory.GetController<SaveDialogController>())
{
    DialogResult result = c.ShowDialog((Form)View, new List<string>() { "A", "B", "C" });
    // Do stuff...
}

My questions on this are as follows:

Is this design/architecture okay, could it be made better, if so, how?
Are there any problems with the way I am disposing the view/Form here?
Any other comments?

Thanks very much for your time.

I am not sure what exactly do you archive with that architecture.
In the example code above, the controller doesn't have any logic - so you can just drop it and use the dialog directly.

However, if that is just an example and you add more logic to the controller lets take a short look to the following method:

public DialogResult ShowDialog(IWin32Window owner, IEnumerable<string> items)
{
    view.LoadItemList(items);
    using ((Form)view)
    {
        var result = ((Form)view).ShowDialog();
        Log.Trace($"SaveDialogController.ShowDialog(): " +
            "DialogResult.{result.ToString()} returned from view");
        return result;
    }
}

If the mehtod ShowDialog is called, the view (instance variable) will be disposed (because: using ((Form)view)) . If the method is called again, it works on a disposed view. If the controller will be disposed later, it tries to dispose the view again. So you dont need the using here.
If you cast your abstraction against the concrete class (((Form)view)), the abstraction is valueless because you can not mock it in unit tests. To avoid casting, add all needed methods (in that case ShowDialog) to your abstraction.
The instance variable view should be readonly because it is initialized once in constructor.


Nesting for loops to find matches in different tables I'm not sure if I'm even approaching this correctly (still very much learning), but this is what I'm attempting to accomplish:
My script gets an array from a column (tab names of the sheet) and then calls another function to copy those tabs (this part works fine).  What I'm attempting to add is a separate list of properties that I want to assign to each copied sheet (visibility, color, etc.)  This secondary list is on another tab, so my thought was that within my first loop (that determines which tabs to copy), I would have a nested loop that would find the match to the tab name and then bring those variables in to be associated with the sheet name.
My secondary for loop works, but it seems counter intuitive or somewhat redundant.  It feels like a lot of wasted effort to loop through the "properties array" for every loop, but maybe it is necessary.
function getSheetArray(ui, ss, settingsSheet, updateID, latestVersion, versionRangeName, updateSheet, updateSheetsSheet, sheetsToUpdateRangeName, updateSheetsVals) {
  var updateSheetsLength = updateSheetsVals.filter(String).length;
  if (updateSheetsLength == null) {
    return
  }
  var sheetDataRange = updateSheetsSheet.getRange(2, 1, updateSheetsLength, 1);
  const sheetData = sheetDataRange.getValues();

  var updateSheet = SpreadsheetApp.openById(updateID);
  var tabSheet = updateSheet.getSheetByName("tabDefinitions");
  var tabDataRange = tabSheet.getRange(2, 1, tabSheet.getMaxRows() - 1, tabSheet.getMaxColumns());
  const tabData = tabDataRange.getValues();
  var tabProps = [];
  for (var i = 0; i < sheetData.length; i++) {
    var sheetName = sheetData[i];

    for (var j = 0; j < tabData.length; j++) {
      if (sheetName == tabData[j][0]) {
        var tabProps = [
          tabName = tabData[j][0],
          tabVisibility = tabData[j][3],
          tabUpdVisibility = tabData[j][4],
          tabClearData = tabData[j][5],
          tabFormat = tabData[j][6],
          tabPosition = tabData[j][7],
          tabColor = tabData[j][8]
        ];
      }
    }
    console.log("tabProps = ", tabProps);
    console.log("tabName = " + tabName);
  }
}

I don't know enough about the shape of testData to write great code, but you could preprocess and convert testData from an array to an object or Map of key-value pairs. By using the first element of each item in testData as the key, then you can convert your \$O(n)\$ inner "search" loop into a constant time \$O(1)\$ key lookup. This would reduce your overall complexity from something on the order of \$O(n^2)\$ to a nicer linear \$O(n)\$ algorithm.
Note: This assumes your data has unique values sitting at index 0 of each element in the main array. The difference being your code returns the first match, but the following technically return the last match as duplicate key overwrite existing value(s).
Something like:
var tabData = tabDataRange
  .getValues()
  .reduce((tabData, data) => ({ ...tabData, [data[0]]: data }), {});

Then your loop becomes
for (var i = 0; i < sheetData.length; i++) {
  var sheetName = sheetData[i];

  if (tabData[sheetName]) {
    var data = tabData[sheetName];
    var tabProps = [
      tabName = data[0],
      tabVisibility = data[3],
      tabUpdVisibility = data[4],
      tabClearData = data[5],
      tabFormat = data[6],
      tabPosition = data[7],
      tabColor = data[8]
    ];
    break; // <-- found data, break out of loop early
  }
  console.log("tabProps = ", tabProps);
  console.log("tabName = " + tabName);
}


Shorten a sorting function I have this function:

int sort(book *data, int books, char mode, char sortBy) {

  int i, j;
  book aux;
  aux.cat = malloc(sizeof *(aux.cat));

  if(sortBy == 'n') {
    for(i = 0; i < books; i++) {
      for(j = i; j < books; j++) {
        if(mode == 'a') {
          if(strcmp((data + i)->fname, (data + j)->fname) > 0) {
            aux = *(data + i);
            *(data + i) = *(data + j);
            *(data + j) = aux;
          }
        }
        else {
          if(strcmp((data + i)->fname, (data + j)->fname) < 0) {
            aux = *(data + i);
            *(data + i) = *(data + j);
            *(data + j) = aux;
          }
        }
      }
    }
  }
  else {
    for(i = 0; i < books; i++) {
      for(j = i; j < books; j++) {
        if(mode == 'a') {
          if(strcmp((data + i)->categ, "Fictiune") == 0) {
            if(((data + i)->cat)->price > ((data + j)->cat)->price) {
              aux = *(data + i);
              *(data + i) = *(data + j);
              *(data + j) = aux;
            }
          }
        }
        else {
          if(strcmp((data + i)->categ, "Fictiune") == 0) {
            if(((data + i)->cat)->price < ((data + j)->cat)->price) {
              aux = *(data + i);
              *(data + i) = *(data + j);
              *(data + j) = aux;
            }
          }
        }
      }
    }
  }

  free(aux.cat);

  return 0;
}

I need to sort the data array in two modes: ascending and descending, and by two criteria: name and price.

The question is: is there a way to write another, more compact function that will do the same thing?

And, what do you think about line 5 (aux.cat = malloc(sizeof *(aux.cat));)? Is it a right way to initialize the cat pointer?

These are the structures:

typedef struct {
  int price, edition;
  char favCharacter[SMAX];
} category;

typedef struct {
  char title[SMAX], fname[SMAX], lname[SMAX], categ[SMAX];
  category *cat;
} book;

The code for the two cases (sorting by name and sorting by price) have nearly nothing in common.  You may be better off writing two functions.  Even if you really like this interface, it might make sense to have the master function to call a sort-by-name function or a sort-by-price function to do the work.

I have no idea what aux.cat = malloc(sizeof *(aux.cat)) is for.  You never really store anything in the .cat member.  Furthermore, after all the swapping done in the sorting process, the aux.cat at the end of the function is not the same aux.cat that you started out with.  That's a memory leak.

Your sort algorithm is selection sort, which takes O(n2) time.  There are more efficient algorithms that work in O(n log n) time.

Your sort() function always returns 0.  You might as well make it a void function and not return any value.

For efficiency and brevity, you would be better off using sorting routines built into the C library.  All you have to do to use those routines is to define a comparator function, which tells the sorting function how to order any pair of books.

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

static int compare_books_by_name_asc(const void *a, const void *b) {
    const book *book_a = (const book *)a,
               *book_b = (const book *)b;
    return strcmp(book_a->fname, book_b->fname);
}

static int compare_books_by_name_desc(const void *a, const void *b) {
    return compare_books_by_name_asc(b, a);
}

static int compare_books_by_price_asc(const void *a, const void *b) {
    int price_a = ((const book *)a)->cat->price,
        price_b = ((const book *)b)->cat->price;
    return price_a < price_b ? -1 :
           price_a > price_b ? +1 : 0;
}

static int compare_books_by_price_desc(const void *a, const void *b) {
    return compare_books_by_price_asc(b, a);
}

void sort(book *books, int num_books, char mode, char sortBy) {
    int (*comparator)(const void *, const void *);
    if (sortBy == 'n') {   /* Sort by name */
        comparator = (mode == 'a') ? compare_books_by_name_asc
                                   : compare_books_by_name_desc;
    } else {               /* Sort by price */
        comparator = (mode == 'a') ? compare_books_by_price_asc
                                   : compare_books_by_price_desc;
    }
    qsort(books, num_books, sizeof(*books), comparator);
}


Game object structure for Minecraft server I'm trying to write a Minecraft server in Ruby as an educational project and I've hit the point now where I need to decide on how I am going to structure the in-game objects (entities). In my mind, an Entity is an in-game thing such as a block, or a player, or a skeleton.

The reason this is code review, and not game design, is because I am interested in what you think of the design from a general programming perspective, as this is an open source project and I want it to be as programmer friendly as possible.

When I started writing the current spike, I came up with a few methods I could use for entity structure.

Standard Object-Orientated Programming Inheritance

In this example, I would have used the standard OOP class hierarchy to design my entities. you would have a base class Entity which would be extended by Player, Sign, Skeleton, you get the idea. The upside of this is that it's simple and fast to come up with - until you start wanting to share functionality in two derivatives. Ruby does not have multiple inheritance, so it gets messy when I want an Enderdragon entity to share some functionality with a Sheep. It also limits functionality unless you do a lot of code duplication at runtime if you want to, say, make a Villager controllable by a Player.

Modules and Inheritance

This approach would use modules to share functionality and have it all defined at compile-time. We would have modules such as Positionable, Orirentable, Nameable and a base Entity class would define all of these at compile time in derivatives (for want of a better word - no compiling in Ruby). The advantage of this is that it's more 'clear' to other programmers. Player would have Positionable and RemoteControllable whereas a Sheep might have Positionable. These subclasses are all designed at compile time.

Modules and Meta-Programming

Finally, this one would utilize the Factory (or Builder) pattern to have a similar approach to Modules and Inheritance, except this time, it would define the mixins at runtime. Instances of Entity would be re-opened at runtime and have the appropriate modules mixed-in in their factory. This has the benefit of huge flexibility at the cost of maybe perhaps slightly violating POLA.

I'm currently siding on the side of meta-programming.  What do you think?

module Entity
  # The Id of the Entity
  attr_accessor :id

  def to_s
    "Entity <ID:#{id}>"
  end
end

module Positionable
  attr_accessor :x, :y, :z
end

module Orientable
  attr_accessor :yaw, :pitch
end

module Nameable
  attr_accessor :name
end

entity = Entity.new
entity.extend(Positionable)
entity.extend(Nameable)
entity.extend(Orientable)

entity.x # => 0f

I would prefer your second plan, with inheritence and mixin modules.  I'll restate their different uses, which I am sure you know:

Use inheritance to model relationships between objects (B is a A).
Use mixins to model behavior (C logs, D remembers its position).

Unless you have a need to dynamically add mixins, apply YAGNI and don't do it yet.  Wait until you have a compelling need for dynamic mixins, and use them only where needed.  Metaprogramming is a power tool, but it does make the code harder to follow.  Use it judiciously.


Display Tournament League from total Players I've created function that will display a league competition from the total team numbers.

For example if we have 4 teams it will display (a vs b) vs (c vs d)
and if we have 5 teams it will diplay ((a vs b) vs c) vs (d vs e) and so on. It will display the competition looks square or equal.

here is the complete code :

public class EqualPlayer {
    private int index=0;

    public static void main(String[] args){
        EqualPlayer equalPlayer = new EqualPlayer();
        System.out.println(equalPlayer.getEqualPlayerID(26));
    }

    private String getEqualPlayerID(int numberPlayers){
        int nPDevided = numberPlayers/2;
        int A = numberPlayers-nPDevided;
        int B = nPDevided;
        String result="";

        if(A>2 || B > 2)
            return "( "+getEqualPlayerID(A)+" )"+" vs "+ "( "+getEqualPlayerID(B)+" )";

        if(A==2)
            if(result.isEmpty()){
                result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
                index+=2;
            }
            else{
                result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
                index+=2;
            }
        else if(A==1)
            if(result.isEmpty()){
                result = ""+getPlayerId(index);
                index+=1;
            }
            else{
                result = result+" vs "+ getPlayerId(index);
                index+=1;
            }

        if(B==2)
            if(result.isEmpty()){
                result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
                index+=2;
            }
            else{
                result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
                index+=2;
            }
        else if(B ==1)
            if(result.isEmpty()){
                result = ""+getPlayerId(index);
                index+=1;
            }
            else{
                result = result+" vs "+ getPlayerId(index);
                index+=1;
            }

        return result;

    }

    private char getPlayerId(int index){
        char [] alphabet = "abcdefghijklmnopqrstuvwxyz".toCharArray();
        return  alphabet[index];
    }

}

Any better idea how to create this function without recursive?

And also the limits of the player is 26 which is the total alphabets, if I want to increase the total player I need to add additional char to getPlayerId method, I would like to know about what are you guys think about that?
Improvement, suggestion, are welcome.

You can clean up your code by looking for similarities and getting rid of the duplication by creating separate functions. I'll demonstrate.

Let's take a look at this bit:

    if(A==2)
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
            index+=2;
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
            index+=2;
        }
    else if(A==1)
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
            index+=1;
        }
        else{
            result = result+" vs "+ getPlayerId(index);
            index+=1;
        }

    if(B==2)
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
            index+=2;
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
            index+=2;
        }
    else if(B ==1)
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
            index+=1;
        }
        else{
            result = result+" vs "+ getPlayerId(index);
            index+=1;
        }

Step 1 - add braces.

    if(A==2){
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
            index+=2;
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
            index+=2;
        }
    } else if(A==1) {
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
            index+=1;
        }
        else{
            result = result+" vs "+ getPlayerId(index);
            index+=1;
        }
    }
    if(B==2){
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
            index+=2;
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
            index+=2;
        }
    } else if(B == 1){
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
            index+=1;
        }
        else{
            result = result+" vs "+ getPlayerId(index);
            index+=1;
        }
    }

Step 2 - identify sections of duplicate code and generalize them.

In your case, this is the index+=2 and index+=1 at the end of each case.

    if(A==2){
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
        }
        index+=2;
    } else if(A==1) {
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
        }
        else{
            result = result+" vs "+ getPlayerId(index);
        }
        index+=1;
    }
    if(B==2){
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
        }
        index+=2;
    } else if(B == 1){
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
        }
        else{
            result = result+" vs "+ getPlayerId(index);
        }
        index+=1;
    }

Next, further reduce the duplicate code. You can see that if B is 2, we'll add 2 to index. If B is 1, we'll add 1 to index. We can just add B to index. (We're reducing duplication of constants here)

    if(A==2){
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
        }
        index+=A;
    } else if(A==1) {
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
        }
        else{
            result = result+" vs "+ getPlayerId(index);
        }
        index+=A;
    }
    if(B==2){
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
        }
        index+=B;
    } else if(B == 1){
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
        }
        else{
            result = result+" vs "+ getPlayerId(index);
        }
        index+=B;
    }

Next, remove duplication by creating a separate function that allows you to create the "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )" part in one call.

private String printVersus(int startIndex){
    return "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
}

Then, use it.

    if(A==2){
        if(result.isEmpty()){
            result = printVersus(index);
        }
        else{
            result = result+" vs "+printVersus(index);
        }
        index+=A;
    } else if(A==1) {
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
        }
        else{
            result = result+" vs "+ getPlayerId(index);
        }
        index+=A;
    }
    if(B==2){
        if(result.isEmpty()){
            result = printVersus(index);
        }
        else{
            result =  result+" vs "+printVersus(index);
        }
        index+=B;
    } else if(B == 1){
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
        }
        else{
            result = result+" vs "+ getPlayerId(index);
        }
        index+=B;
    }

Next, once again identify that there is duplication in handling of string appending - you handle result.isEmpty() in multiple places. Create a separate function. I gave it a bit of a poor name, since I haven't actually checked your business logic yet - I'm just optimizing this based on patterns!

private String appendVersusString(String original, String toAdd){
    if(original.isEmpty()){
        return toAdd;
    }
    else{
        return original+" vs "+ toAdd;
    }
}

Use the new function.

    if(A==2){
        result = appendVersusString(result, printVersus(index));
        index+=A;
    } else if(A==1) {
        result = appendVersusString(result, ""+getPlayerId(index));
        index+=A;
    }
    if(B==2){
        result = appendVersusString(result, printVersus(index));
        index+=B;
    } else if(B == 1){
        result = appendVersusString(result, ""+getPlayerId(index));
        index+=B;
    }

Once again, spot duplication between A and B - they're doing the exact same at the end of the case. I am assuming that A and B cannot go into the negatives.

Since if(A>2 || B > 2) has an early return, A and B are (individually) either 2, 1, or 0.

Thus we can see that if A is 2, we add A. If A is 1, we add A. If A is 0, we don't add anything. Same goes for B.

So we can just do index+=A in all three cases.

    if(A==2){
        result = appendVersusString(result, printVersus(index));
    } else if(A==1) {
        result = appendVersusString(result, ""+getPlayerId(index));
    }
    index+=A;
    if(B==2){
        result = appendVersusString(result, printVersus(index));
    } else if(B == 1){
        result = appendVersusString(result, ""+getPlayerId(index));
    }
    index+=B;

There is still duplication in the handling of A and B when it comes to appendVersusString. I think they're "groups", so I'll name the function handleGroup:

private String handleGroup(String result, int group, int currentIndex){
    if(group==2){
        result = appendVersusString(result, printVersus(currentIndex));
    } else if(group == 1) {
        result = appendVersusString(result, ""+getPlayerId(currentIndex));
    }
    return result;
}

And use your new function.

    handleGroup(result, A, index);
    index+=A;
    handleGroup(result, B, index);
    index+=B;

Final result:

public class EqualPlayer {
    private int index=0;

    public static void main(String[] args){
        EqualPlayer equalPlayer = new EqualPlayer();
        System.out.println(equalPlayer.getEqualPlayerID(26));
    }

    private String getEqualPlayerID(int numberPlayers){
        int nPDevided = numberPlayers/2;
        int A = numberPlayers-nPDevided;
        int B = nPDevided;
        String result="";

        if(A>2 || B > 2)
            return "( "+getEqualPlayerID(A)+" )"+" vs "+ "( "+getEqualPlayerID(B)+" )";

        handleGroup(result, A, index);
        index+=A;
        handleGroup(result, B, index);
        index+=B;

        return result;
    }

    private String handleGroup(String result, int group, int currentIndex){
        if(group==2){
            result = appendVersusString(result, printVersus(currentIndex));
        } else if(group == 1) {
            result = appendVersusString(result, ""+getPlayerId(currentIndex));
        }
        return result;
    }

    private String appendVersusString(String original, String toAdd){
        if(original.isEmpty()){
            return toAdd;
        }
        else{
            return original+" vs "+ toAdd;
        }
    }

    private String printVersus(int startIndex){
        return "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
    }

    private char getPlayerId(int index){
        char [] alphabet = "abcdefghijklmnopqrstuvwxyz".toCharArray();
        return  alphabet[index];
    }

}

Lastly, you could consider using a StringBuilder to prevent needless copying of String contents. I also recommend putting spaces around operators to improve readability (if(A == 2) instead of if(A==2) and index += A instead of index+=A).


jQuery slider for WordPress I have developed a jQuery slider and I want to implement it in WordPress (it will be a Featured Posts slider).

This is my slider logic:

If there are 1 or 3 posts, show them but hide the prev and next buttons.
If there are more than 3 posts but less than or equal to 6, or more than 6, show navigation.
If there are more than 6 posts, show navigation.
After the animation is finished, hide the next button.

In my code, there are a lot of if else statement. Is there a better way to write my slider?

You can look at the Fiddle and try to delete divs from container (slider and its navigation will remain functional...)

jQuery:

$(document).ready(function () {
  //If less than four divs Hide next button
  if ($('.container div').length < 4) {
    $('.next').hide();
  }
  //Also hide prev button
  $('.prev').hide();
  i = 0; //Click counter...
  $('.next').click(function () {
    //Prevent animation to mess up...
    if ($('.container').is(':animated')) {
      return false;
    }
    //Animate next click...
    $('.container').animate({
      marginLeft: '-=300'
    }, 1000, function () {
      i++; //Increase Var by One...
      // Animation Callback If there are LESS than 6 divs
      //hide next button
      if ($('.container div').length <= 6) {
        $('.next').hide();
      }
      // Else if there are more than 6 divs
      //show next button
      else if ($('.container div').length >= 6) {
        $('.next').show();
      }
      //If first click exists show prev button
      if (i == 1) {
        $('.prev').show();
      }
      //If second click exists hide next button
      else if (i == 2) {
        $('.next').hide();
      }
    });
  });
  //on prev button click
  $('.prev').click(function () {
    //Prevent animation to mess up...
    if ($('.container').is(':animated')) {
      return false;
    }
    //Run animation...
    $('.container').animate({
      marginLeft: '+=300'
    }, 1000, function () {
      i--; //Decrase number of clicks
      //If on prev click counter===0 hide prev button
      //and show next button
      if (i == 0) {
        $('.prev').hide();
        $('.next').show();
      }
      //Else if click===1 or click===0
      //Show next button
      else if (i == 1 || i == 0) {
        $('.next').show();
      }
    });
  });
});

HTML:

//If you delete those guys below //Slider and navigation will still work!!

Next

Prev

Here's how I would have written the slider. For a small project like this, you probably don't need to use Prototypal Inheritance. However, if you wanted to scale or add new functionality it would be a walk in the park. As you dive into larger projects, taking time to plan how you're going to structure your code will greatly improve your ability to write the same. Keep in mind if any part of this is confusing or unclear, I'll be happy to walk you through it. I've included the code in this fiddle, but I'll do most of the explaining in the commented code below.
Now onto the fun stuff. You can place the following code in a separate .js file.
function Slider ( container, nav ) {
    //Get the elements passed in
    this.container = container;
    this.nav = nav;

    this.divs = this.container.find('div'); //Select the divs in the container
    this.divWidth = this.divs.width(); //Width of each div
    this.divsLen = this.divs.length; //Count how many divs there are

    this.current = 0; //Set the counter to the first div
};

//This function does all the animation
Slider.prototype.transition = function( speed ) {
    this.container.animate({
        //Set the margin to a negative, hence the "-" in front.
        //Get the current div, multiply by its width, and multiply by 3.
        //This makes the animation move three divs at a time.
        //If you changed the width in the CSS, this code would work just fine.
        'margin-left': -( this.current * this.divWidth * 3 )
    }, speed); //Here I get the speed from the click event set up in your html file.
};

//Here is where it might get tricky.
//This function controls the current set of divs being displayed.
Slider.prototype.setCurrent = function( dir ) {
    var pos = this.current; //Cache the current div

    //Okay. Take a breather right here :)
    //Get the current div and add to it depending on what is returned.
    //We test to see if the clicked button was 'next'.
    //If true, it returns '1' and adds that to 'pos'.
    //If false, that means you clicked 'prev'. It then returns a '-1' and adds to 'pos'.
    pos += ( ~~( dir === 'next' ) || -1 );

    //Now if we kept clicking 'prev', we would end up with a negative number after a while.
    //This tests to see if 'pos < 0'.
    //If yes, take the total divs, divide by 3, and subtract one.
    //This will make it display the last three divs. It kinda goes 'backwards'.
    //If pos isn't < 0, then take modulus of pos and the total / 3
    //Modulus is a math operator and if you're not clear on it, there's a link in the bottom.
    this.current = ( pos < 0 ) ? (this.divsLen / 3) - 1 : pos % (this.divsLen / 3);

    return pos;
};

This part comes after your script in the previous .js file. It can be in your html page at the bottom. I set up the previous piece of code so that we can refrain from referencing any DOM elements or specific values, keeping the logic separate. This part now sets all the configurations we and values to be sent to the slider.
//First off, if your user doesn't have JS on, your plugin will fail.
//I've set the wrapper overflow to scroll in the CSS, so if there is no JS, the
//user can still scroll and see all the divs.
//Here we set the overflow to hidden and select our container element.
var container = $('.wrapper').css('overflow', 'hidden').children('.container');

//Set up the Slider constructor function, and pass in the elements we want.
var slider = new Slider( container, $(".nav_wrapper") );

//Again, if there's no JS, there's no point in having buttons.
//The buttons are hidden in the CSS, and here we display then with .show().
//Then we select and set up the click event on the actual div button.
slider.nav.show().find('div').on('click', function() {
    //When clicked, we call the functions from our previous code, passing in our configurations.
    //By doing this, if we want to change anything, it can be done here and only done once.
    //Calls setCurrent and passes the clicked div class. It's either 'prev' or 'next'.
    slider.setCurrent( $(this).attr('class') );

    //After the current div is all figured out, we just animate.
    //Call transition and pass in the speed we want.
    slider.transition( 1000 );
});

Hopefully this all made sense to you. If you're a beginner in JS, this might be a little over your head, but that's perfectly ok! Try reading as many articles as you can find on the subjects here and you'll catch on soon.
Article explaining modulus: http://mathforum.org/library/drmath/view/54363.html


"Guess a random letter" game with command line interface The code below if launched on the command line is a simple guessing game.
A random character 'a', 'b' or 'c' is chosen, and the user is prompted until they guess correctly.
It's arguable that invalid input and a win is not necessarily exceptional.  Should exceptions be used in this manner?
I am not sure what's a sensible way to check for a winning situation and to terminate the game.
<?php

class ABorCException extends Exception {
    const WIN     = 'Win';
    const INVALID = 'Invalid';
}

class ABorC
{
    const CHOICES = ['a', 'b', 'c'];
    public $answer;
    public $turns;

    public function __construct($answer = null)
    {
        $this->answer = $answer ?? self::CHOICES[array_rand(self::CHOICES)];
    }

    public function turn($guess)
    {
        if(!in_array($guess, self::CHOICES))
            throw new ABorCException(ABorCException::INVALID);
        $this->turns[] = $guess;
        if($guess === $this->answer)
            throw new ABorCException(ABorCException::WIN);
    }
}

$game = new ABorC;

while (true) {
    try {
        $input = readline("Enter your guess a, b or c?\n");
        $game->turn($input);
        echo "Wrong!\n", "Guess again.\n";
    } catch (ABorCException $e) {
        if($e->getMessage()===ABorCException::INVALID) {
            echo "Invalid input.\n";
        }
        if($e->getMessage()===ABorCException::WIN) {
            echo "Correct!\n", "You won in ", count($game->turns), " valid attempts.\n";
            exit;
        }
    }
}

Sample game:
$ php aborc.php
Enter your guess a, b or c?a
Wrong!
Guess again.
Enter your guess a, b or c?z
Invalid input.
Enter your guess a, b or c?b
Correct!
You won in 2 valid attempts.

You are using exceptions for flow control, and that is considered an anti-pattern.
Exceptions are meant for unhandlable conditions that would otherwise corrupt your application. Winning the game should not corrupt the application and cause undefined behavior. If it does, you are likely building a stock trading application (I'm only kidding ;-) of course).
Invalid input could certainly corrupt things or cause undefined behavior, but in this case your program should expect invalid input and handle it gracefully. Always expect the unexpected with user input.
When reading input, check that it is correct. Loop until you get correct input.
The $game-turn() method could return a "result" object, which reports back whether the game has been won, and if so who the winner is (and possibly a score). Again, handle this with a simple if statement:
$result = $game->turn($input);

if ($result->isWinningTurn()) {
    echo "Winner! {$result->winner}";
}
else {
    echo "next move";
}

If $game->turn() throws an exception, I would expect it to be an unhandled condition or something unrecoverable like an out of memory exception. As a programmer, I would be VERY surprised if winning the game crashed the program.
Now, the turn() method should throw an exception if you give it bad input. That makes sense. But you need to check for valid input before calling $game->turn($input). You likely need an isValidInput method on the game object in order to check this.
$game-turn("d"); // I expect this to throw

if ($game->isValidInput($input)) {
    $result = $game->turn($input)); // I expect this to always succeed

    if ($result.isWinningTurn()) {
        ...
    }
    else {
        ...
    }
}
else {
    echo "Bad input, try again."
}


On click, doubleclick and mouse moving I have an application using tkinter where different actions are required on mouse click, double click and when the mouse is being moved. The following class works however I wonder if there is no easier- or tkinter builtin way of doing this?

from tkinter import Tk, Canvas

class MouseControl:
    '''  Class for mouse control to establish if there is a 'click',
         'double click' or mouse is being moved '''
    def __init__(self, aw):
        self.double_click_flag = False
        self.button_released_flag = False
        self.aw = aw
        self.aw.bind('<Button-1>', self.clicked)  # bind left mouse click
        self.aw.bind('<Double-1>', self.double_click)  # bind double left clicks
        self.aw.bind('<ButtonRelease-1>', self.button_released)  # bind button release
        self.aw.bind('<B1-Motion>', self.moved)  # bring when mouse is moved

    def clicked(self, event):
        '''  add a little delay before calling action to allow for double click
         and button released to have occurred '''
        self.double_click_flag, self.button_released_flag = False, False
        self.aw.after(300, self.action, event)

    def double_click(self, event):
        '''  set flag when there is a double click '''
        self.double_click_flag = True

    def button_released(self, event):
        '''  set flag when button is released '''
        self.button_released_flag = True

    def moved(self, event):
        '''  define action on when mouse is moved in this case just printing
             the coordinates'''
        print('mouse position is at ({:03}. {:03})'.
              format(event.x, event.y), end='\r')

    def action(self, event):
        '''  define action on click and double click in this case just printing
             the event '''
        if self.button_released_flag:

            if self.double_click_flag:
                print('double mouse click event')

            else:
                print('single mouse click event')

root = Tk()
window = Canvas(root, width=400, height=400, bg='grey')
mouse = MouseControl(window)
window.place(x=0, y=0)
window.mainloop()

Avoid wildcard imports

You should use import tkinter as tk instead of from tkinter import Tk, Canvas. This is a PEP8 recommendation where you can find the reason of this. Of course, in this case, you have to prefix your widgets with tk. For instance: my_button = tk.Button(...)

Remove useless comments

To be honest, most of your comments (docstrings, to be exact) can be removed. Let us take an example:

def double_click(self, event):
    '''  set flag when there is a double click '''
    self.double_click_flag = True

The docstring is just noisy. It does not bring any additional information when I read self.double_click_flag = True. That comment just repeats what the instruction already tells me. Avoid repeating yourself, even through comments.

The same is true when it comes to inline comments. Example:

self.aw.bind('<Button-1>', self.clicked)  # bind left mouse click

Use the main guard

Sometimes we want to run some code only if the program was used by itself and not when it was imported from another module, for this reason it is good to use if __name__ == "__main__":

def main():
    root = tk.Tk()
    window = tk.Canvas(root, width=400, height=400, bg='grey')
    mouse = MouseControl(window)
    window.place(x=0, y=0)
    window.mainloop()

if __name__ == "__main__":
    main()

Simplify the handlers

I find your handlers having unnecessary code. Simply take advantage of the button events themselves and get rid of all those unnecessary flags which are rather cumbersome in case you want to add more code in the future (because they play the role of global variables, which thing we do not like in programming).

Taking in consideration what has been said so far, here is your code cleaned:

import tkinter as tk

class MouseControl:
    def __init__(self, canvas):
        self.canvas = canvas
        self.canvas.bind('<Button-1>', self.clicked)
        self.canvas.bind('<Double-1>', self.double_click)
        self.canvas.bind('<ButtonRelease-1>', self.button_released)
        self.canvas.bind('<B1-Motion>', self.moved)

    def clicked(self, event):
        print('single mouse click event at ({}, {})'.format(event.x, event.y))

    def double_click(self, event):
        print('double mouse click event')

    def button_released(self, event):
        print('button released')

    def moved(self, event):
        print('mouse position is at ({:03}. {:03})'.format(event.x, event.y), end='\r')

def main():
    root = tk.Tk()
    window = tk.Canvas(root, width=400, height=400, bg='grey')
    mouse = MouseControl(window)
    window.place(x=0, y=0)
    window.mainloop()

if __name__ == "__main__":
    main()


Importing and deleting product data in WordPress I am interested in how to handle errors/exceptions/returned data correctly.

For example, I have a script which creates, updates and deletes products data.

Here is method that receives data from an API, processes and decides what to create/update/delete.

private function importProducts($brand)
{
    $products = $this->dsApi->getProductsByBrand($brand);

    //returns ['create' => [], 'update' => [], 'delete' => [] ]
    $preparedProducts = $this->prepareProducts($products, $brand);

    foreach ($preparedProducts['delete'] as $key => $product) {
        $this->deleteProduct($product);
        $this->log->info(
            'Brand ' . $brand->name . '(' . $brand->id_brand . ')'
            . ' id: ' . $product . ' was deleted'
        );
    }

    foreach ($preparedProducts['create'] as $key => $product) {

        $this->importProduct($product, $brand);
        $this->log->info(
            'Brand ' . $brand->name . '(' . $brand->id_brand . ') sku: ' . $product['sku'] . ' was imported'
        );
    }

    foreach ($preparedProducts['update'] as $key => $product) {
        $this->importProduct($product, $brand, $product['idProductWc']);
        $this->log->info(
            'Brand ' . $brand->name . '(' . $brand->id_brand . ') sku: ' . $product['sku']
            . ' id: ' . $product['idProductWc'] . ' was updated'
        );
    }
}

And for example deleteProduct() method:

private function deleteProduct($product)
{
    wp_delete_post($product, true);

    $args = [
        'order' => 'ASC',
        'post_mime_type' => 'image',
        'post_parent' => (int) $product['idProductWc'],
        'post_status' => 'inherit',
        'post_type' => 'attachment',
    ];
    $child = get_children($args);
    foreach ($child as $id => $item) {
        wp_delete_attachment($id, true);
    }
}

Is the deletion method written correctly? Is it necessary to handle an error in wp_delete_post() method? How to handle it? What to write in logs in this case? Should the method return a response?

Looking at your importProducts() method, firstly ask if this needs to be public, private or protected see PHP visibility

Now the importProducts() method in question, I would separate and have three methods as follows:

/**
 * @param $brand string
 * @param $productsToDelete array
 */
protected function _deletePreparedProducts($brand = null, $productsToDelete = array()) {
    if(empty($brand)) {
        /* Do we need a brand? If not throw an Exception or log to your debug logger */
    }
    /* deletion logic here */
}

/**
 * @param $brand string
 * @param $productsToCreate array
 */
protected function _createPreparedProducts($brand = null, $productsToCreate = array()) { /* create logic here */ }

/**
 * @param $brand string
 * @param $productsToUpdate array
 */
protected function _updatePreparedProducts($brand = null, $productsToUpdate = array()) { /* prepare logic here */ }

I would therefore change the importProducts() method as follows (assuming that you still want this as a private method):

<?php
private function importProducts($brand)
{
    $products = $this->dsApi->getProductsByBrand($brand);

    $preparedProducts = $this->prepareProducts($products, $brand);

    if(!empty($preparedProducts['delete'])) {
        $this->_deletePreparedProducts($brand, $preparedProducts['delete']);
    }

    if(!empty($preparedProducts['create'])) {
        $this->_createPreparedProducts($brand, $preparedProducts['create']);
    }

    if(!empty($preparedProducts['update'])) {
        $this->_updatePreparedProducts($brand, $preparedProducts['update']);
    }
}

Using this approach, if you need to change the logic to update a product, you will only have to worry about your update method, rather than having lots of logic in one method. And in your main method you will be checking if there are any products to delete, update or create first rather than assuming that there is, as is the case in your original script.

If there are no products to import then why was this method called? This is where you could throw an exception, or log to a debug file, like this:

    if(empty($preparedProducts['delete'])
        && empty($preparedProducts['create'])
        && empty($preparedProducts['update'])) {
        throw new \Exception("Import products called without any products to import");
    }

I might also consider preparing the data first before calling the importProducts() method, rather than preparing that data within the method - this will depend on your dataflow.

I would finally suggest that as you are updating and deleting products, the method name may be incorrect. manageProducts() or something similar might be a better name for this method.


Reformatting a structured string like "a-b-c|A-B-C" into "a-A|b-B|c-C" The code below was inspired by this post in Code Review.
Here is how it was first intended by its author:

I have the following code that converts a string that looks like :

aaa-bbb|ccc-ddd|eee-fff

to two strings that look like

aaa|ccc|eeeandbbb|ddd|fff

Somebody already proposed an answer, and I found it could be more improved using reduce() and map() methods.
But after a few time I realized it could be made of more general use, so I hooked in the intent to have a code which both:

accept any main- and sub-separator (| and -) in the above example.
accept any number of main parts and sub-parts, the only (not checked) requirement being the number of sub-parts is the same through all parts.
is strictly written functional-programming style

Then I ended up with pretty different requirements than in the original post I cited, so rather than an answer I prefer to post it as a question, waiting for comments about:

is there a simpler strategy for doing that in functional style?
and even is it a good idea to use functional style, since it might be actually slower than with procedural code?

function reformat(string, mainSep, subSep) {
  return string.split(mainSep).map(item => item.split(subSep)).reduce(
    (result, part) =>
      part.map((str, index) =>
        `${result[index]}${mainSep}${str}`
      )
  ).join(subSep || ' ');
}

console.log(reformat('HEL-CAS|MAD-STO|XXX-YYY', '|', '-'));
console.log(reformat('1:2:3_one:two:free_ONE:TWO:THREE_I:II:III', '_', ':'));
console.log(reformat('abcdefgh.ABCDEFGH', '.', ''));

To start, I must say that I wish I was writing JS like this in 2016. I have only recently been becoming acquainted with the newer features ecmascript-6 has to offer. After looking at the other answers to the post you mentioned I would have suggested they consider using destructuring assignment, but I doubt it would help for your code.
For readability it can be a great idea to use functional code, but yes performance can suffer because each iteration calls a function, which adds to the call stack.
This code makes great use of template literals and arrow functions. It also has consistent indentation.
The template literal may be considered excessive, since it only contains interpolated values. The string concatenation could be achieved with the + operator - instead of:
 part.map((str, index) =>
       `${result[index]}${mainSep}${str}`

It could merely be:
 part.map((str, index) =>
       result[index] + mainSep + str

Which would save a few characters but also should be faster. See this jsBin for a comparison, as well as this answer to Most efficient way to concatenate strings in JavaScript?  which has a link to an updated JSPerf. The graphs don't appear to be loading in my browser but in MacOS FF 77 the concat appears to be fastest with the template literal 4% slower. In Chrome 83.04 the results were similar - template literals appeared to be 22% slower than appending.
I forgot that reduce() started with the 2nd element when no initialValue is supplied. While it may not be a huge improvement it does mean one less iteration...


Generating combinations of specific values of a nested dict I am developing a framework that allows to specify a machine learning model via a yaml file with different parameters nested so the configuration files are easy to read for humans.

I would like to give users the option of instead of specifying a parameter giving a range of options to try via a list.

Then I have to take this and generate all the possible valid combinations for the parameters the user has given multiple values for.

To mark which parameters are in fact lists and which ones are multiple values, I have opted to choose that combination values begin with 'multi_' (though if you have a different take I would be interested to hear it!).

So for example an user could write:

config = {
    'train_config': {'param1': 1, 'param2': [1,2,3], 'multi_param3':[2,3,4]},
    'model_config': {'cnn_layers': [{'units':3},{'units':4}], 'multi_param4': [[1,2], [3,4]]}
}

Indicating that 6 configuration files must be generated, where the values of 'param3' and 'param4' take all the possible combinations.

I have written a generator function to do this:

from pandas.io.json.normalize import nested_to_record
import itertools
import operator
from functools import reduce

from collections import MutableMapping
from contextlib import suppress

def generate_multi_conf(config):
    flat = nested_to_record(config)
    flat = { tuple(key.split('.')): value for key, value in flat.items()}
    multi_config_flat = { key[:-1] + (key[-1][6:],) : value for key, value in flat.items() if key[-1][:5]=='multi'}
    if len(multi_config_flat) == 0: return # if there are no multi params this generator is empty
    keys, values = zip(*multi_config_flat.items())

    # delete the multi_params
    # taken from https://stackoverflow.com/a/49723101/4841832
    def delete_keys_from_dict(dictionary, keys):
        for key in keys:
            with suppress(KeyError):
                del dictionary[key]
        for value in dictionary.values():
            if isinstance(value, MutableMapping):
                delete_keys_from_dict(value, keys)
    to_delete = ['multi_' + key[-1] for key, _ in multi_config_flat.items()]
    delete_keys_from_dict(config, to_delete)

    for values in itertools.product(*values):
        experiment = dict(zip(keys, values))
        for setting, value in experiment.items():
            reduce(operator.getitem, setting[:-1], config)[setting[-1]] = value
        yield config

Iterating over this with the example above gives:

{'train_config': {'param1': 1, 'param2': [1, 2, 3], 'param3': 2}, 'model_config': {'cnn_layers': [{'units': 3}, {'units': 4}], 'param4': [1, 2]}}
{'train_config': {'param1': 1, 'param2': [1, 2, 3], 'param3': 2}, 'model_config': {'cnn_layers': [{'units': 3}, {'units': 4}], 'param4': [3, 4]}}
{'train_config': {'param1': 1, 'param2': [1, 2, 3], 'param3': 3}, 'model_config': {'cnn_layers': [{'units': 3}, {'units': 4}], 'param4': [1, 2]}}
{'train_config': {'param1': 1, 'param2': [1, 2, 3], 'param3': 3}, 'model_config': {'cnn_layers': [{'units': 3}, {'units': 4}], 'param4': [3, 4]}}
{'train_config': {'param1': 1, 'param2': [1, 2, 3], 'param3': 4}, 'model_config': {'cnn_layers': [{'units': 3}, {'units': 4}], 'param4': [1, 2]}}
{'train_config': {'param1': 1, 'param2': [1, 2, 3], 'param3': 4}, 'model_config': {'cnn_layers': [{'units': 3}, {'units': 4}], 'param4': [3, 4]}}

Which is the result expected.

Any feedback on how to make this code more readable would be very much appreciated!

For non-trivial list comprehensions such as

multi_config_flat = { key[:-1] + (key[-1][6:],) : value for key, value in flat.items() if key[-1][:5]=='multi'}

You should split it onto multiple lines, i.e.

multi_config_flat = {key[:-1] + (key[-1][6:],): value
                     for key, value in flat.items()
                     if key[-1][:5]=='multi'}

This:

key[-1][:5]=='multi'

should be

key[-1].startswith('multi')

This:

if len(multi_config_flat) == 0: return

is equivalent (more or less) to

if not multi_config_flat:
    return

The latter also catches the case of multi_config_flat being None, but that won't be possible in this context.

This:

for key, _ in multi_config_flat.items():

is not necessary; simply iterate over keys:

for key in multi_config_flat:

This is fairly opaque:

reduce(operator.getitem, setting[:-1], config)[setting[-1]] = value

Probably you should assign the output of reduce to a meaningfully named variable, so that your code is more clear.


Check if strings from a list in a text and return first longest match I'm trying to design an application that binds a phrase or word to some item, for example image, and saves phrase-item pair in the database. Then it receives a text, and if it contains binded substring, corresponding item should be returned.
It should return only one item (first match), and longest substrings should take precedence.

I wrote a function, that returns expected values:

from operator import itemgetter

def get_item(text, bindings):
    text = text.lower()
    matches = []
    for phrase, item in bindings:
        phrase = phrase.lower()
        index = text.find(phrase)
        if index != -1:
            matches.append((phrase, item, index))
    if matches:
        matches.sort(key=lambda x: len(x[0]), reverse=True)
        matches.sort(key=itemgetter(2))
        item_id = matches[0][1]
    else:
        item_id = None
    return item_id

Example:

bindings = [
('i', 'item1'), ('like', 'item2'), ('i like', 'item3'), ('turtles', 'item4'),
]
text = 'I like turtles!'
print(get_item(text, bindings)) # should return item3

Is there cleaner ways to complete such task, or faster, perhaps?

You can use an early return in your function to get rid of a few lines, because python's default return value is already None:

from operator import itemgetter

def get_item(text, bindings):
    text = text.lower()
    matches = []
    for phrase, item in bindings:
        index = text.find(phrase)
        if index != -1:
            matches.append((phrase, item, index))
    if matches:
        matches.sort(key=lambda x: len(x[0]), reverse=True)
        matches.sort(key=itemgetter(2))
        return matches[0][1]

Algorithm wise, I'm not sure this code is up to the specification you wrote. You said, phrases should be preferred over single words. However, you only sort by length. Therefore

bindings = [('i am a', 'item1'), ('terminator', 'item2')]
text = 'I am a terminator'
print get_item(text, bindings)

will print 'item2' and not 'item1', even though it is a single word and the other binding is a phrase. Maybe also use something like len(x[0].split()) to give you the number of words as the sort key?

Also, your code scales with something like \$\mathcal{O}(m)*\mathcal{O}(n*k)\$, where \$m\$ is the number of defined bindings, \$n\$ is the length of the text and \$k\$ is the average length of the phrases in the bindings. I'm not sure whether there is a better algorithm, though.


Global data singleton I need a global data in my application. My application uses several threads to access and add items to my global variable. The global variable is a ConcurrentDictionary I choose to ensure data is added to the variable. What do you think about my approach?

public class GlobalData
{
    private static volatile GlobalData instance;
    private volatile ConcurrentDictionary<string, object> _data = new ConcurrentDictionary<string, object>();
    private static object syncRoot = new Object();

    public static GlobalData Instance
    {
        get
        {
            if (instance == null)
            {
                lock (syncRoot)
                {
                    if (instance == null)
                        instance = new GlobalData();
                }
            }

            return instance;
        }
    }

    public void Add(string id, object obj)
    {
        _data.TryAdd(id, obj);
    }

    public object Get(string id)
    {
        object value;
        _data.TryGetValue(id, out value);
        return value;
    }
}

Singletons are evil
Even if you do want to use it, use Lazy<T> instead of manually doing double-checked locking

Best solution would be to leverage IoC container to inject dependencies you need.


Which loop to use for multiple if statements which serialize objects I have figured out how to serialize multiple objects, however I am utterly convinced I can use some kind of loop for the if statements below, to avoid duplication. Please can someone advise how I would do this?

I have 20 objects in total, this code snippet shows the "if statements" for 4 of them.

import java.util.InputMismatchException;
import java.util.Scanner;
import java.io.*;
import java.util.Arrays;

public class Serializer extends WriteClub
{
   public void Serialize()
   {

    MainMenu pass_choice = new MainMenu();
    int passed_choice = pass_choice.chooseTeam();

    if(passed_choice==1){
    ClubInfo club = new ClubInfo("Arsneal" , "Emirates" , "Premier League" , 11122333 , 60000);
    tryWriteClub(club);
    } // end if 1

    if(passed_choice==2){
    ClubInfo club1 = new ClubInfo("Aston Villa" , "Villa Park" , "Premier League" , 111223334 , 40000);
    tryWriteClub(club1);
    } // end if 2

    if(passed_choice==3){
    ClubInfo club2 = new ClubInfo("Bournemouth" , "Dean Court" , "Premier League" , 111223335 , 20000);
    tryWriteClub(club2);
    } // end if 3

    if(passed_choice==4){
    ClubInfo club3 = new ClubInfo("Chelsea" , "Stamford Bridge" , "Premier League" , 111223336 , 50000);
    tryWriteClub(club3);
    } // end if 2
   }
}

The solution I propose to you is to create an array or another ordered structure where you could create all the 20 club objects and just select the right one base on the index of the club.

ClubInfo [] clubs =  new ClubInfo[20];
clubs[0] = new ClubInfo("Arsneal" , "Emirates" , "Premier League" , 11122333 , 60000);

Then you can do :

ClubInfo club = clubs[passed_choice - 1];
tryWriteClub(club);

You could find a better way to deal with the index problem. Small problem is now the ClubInfo objects will either be created everytime the method is called or you could create it one time as a static array or something similar.

Naming

You do not follow the naming standard for the Java language. Method name should be nameLikeThis() and variable should be nameLikeThisToo. So your  int passed_choice should be int passedChoice and the method public void Serialize() should be public void serialize().

Comments

I recommend that you get rid of comments like // end if 2. It is just noise that serve nothing. Comments should be use to explain why you have this particular piece of code.


List of tuples generated through list comprehension The function returns a list of tuples which is generated through list comprehension:

getUserRatings :: String -> [Film] -> [(String,Int)]
getUserRatings search database = [(title,rate) | (Film title _ _ ratings) <- database, (user,rate) <- ratings, search == user]

Film is defined as:

type Rating = (String, Int)
data Film = Film {  title,director :: String,
                    year :: Int,
                    ratings :: [Rating] } deriving (Read,Show)

such that a database is:

testDatabase :: [Film]
testDatabase = [(Film "Blade Runner" "Ridley Scott" 1982 [("Amy",5),("Bill",8),("Ian",7),("Kevin",9),("Emma",4),("Sam",7),("Megan",4)])]

The function getUserRatings takes in a the users name and the database, searches the ratings in the database, then compiles the the tuple so that its in the format of:

  film name (String), user rating (Int)

How might I write this more efficiently using higher order functions? I've attempted a few times but to no success using filters and maps; I don't know whether to post those attempts here or not. So I won't until advised otherwise.

Using high-order functions is not much readable:

userRatings search = concatMap (\film ->
  map ((title film,) . snd)
    $ filter ((==search) . fst) $ ratings film)

You need {-# LANGUAGE TupleSections #-} to be able to write (title film,).

Another useful extension is NamedFieldPuns.

getUserRatings search database
  = [ (title,rate)
    | Film{title, ratings} <- database
    , (user,rate) <- ratings
    , search == user
    ]

Writing Film{title, ratings} instead of Film title _ _ ratings is not only a bit shorter, it also allows you to change/reorder fields in Film without updating getUserRatings.

Btw, in my opinion, splitting getUserRatings in several lines adds more readability than rewriting it somehow.


Converting events in an agenda to textual format Here is a serializer. My concern is to make the convert_to_internal function more pythonic, efficient and ideally more functional. Maybe I could also get rid of three nested loops.

class TimeSerializer(serializers.Field):
    DAYS = {
        0: 'monday',
        1: 'tuesday',
        2: 'wednesday',
        3: 'thursday',
        4: 'friday',
        5: 'saturday',
        6: 'sunday',
        7: 'holidays'
    }
    INVERSE_DAY = {v: k for k, v in DAYS.items()}

    def __day_comparator(day_1, day_2):
       """
       A helper to sort days, so they're sorted
       as they appear in a week calendar.
       """
       inverse_dict = TimeSerializer.INVERSE_DAY
       return inverse_dict[day_1[0]] - inverse_dict[day_2[0]]

    def convert_to_internal(self, agenda):
        x = []
        t = OrderedDict(sorted(agenda['timconf'].items(),
                               key=TimeSerializer.__day_comparator))
        for day, items in t.items():
            day_num = TimeSerializer.INVERSE_DAY[day]
            for item in sorted(items, key=lambda k: k['begin']):
                for attr in ['begin', 'end']:
                    if hasattr('item', attr):
                        item[attr] = item[attr] + day_num*24*60*60
                x.append(TimeItem(**item))
        p = ';'.join(map(str, x))
        return "-t {} -n {} -p '{}'".format(agenda['timeZone'], agenda['locale'], p)

This is very hard to read. What's an internal, why are we converting to it? What's an agenda? This is clearly a class method so I'm missing some context but it's not a self explanatory function. You could improve this with proper naming. x and t are meaningless. For instance, if x was appointment_times it would tell me much more.

Likewise, your code is not broken up at all. It's one solid block, which makes it harder to see what each individual section does or even is. Adding whitespace will make it much clearer immediately:

def convert_to_internal(self, agenda):
    x = []
    t = OrderedDict(sorted(agenda['timconf'].items(),
                           key=TimeSerializer.__day_comparator))

   for day, items in t.items():
        day_num = TimeSerializer.DAYS_INVERSE[day]

        for item in sorted(items, key=lambda k: k['begin']):
            for attr in ['begin', 'end']:
                if hasattr('item', attr):
                    item[attr] = item[attr] + day_num*24*60*60

            x.append(TimeItem(**item))

    p = ';'.join(map(str, x))
    return "-t {} -n {} -p '{}'".format(agenda['timeZone'], agenda['locale'], p)


Computing hash of string I want my code to be reviewed and I'd like to hear your opinions.

Sun's code to compute hash of string:

public int hashCode() {
    int h = hash;
  if (h == 0 && value.length > 0) {
      char val[] = value;
      for (int i = 0; i < value.length; i++) {
          h = 31 * h + val[i];
      }
      hash = h;
  }
    return h;
}

My code:

public int hashCode2() {
    if (hash == 0) {
        for (int i = 0; i < value.length; i++) {
            hash = 31 * hash + value[i];
        }
    }
    return hash;
}

Why I consider my code an improved version:

There is no need to create variables h and val as done in Sun's method.
The premature check of value.length > 0 is not good enough to add this complexity.

I do not understand why val was introduced in addition to value – it isn't even a copy of the contents.

On the other hand h does make sense once you consider thread safety. Assume we have two threads which calculate the hash of the string at nearly the same time – to trigger this, have both threads sleep for a moment when they enter the if branch. At the beginning, both threads will have hash == 0, so they will calculate the hash anew. With the original implementation, only a local variable is mutated, and then atomicaly committed to the hash variable. With your code, both threads mutate the same hash directly and will thus compute an invalid result. Your solution would be OK iff all accesses to the hash were synchronized (by locking the whole string object for each such method which is silly when using immutable strings).

As this is Code Review, I will also suggest to return early in both implementations, instead of putting the whole calculation into an if branch. It is also nicer to use a foreach loop than to use indices.

public int hashCode() {
    int h = hash; // copy for thread safety
    if (h != 0 || value.length == 0) return h;

    for (char c : value) {
       h = 31 * h + c;
    }

    hash = h; // commit the change
    return h;
}

On the other hand, a foreach-loop might not be as optimized as looping over the indices, and an early return could also impact what code is generated.

Edit: The value.length > 0 test in the original avoids committing the old value when no change of the hash is required (for the empty string). This can be relevant in cache-sensitive applications (remember that writing to shared memory can invalidate caches for other processors as well – we want to avoid this if possible. The condition also provides the guarantee that the loop will execute at least once if that branch is taken. Code ought to be generated in a way that this prior check does not reduce performance – on the contrary.


String Format object extension I wrote an extension to create a string-representation by an object:

public static string ToString<T>(this T @this, Func<T, string> predicate) where T: class
{
    return predicate(@this);
}

So I can do something like

return handler.GetUserById(id)?.ToString(x => $"{x.LastName}, {x.FirstName}");

which gives me either null if the user is null or otherwise e.g. "Miller, Peter"

My question: is this a good extension? Makes it sense? Anything I didn't consider?
And what about the null-thing? Should I check the null value in the extension? Imho, the thrown NullReferenceException is okay, if s.o. calls this with a null object.

There are not much to review, so to answer your questions:

It's a good extension if you can see it useful in more than one place.
It makes sense.
I can't see anything else to consider - a null check on the predicate maybe?
If the exception thrown by the predicate when @this is null is good enough for you, don't bother further.

I don't see why the restriction where T: class is necessary

If you always want to generate a string containing a list of properties (or another predefined format), you could change the signature to:

public static string ToString<T>(this T @this, params Func<T, object>[] getters)
{
  return string.Join(", ", getters.Select(g => g(@this)));
}

called as:

handler.GetUserById(id)?.ToString(p => p.LastName, p => p.FirstName)

so that you don't have to format the input when calling the extension.


Overloading python class constructor I am trying to create a very basic class: Line as follows:

class Line:

    def __init__(self, x1, y1, x2=None, y2=None, dx=None, dy=None):
        self._x1 = x1
        self._y1 = y1

        if dx is None:
            if x2 is None:
                raise ValueError("You must specify either x2 or dx. both argument are not passed")
            else:
                self._x2 = x2
                self._dx = dx
        else:
            if x2 is None:
                self._x2 =  self._x1 + dx
                self._dx = dx
            else:
                raise ValueError("Both x2 and dx are passed. You must supply only one of them")

        if dy is None:
            if y2 is None:
                raise ValueError("You must specify either y2 or dy. both argument are not passed")
            else:
                self._y2 = y2
                self._dy = dy
        else:
            if y2 is None:
                self._y2 =  self._y1 + dy
                self._dy = dy
            else:
                raise ValueError("Both y2 and dy are passed. You must supply only one of them")

    def x1(self):
        return self._x1

    def x2(self):
        return self._x2

    def y1(self):
        return self._y1

    def y2(self):
        return self._y2

    def dx(self):
        return self._dx

    def dy(self):
        return self._dy

x1 and y1 arguments are mandatory. But I want:

Only x2 or dx is passed not both.
Only y2 or dy is passed not both.

In the code above I feel that I am repeating my self. How can I achieve that with minimum conditons?

I appreciate your help

I noticed with dx and dy, you're assigning them to self.dx and self.dy regardless of what they are. Those assignments can be moved up with x1 and x2 which thins out the checks a bit.

Unless you really want all four combinations to happen using the same function though, I'd probably break this down into some "pseudo-constructors" that defer to the main constructor. Instead of allowing all possible combinations of data then error checking after, I'd just try to limit up front what options are available. There are four possible input combinations: dx, dy, dx, y2, x2, y2, and x2, dy. You could have four static methods that explicitly request a certain combination, and construct a line accordingly.

In the below code, I also decided to use a NamedTuple here. They're basically a shortcut way of creating an immutable class, complete with a generated constructor, a __repr__ implementation and some other nice features. See the comments doc page and the comments.

Now, I'll admit that this code that I ended up with isn't quite as nice as what I originally saw in my head. I think it is still quite an improvement though in terms of readability:

from typing import NamedTuple, Optional

# NamedTuple automatically creates a constructor that handles all the annoying self._x2 = x2 lines
# It also makes Line immutable. This gets rid of the need for the getters you have
class Line(NamedTuple):
    x1: int  # "Declare" the fields that the tuple will have
    y1: int

    x2: int
    y2: int

    dx: Optional[int] = None  # With defaults of None for some
    dy: Optional[int] = None

    # staticmethod essentially gets rid of the `self` parameter and makes the method
    #  intended to be called on the class itself instead of on an instance.
    @staticmethod
    def from_dx_dy(x1, y1, dx, dy):
        return Line(x1, y1, dx=dx, dy=dy, x2=x1 + dx, y2=y1 + dy)

    @staticmethod
    def from_dx_y2(x1, y1, dx, y2):
        return Line(x1, y1, dx=dx, y2=y2, x2=x1 + dx)

    @staticmethod
    def from_x2_dy(x1, y1, x2, dy):
        return Line(x1, y1, x2=x2, dy=dy, y2=y1 + dy)

    @staticmethod
    def from_x2_y2(x1, y1, x2, y2):
        return Line(x1, y1, x2=x2, y2=y2)

Then, as an example:

>>> Line.from_dx_dy(1, 2, 3, 4)
Line(x1=1, y1=2, x2=4, y2=6, dx=3, dy=4)

>>> Line.from_x2_dy(1, 2, 3, 4)
Line(x1=1, y1=2, x2=3, y2=6, dx=None, dy=4)

It still has an unfortunate amount of duplication, but I think it's easier to make sense of.

As mentioned in the comments, since I'm referring to the class itself in the static methods, they should arguably be classmethods instead:

@classmethod
def from_dx_dy(cls, x1, y1, dx, dy):
    return cls(x1, y1, dx=dx, dy=dy, x2=x1 + dx, y2=y1 + dy)

@classmethod
def from_dx_y2(cls, x1, y1, dx, y2):
    return cls(x1, y1, dx=dx, y2=y2, x2=x1 + dx)

@classmethod
def from_x2_dy(cls, x1, y1, x2, dy):
    return cls(x1, y1, x2=x2, dy=dy, y2=y1 + dy)

@classmethod
def from_x2_y2(cls, x1, y1, x2, y2):
    return cls(x1, y1, x2=x2, y2=y2)

Where cls is referring to the current class (Line). I don't like the look of this as much, but it is the intent of class methods.


Developing an alert notification in AngularJS I'm studying Angular and want to develop my own notification alert. This is what I done, I like it and it's working like a charm but I think that it can be improved:

Do I need to use it in conduction with a service? If yes, how?
Will it be better if it became an E (element)?
I needed to put style="display: none;" (see the HTML code) so it does not appear when the page is loaded and I think that it's not the best way. What do I do?
How can I show and hide more classy, with some animation? CSS? Add/Remove [custom] class?

This is the directive:

myApp.directive('notification', ['$timeout', function ($timeout) {
  return {
    restrict: 'A',
    controller: ['$scope', function ($scope) {
      $scope.notification = {
        status: 'hide',
        type: 'success',
        message: 'Welcome! It\'s yet another angular alert ;)'
      };
    }],
    link: function(scope, elem, attrs) {
      // watch for changes
      attrs.$observe('notification', function (value) {
        if (value === 'show') {
          // shows alert
          $(elem).show();

          // and after 3secs
          $timeout(function () {
            // hide it
            $(elem).hide();

            // and update the show property
            scope.notification.status = 'hide';
          }, 3000);
        }
      });
    }
  };
}]);

This is the HTML:

{{notification.message}}

A simple example on how to use it:

<button id="submit" name="submit" class="btn btn-default" type="submit" ng-click="notification.status = 'show'; notification.message = 'Oh yeah!'; notification.type = 'info';">Show</button>

This is the Plunkr.

One simple thing you can do is remove the need for the $observe and place the template inside the directive and use ngShow to handle the show/hide action.  (also added in scope that has two way binding to the alertData object.)

Updated Directive

    var myApp = angular.module('myApp', []);

    myApp.directive('notification', ['$timeout', function ($timeout) {
        return {
            restrict: 'E',
            template:"
{{alertData.message}}
",
            scope:{
              alertData:"="
            }
        };
    }]);

And html

    <notification alert-data="notification"></notification>
<button id="submit" name="submit" class="btn btn-default" type="submit" ng-click="notification.status = 'show'; notification.message = 'Oh yeah!'; notification.type = 'info';">Show</button>

Updated plunkr